div as child of dl obfuscates the role mapping of dd/dt

RESOLVED FIXED in Firefox 63

Status

()

defect
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: faulkner.steve, Assigned: Jamie)

Tracking

62 Branch
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

When a dl element contains child div elements that are used to group dt/dd elements ,for styling purposes, the roles of the dt/dd elements are obfuscated and this causes issues screen readers being able to interpret the list items and the list size.

refer to test cases: https://s.codepen.io/stevef/debug/GxwaoP

Issues are manifested in NVDA and JAWS (latest versions) with Firefox, but not with Chrome, also no issue with VO in chrome/safari
For Chrome parity, setting this to P2. Jamie, do you agree?
Component: Disability Access → Disability Access APIs
Flags: needinfo?(jteh)
Priority: -- → P2
Product: Firefox → Core
I agree it's a p2 for the role breakage. However, the item counting is a different matter.

I'm not sure about JAWS, but NVDA is just counting the children in the list to report the item count. The only reason this doesn't break in Chrome is that the div gets pruned out of the tree. However, if you give Chrome a reason to make the div exist in the tree:
data:text/html,before<dl><div onClick=";"><dt>foo</dt><dd>bar</dd></div></dl>
now NVDA reports only 1 item when it would normally report 2. So the item counting is something that needs to be addressed by (or in collaboration with) screen readers.

There's also a mismatch between the HTML/Core AAM specs and browsers here. HTML AAM says the ARIA mappings should be used for dt and dd. However, Core AAM doesn't specify an MSAA/IA2 role of ROLE_SYSTEM_LISTITEM for the term role, which is what NVDA at least is using to allow list item quick nav to terms. It may not even be appropriate for ARIA role term to be mapped to list item because ARIA doesn't require terms/definitions to be inside a list. Furthermore, the Core AAM says the role should be exposed in xml-roles, but neither Firefox or Chrome expose this for HTML dt and dd.

I think we should restrict the scope of any patches in this bug to deal with the role breakage. However, beyond that, it seems the whole situation with dl/dt/dd/term/definition is a big ugly mess which needs to be dealt with right across the board.
Flags: needinfo?(jteh)
> it seems the whole situation with dl/dt/dd/term/definition is a big ugly mess which needs to be dealt with right
across the board.

agreed
Can confirm that Firefox thinks there are only 2 list items in the following example, so this affects role="list|listitem" too. 

```
<div role="list">
   <div>
     <div role="listitem"></div> 
     <div role="listitem"></div> 
   </div>
   <div>
     <div role="listitem"></div> 
     <div role="listitem"></div> 
   </div>
</div>
```
Group position for list items with interspersed divs is a different issue. dt/dd don't get group position info at all at this point, even without an interspersed div.
Assignee: nobody → jteh
Comment on attachment 8992905 [details]
Bug 1476347: Fix accessibility for HTML dt/dd with a div as its parent.

https://reviewboard.mozilla.org/r/257740/#review264650


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: accessible/tests/mochitest/tree/test_list.html:152
(Diff revision 1)
>  
>        // span having display:list-item style
>        testAccessibleTree("list9", discAccTree);
>  
> +      // dl with div grouping dt/dd
> +      var tree =

Error: 'tree' is already defined. [eslint: no-redeclare]
Comment on attachment 8992905 [details]
Bug 1476347: Fix accessibility for HTML dt/dd with a div as its parent.

https://reviewboard.mozilla.org/r/257740/#review264706

::: accessible/base/nsAccessibilityService.cpp:231
(Diff revision 2)
> +  if (parent->IsHTMLElement(nsGkAtoms::div)) {
> +    // It is conforming in HTML to use a div to group dt/dd elements.
> +    parent = parent->GetParent();
> +  }
> +
> +  if (parent && parent->IsHTMLElement(nsGkAtoms::dl)) {

This approach doesn't work for dynamic tree updates, I would create an accessible uncondionally, and then checked the hierarchy when exposing role. It'd be also nice to have this case convered by a test.

Also it makes sense to check group position computations, I afraid it may not work properly in  case of obscured dt/dd.
Comment on attachment 8992905 [details]
Bug 1476347: Fix accessibility for HTML dt/dd with a div as its parent.

https://reviewboard.mozilla.org/r/257740/#review264806

r=me, thanks for the ultra-quick fix!
Attachment #8992905 - Flags: review?(mzehe) → review+
Comment on attachment 8992905 [details]
Bug 1476347: Fix accessibility for HTML dt/dd with a div as its parent.

https://reviewboard.mozilla.org/r/257740/#review264890
Comment on attachment 8992905 [details]
Bug 1476347: Fix accessibility for HTML dt/dd with a div as its parent.

https://reviewboard.mozilla.org/r/257740/#review264706

> This approach doesn't work for dynamic tree updates, I would create an accessible uncondionally, and then checked the hierarchy when exposing role. It'd be also nice to have this case convered by a test.
> 
> Also it makes sense to check group position computations, I afraid it may not work properly in  case of obscured dt/dd.

Thanks for the review. Can you explain why this doesn't work for dynamic updates and how to reproduce it? Is aContext null or does GetParent fail or something for dynamic updates? I tried to reproduce a problem, but everything worked as expected (correct tree exposure and roles with the patch) with three test cases:
1. Adding a dl containing a div:
data:text/html,<div id="content">before</div><button onClick="content.innerHTML += '<dl><div><dt>foo</dt><dd>bar</dd></div></dl>';">Add</button>
2. Adding an additional div to an existing dl:
data:text/html,before<dl id="dl"><div><dt>a</dt><dd>1</dd></div></dl><button onClick="var div = document.createElement('div'); div.innerHTML = '<dt>b</dt><dd>2</dd>'; dl.appendChild(div);">Add</button>
3. Adding a dt/dd to an existing empty div inside a dl:
data:text/html,before<dl id="dl"><div><dt>a</dt><dd>1</dd></div><div id="second"></div></dl><button onClick="second.innerHTML = '<dt>b</dt><dd>2</dd>';">Add</button>
Can you provide a test case which might reproduce a problem with this code?

Regarding group position, we don't do group position at all even for a normal dl, probably because of the intervening dd elements between the dt elements. For example:
data:text/html,<dl><dt>a</dt><dd>b</dd><dt>c</dt><dd>d</dd></dl>
None of the children of the dl have group position, with or without the patch.
You need something that won't trigger a recreation of the whole subtree, i.e. when dt/dd accessible won't be recreated, but moved within the tree. I have to say it may not happen because of DOM mutations you experimented with, because iirc they always come as remove/insert pairs. You may argue the spec refers to DOM hierarchy, and we should ignore accessible hierarchy, but it's something to consider I'd say. So the examples may looks a bit artificial, but they may give a sense of a potential problem. I didn't try it myself, but it makes sense to play with it a bit:

<dl id="dl"><div id="top"><div style="visibility:visible"><dt>a</dt><dd>1</dd></div></div></dl>
which shouldn't create accessible for dt and dd, because they are inside double div, and then
document.getElementById('top').style.visibility = 'hidden';
will hide top div from the accessibility hierarchy, but it doesn't recreate dt/dd accessibles.

You could try to do the opposite:
<dl id="dl"><div id="div0"><dt>a</dt><dd>1</dd></div></dl>
will create accessibles for dt/dd, but when you will move div0 by aria-owns for example, it won't shut them down.

While I don't have a working demo on top of my head, I worry people can hit that in the wilds. This is why I said it makes sense to create hypertext accessible for those unconditionally, but expose correct role/states depending on current hierarchy (yeah, platform API may not like role change).

I take your point regarding group position (I guess there's some role issue in group position computation), but should they expose group position at all?
Ah, I understand what you're saying.

The spec is pretty clear about dl allowing div children with dt/dd children. It doesn't say this is permitted for descendants (multiple levels of divs). What we're really trying to do here is enforce the HTML rules about when these elements are actually valid. There are no such rules for accessibility; e.g. there's no rule anywhere saying that a dt accessible can't be the child of a div outside of a dl due to aria-owns.

It's true that we can expect anything to happen in the wild, but I also don't think this is going to "break" anything. It might be a bit odd, but the author has to take some responsibility for doing silly non-spec things, so long as we don't crash or leak memory or something.

I'm not comfortable with mutating the role at runtime. If you're not convinced by the above, I'd suggest we just get rid of the checks altogether and just always unconditionally create dt and dd accessibles with the correct role, regardless of the HTML rules. FWIW, this is what Chrome does.
Regarding group position, I'm honestly not sure what I think there, and it looks like no screen reader actually uses that info for the dl/dt/dd case right now anyway. IMO, that's outside the scope of this bug and requires a broader browser/AT vendor/spec discussion. Certainly, we're not regressing anything; it isn't exposed already.
let's go with your/spec approach then, it looks weird to create listitem role for dt outside dl.
Comment on attachment 8992905 [details]
Bug 1476347: Fix accessibility for HTML dt/dd with a div as its parent.

Turns out we do need to use HTMLLIAccessible for dt (but not dd). Otherwise, dt doesn't get the read-only state, which is wrong and breaks other tests.

Marco, can you please review again?
Attachment #8992905 - Flags: review+ → review?(mzehe)
Comment on attachment 8992905 [details]
Bug 1476347: Fix accessibility for HTML dt/dd with a div as its parent.

https://reviewboard.mozilla.org/r/257740/#review265294

Hm, ReviewBoard shows this as if I'd already r+'d it, let's see if it accepts this.
Attachment #8992905 - Flags: review?(mzehe) → review+
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab79c47390b5
Fix accessibility for HTML dt/dd with a div as its parent. r=MarcoZ
https://hg.mozilla.org/mozilla-central/rev/ab79c47390b5
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.