Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid HTML structure in search result item #1395

Closed
ggeisler opened this issue Jan 24, 2023 · 5 comments · Fixed by #1468
Closed

Invalid HTML structure in search result item #1395

ggeisler opened this issue Jan 24, 2023 · 5 comments · Fixed by #1468
Assignees

Comments

@ggeisler
Copy link
Contributor

Within the search result item structure there is this code for displaying the breadcrumb links/path to the item:

<dl class="document-metadata dl-invert row">
    <div class="breadcrumb-links">
         <ol class="breadcrumb">
           <li class="breadcrumb-item">
              ...

This raises an accessibility error, and is also just invalid HTML, I believe. The dl can only contain dt and dd elements, so the child div is invalid, as is the subsequent ol.

Probably fixable just by changing the dl to a div? However, if the result item has metadata to display aside from the breadcrumb path (e.g., an abstract/scope value), that part is also within the dl and includes an additional invalid div:

<dl class="document-metadata dl-invert row">
    <div class="row">
       <dt class="visually-hidden">Abstract Or Scope</dt>
         <dd class="col al-document-abstract-or-scope truncator truncated" data-controller="arclight-truncate">
            <div class="content" data-arclight-truncate-target="content"><p>"Broadcasting", mainly ...
           ...

Basically it seems we're using a dl for the result item metadata following the basic Blacklight pattern (where the dl is used correctly and appropriately for displaying several field labels and values), but ArcLight result items display very different item metadata, so the dl ends up interspersed with a lot of invalid elements.

Perhaps the whole result item structure in ArcLight should be rewritten (ignoring the Blacklight way of doing it) with an eye on valid HTML.

The code in question is what renders this:

Screen Shot 2023-01-24 at 11 01 24 AM

@cbeer
Copy link
Member

cbeer commented Jan 31, 2023

@ggeisler
Copy link
Contributor Author

Ah, interesting. One of the accessibility checking tools I used (Axe DevTools) says this:

dl element has direct children that are not allowed: div > ol

Either way, based on the documentation, yes, I think the second part of my initial description that says we shouldn't have a div within the dl is incorrect.

I guess I'm not completely clear on whether the first part -- whether the dl can have a div that then contains an ol -- is still true, or whether the Axe DevTools is incorrect (or maybe is using a different/outdated spec for determining what is allowable).

Regardless, this doesn't seem to have a negative effect on the UI, and the other accessibility tools I used didn't mention this issue, so perhaps I should close this ticket?

@cbeer
Copy link
Member

cbeer commented Jan 31, 2023

A name-value group consists of one or more names (dt elements, possibly as children of a div element child) followed by one or more values (dd elements, possibly as children of a div element child), ignoring any nodes other than dt and dd element children, and dt and dd elements that are children of div element children.


If the element is a child of a dl element: one or more dt elements followed by one or more dd elements, optionally intermixed with script-supporting elements.

🤔 . It sounds like a dl > div isn't the same as a regular old div... so, I guess I agree we should do something better?

@dinahhandel
Copy link

Needs developer investigation but can be worked on.

@lfarrell lfarrell self-assigned this Dec 7, 2023
@lfarrell
Copy link
Contributor

lfarrell commented Dec 7, 2023

Fix PR for: #1468

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants