Closed Bug 1455357 Opened 6 years ago Closed 6 years ago

Setting grid item to display:contents resets its accessible role

Categories

(Core :: Disability Access APIs, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: hidde, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

CSS Grid works on direct children of the grid container. To be able to place ‘grand children’ onto a CSS Grid, one can set 'display: contents' to a grid item. Its children can then be placed on the grid. 

When the  grid item is set to `display: contents` in CSS, its accessible role changes to ‘text leaf’.

Steps to reproduce:

1. Create a grid by setting 'display: grid' on a div element
2. Place some HTML in it, and make on of the items a <ul> with a couple of <li> elements
3. Inspect the <ul> item in the accessibility tree and see that it has role=list
4. Set 'display: contents' on the UL with CSS

Expected result: UL still has accessible role of 'list'. The CSS only affects the layout.

Actual result: UL now has accessible role of 'text leaf'. The CSS has affected more than just layout.

(Codepen example: https://codepen.io/hidde/pen/gzbMeL)

(Note, I have verified if the same happens in other browsers' trees: Safari still makes it as list, Chrome makes it unexposed).
I think we have a larger problem here. In Nightly, I can't get this Codepen to work. The sponsors list isn't in the a11y tree at all.

On this page:
https://rachelandrew.co.uk/archives/2016/01/29/vanishing-boxes-with-display-contents/
the content of the sample iframe doesn't appear in the a11y tree, even though it's visible on screen. If you switch the view to HTML and then back to the result, it appears in the a11y tree. So, it seems the initial load doesn't populate the a11y tree for display: contents.

With the Codepen from comment 0, I can also get the tree to be populated as described in the actual result if I hide the iframe (set display: none) and then show it again (clear display).

The display attribute affecting semantics is similar to bug 1005271. However, I'm not sure whether special casing tags is appropriate here. We really don't want display: contents to ever affect semantics.

Alex, thoughts?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(surkov.alexander)
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
(In reply to James Teh [:Jamie] from comment #1)

> The display attribute affecting semantics is similar to bug 1005271.
> However, I'm not sure whether special casing tags is appropriate here. We
> really don't want display: contents to ever affect semantics.

We have UL in the markup map, so it has to be something different from bug 1005271. It appears display:contents results in unexpected accessible type, possibly related with parent display:grid.

I tried to make some tests on Nightly to investigate the issue further, but keep running into some nasty crash.
Flags: needinfo?(surkov.alexander)
This is becoming something of a hot topic on the Twitters. Other browsers have problems there, too.
display:contents seems quite special, it makes us to not create accessible at all:

data:text/html,<ul style="display: contents"><li>hey</li><li>hey2</li></ul>

Daniel, do you have any ideas how display:contents may be so special?
Flags: needinfo?(dholbert)
Blocks: treea11y
Mats is probably a better person to ask (in part because he implemented display:contents, and in part because I am mostly AFK on vacation :)
Flags: needinfo?(dholbert) → needinfo?(mats)
Fwiw, the spec for display:contents is here:
https://drafts.csswg.org/css-display/#box-generation

The essence of it is that a display:contents element does not have
a box (like display:none), but its children still have boxes (unlike
display:none).

My guess is that the reason something like <ul style="display: contents">
doesn't have a role is that we check to see if the element has
a primary frame (which it doesn't) and then assume it's because
it's display:none and then suppress it.  Right around here maybe:
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/accessible/base/nsAccessibilityService.cpp#1073

I tend to think that <ul style="display: contents"><li>hey</li><li>hey2</li></ul>
should behave exactly as if it were <li>hey</li><li>hey2</li>.
I don't see a reason for the <ul> to show up in the a11y tree at all.
You could also have something like this:
<ul>
  <ul style="display: contents">
  <ul style="display: contents">
  <ul style="display: contents">
    <li>hey</li><li>hey2</li></ul>

which I think it should behave the same as:
<ul>
    <li>hey</li><li>hey2</li></ul>

In my opinion the a11y tree should behave similarly to the box tree -
it should NOT create an a11y node for the display:contents element
itself, but it should create a11y nodes for the element's children
just as if the display:contents element didn't even exist.

FYI, for testing if an element is display:contents there is Element::IsDisplayContents():
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/dom/base/Element.h#1440
Flags: needinfo?(mats)
If the a11y tree already have some concept of "ignore" or "transparent"
nodes, then I guess that could be an alternative as well rather than not
creating the node at all.  Then again, such things tend to be error
prone for code that is conditional on the parent's role.  It's easy to
forget that the parent might be "transparent" and they should have
tested the nearest non-transparent ancestor instead...
Thank you, mats, for the clarifications and your thoughts!

(In reply to Mats Palmgren (:mats) from comment #6)
> Fwiw, the spec for display:contents is here:
> https://drafts.csswg.org/css-display/#box-generation
> 
> The essence of it is that a display:contents element does not have
> a box (like display:none), but its children still have boxes (unlike
> display:none).

However, here https://drafts.csswg.org/css-display/#the-display-properties, it is explicitly stated:

> the display property only affects visual layout: its purpose is to allow designers freedom to change the layout behavior of an element without affecting the underlying document semantics.

And the only exception noted is display:none;, which has been hiding elements from the accessibility tree on purpose forever.

> I tend to think that <ul style="display:
> contents"><li>hey</li><li>hey2</li></ul>
> should behave exactly as if it were <li>hey</li><li>hey2</li>.

And this clashes directly with the spec (see above), and the expectation of everyone using proper semantic HTML. The notion, which became crystal clear over the past two to three years, is that, whatever is done in CSS, should not have a profound impact on the exposure to assistive technologies like screen readers. In earlier years, Surkov and I were thinking the same thing as you suggest, that we should follow the box/frame model of layout, but as CSS advanced more and more, giving visual styling many more capabilities, it became clear that this trumps the HTML semantics intended by the author more often than it actually helps it. So, we've shifted over the past few years to giving HTML semantics priority over the box/frame model that layout has, because the latter too often conflicts with the author's intentions.

> I don't see a reason for the <ul> to show up in the a11y tree at all.

Again: The assumption by web developers who are consciously making the right semantic decisions is that what they write in the HTML does get reflected by the assistive technology or the accessible tree in browsers, regardless of what visual styling they apply. The only semantic they actually want to have an impact is display:none; or visibility:hidden;.

> You could also have something like this:
> <ul>
>   <ul style="display: contents">
>   <ul style="display: contents">
>   <ul style="display: contents">
>     <li>hey</li><li>hey2</li></ul>
> 
> which I think it should behave the same as:
> <ul>
>     <li>hey</li><li>hey2</li></ul>

For this case, if the author really needs to do something like this, they should explicitly tell the AT to ignore the interim ul elements by setting role="presentation" on those.

> In my opinion the a11y tree should behave similarly to the box tree -
> it should NOT create an a11y node for the display:contents element
> itself, but it should create a11y nodes for the element's children
> just as if the display:contents element didn't even exist.

Your example where there are list items without a containing list around them creates all sorts of semantic disconnect. In Firefox, even without any CSS, we only create text frames for loose list items. In Chrome and Edge, list items without a list container are being created. Lists are very important container elements for screen readers that can be either navigated to, or quickly navigated out of. Taking thhis away just because of some visual styling sounds very wrong to me, web developers in the accessibility community, and other users.

> FYI, for testing if an element is display:contents there is
> Element::IsDisplayContents():
> https://searchfox.org/mozilla-central/rev/
> c0d81882c7941c4ff13a50603e37095cdab0d1ea/dom/base/Element.h#1440

Thank you! That will help a great deal! :)

BTW: This tweet from Fantasai also confirms that display:contents was explicitly invented to aid accessibility, so we definitely need to do the right thing and make sure semantics always win in these cases:
https://twitter.com/fantasai/status/979027939885948928

Thank you, Hidde, for finding that tweet and also the relevant section in the spec that gives us reassurance that letting semantics win over the box model is the right thing to do.
OK, that tweet from @fantasai makes it clear what the spec intention is.

So, creating a node in CreateAccessible for
Element::IsDisplayContents() nodes should handle it then.
Attached patch patch (obsolete) — Splinter Review
mats's suggestion, should work in most cases, but I suppose it doesn't in case of HTML:tables <table style="display:contents"><tr><td>. Also <div role="table" style="display:contents"> might be problematic. I would suggest to check those examples and file separate bugs on those.
Assignee: nobody → surkov.alexander
Attachment #8974393 - Flags: review?(mzehe)
Priority: -- → P3
I am running a local build with this patch, and what I see is:

1. The example Jamie gave in comment #1 now works.
2. The example Hidde links to in comment #0 works from a pure accessibility tree point of view as well, BUT the width and height of the list are 0, so NVDA doesn't render it in its virtual buffer. The output, when you navigate to the list container with NVDA's object nav, and then press NVDA+F1 to get object properties, looks like this on my machine:

INFO - globalCommands.GlobalCommands.script_navigatorObject_devInfo (17:26:45.941):
Developer info for navigator object:
name: None
role: ROLE_LIST
states: STATE_READONLY, STATE_INVISIBLE
isFocusable: False
hasFocus: False
Python object: <NVDAObjects.Dynamic_ListBrokenFocusedStateMozillaIAccessible object at 0x05E153F0>
Python class mro: (<class 'NVDAObjects.Dynamic_ListBrokenFocusedStateMozillaIAccessible'>, <class 'NVDAObjects.IAccessible.List'>, <class 'NVDAObjects.IAccessible.mozilla.BrokenFocusedState'>, <class 'NVDAObjects.IAccessible.mozilla.Mozilla'>, <class 'NVDAObjects.IAccessible.ia2Web.Ia2Web'>, <class 'NVDAObjects.IAccessible.IAccessible'>, <class 'NVDAObjects.window.Window'>, <class 'NVDAObjects.NVDAObject'>, <class 'documentBase.TextContainerObject'>, <class 'baseObject.ScriptableObject'>, <class 'baseObject.AutoPropertyObject'>, <type 'object'>)
description: u''
location: RectLTWH(left=0, top=0, width=0, height=0)
value: None
appModule: <'firefox' (appName u'firefox', process ID 14228) at address 60a7d10>
appModule.productName: u'Nightly'
appModule.productVersion: u'62.0a1'
TextInfo: <class 'NVDAObjects.IAccessible.IA2TextTextInfo'>
windowHandle: 6818286L
windowClassName: u'MozillaWindowClass'
windowControlID: 0
windowStyle: 382664704
windowThreadID: 14628
windowText: u'Grid, children and grand children - Nightly'
displayText: u''
IAccessibleObject: <POINTER(IAccessible2) ptr=0x8c3571c at 61f2c60>
IAccessibleChildID: 0
IAccessible event parameters: windowHandle=6818286L, objectID=-4, childID=-50333769
IAccessible accName: None
IAccessible accRole: ROLE_SYSTEM_LIST
IAccessible accState: STATE_SYSTEM_READONLY, STATE_SYSTEM_INVISIBLE, STATE_SYSTEM_VALID (32832)
IAccessible accDescription: u''
IAccessible accValue: None
IAccessible2 windowHandle: 6818286
IAccessible2 uniqueID: -50333769
IAccessible2 role: ROLE_SYSTEM_LIST
IAccessible2 states: IA2_STATE_SELECTABLE_TEXT (4096)
IAccessible2 attributes: u'tag:ul;class:sponsors;'

The important line is:

location: RectLTWH(left=0, top=0, width=0, height=0)

So yes, it is fixed, but not in a way NVDA considers valid for rendering. ;)
Comment on attachment 8974393 [details] [diff] [review]
patch

cancelling review since this is not yet complete for assistive technologies.
Attachment #8974393 - Flags: review?(mzehe)
(In reply to Marco Zehe (:MarcoZ) from comment #11)

> The important line is:
> 
> location: RectLTWH(left=0, top=0, width=0, height=0)
> 
> So yes, it is fixed, but not in a way NVDA considers valid for rendering. ;)

do I understand correct that NVDA doesn't render it because it has zero boundaries? If so, then I afraid we can do nothing about it - we make sure we expose the semantics and no layout information (by display:contents definition). I'm not sure whether style="position: relative, top: x, right: y, width: w, height:h " will fix a problem. If yes, then this is authoring error, otherwise NVDA's bug I'd say, or display:contents is poorly designed.
This is a question for Jamie, who knows the history behind the NVDA decision, to answer. I keep running into these problems on different occasions where 0 width elements, even with text in them, aren't rendered in the buffer even though its accessible properties are properly calculated. And this time, it's a container that hides elements who do have defined left and top positions *and* width and height. Perhaps if we expose an object attribute indicating that the 0 width and height are due to display:conents, NVDA can be adjusted to handle that. Currently, as you can see above, this particular display property isn't exposed as an object attribute.
Flags: needinfo?(jteh)
(In reply to Marco Zehe (:MarcoZ) from comment #14)
> I keep running into these problems on different
> occasions where 0 width elements, even with text in them, aren't rendered in
> the buffer even though its accessible properties are properly calculated.

I still believe this to be the correct decision. If an element containing only text has a 0 width and height, it's not visible and thus shouldn't be rendered. However...

> And this time, it's a container that hides elements who do have defined left
> and top positions *and* width and height.

The 0 width/height rule in NVDA only applies if the child count is also 0. In other words, containers with 0 width/height should still get rendered. Only if the node has no children (e.g. a button with 0 width and height) should NVDA treat it as invisible. That should not be true in this case.

I'd need to build this and look at it to figure out what's going on.
Marco, are you sure we don't want to take the patch, because it seems it makes our implementation closer to where we should be.
Flags: needinfo?(mzehe)
1. We were exposing the invisible state for elements with display: contents. Technically, the element itself as it would normally be rendered *is* invisible, but this seems semantically wrong to me. Note that this doesn't actually make a difference to NVDA.
2. We weren't exposing embedded object characters for these elements because they have no frame. This is what was breaking NVDA. With this fix, the test case in comment 0 works as expected.

Simpler test case:
data:text/html,a<ul style="display: contents;"><li>b</li></ul>c
Alex, I imagine you'll want to roll this patch into yours (and perhaps tweak it and/or add tests). I'll leave it to you as to how to proceed.
Flags: needinfo?(jteh)
Yes, please roll Jamie's patch into yours, Alex, and add a test for the embedded characters. I'll be happy to review that one. :)
Flags: needinfo?(mzehe)
(In reply to James Teh [:Jamie] from comment #17)
> Created attachment 8974874 [details] [diff] [review]
> For elements with display: contents, don't expose the invisible state and do
> expose embedded object characters.
> 
> 1. We were exposing the invisible state for elements with display: contents.
> Technically, the element itself as it would normally be rendered *is*
> invisible, but this seems semantically wrong to me. Note that this doesn't
> actually make a difference to NVDA.

this part worries me, since we report visible for something not rendered, which has no sense with me

> 2. We weren't exposing embedded object characters for these elements because
> they have no frame. This is what was breaking NVDA. With this fix, the test
> case in comment 0 works as expected.

good catch, I suppose we may have a bunch of such cases through the code - it's very new for Gecko to have an accessible object in no frame case
(In reply to alexander :surkov from comment #20)
> (In reply to James Teh [:Jamie] from comment #17)
> > Created attachment 8974874 [details] [diff] [review]
> > For elements with display: contents, don't expose the invisible state and do
> > expose embedded object characters.
> > 
> > 1. We were exposing the invisible state for elements with display: contents.
> > Technically, the element itself as it would normally be rendered *is*
> > invisible, but this seems semantically wrong to me. Note that this doesn't
> > actually make a difference to NVDA.
> 
> this part worries me, since we report visible for something not rendered,
> which has no sense with me

Jamie, are you sure about 'invisible' part, especially in the light of it doesn't have any effect on NVDA?

Mats, if somebody specifies the size of display:contents, probably by changing its positioning, then will layout create a frame for such element?
Flags: needinfo?(mats)
Flags: needinfo?(jteh)
(In reply to alexander :surkov from comment #21)
> Mats, if somebody specifies the size of display:contents, probably by
> changing its positioning, then will layout create a frame for such element?

No, a display:contents element never has a box.
It is literally handled as if the element didn't exist and instead is
replaced by its children during box construction.  It still matches
CSS selectors and has style values though, so if a child element has
a 'property:inherit' style it will inherit the value of 'property'
from the display:contents element's value.

Note that display:contents elements can still have pseudo-elements
though (which are treated the same as any other child element).
For example:

<style>
div::before { content: "before"; }
div::after { content: "after"; }
</style>
<div style="display:contents; color:red"></div>

will have the box tree:
Block(body)
  Inline(div::before)
    Text "before"
  Inline(div::after)
    Text "after"

The text color is red since 'color' inherits by default.

In general though, for a DOM hierarchy <a><b><c/><d/>...
where <b> is display:contents, we will create frames as
if it were <a><c/><d/>...
Flags: needinfo?(mats)
(In reply to alexander :surkov from comment #21)
> > [Not exposing the invisible state] worries me, since we report visible for something not rendered,
> > which has no sense with me

This is one of those tricky areas where presentation and semantics conflict. Obviously, the list doesn't render the box it normally would. However, its content is rendered, which essentially means the list is rendered; it's just rendered in an author defined way rather than via normal rendering. It's not "invisible" semantically speaking.

> Jamie, are you sure about 'invisible' part, especially in the light of it
> doesn't have any effect on NVDA?

I'm sure. I'd be fine with exposing the off-screen state if that makes you happier, but not invisible. Exposing invisible would be just one more reason clients can't trust the invisible state. That's the whole reason NVDA has the width > 0 && height >0 hack.

It doesn't affect NVDA browse mode, but NVDA's object navigation functionality with "simple review" enabled (default) will skip any object (and its descendants) with the invisible state.
Flags: needinfo?(jteh)
(In reply to James Teh [:Jamie] from comment #23)
> (In reply to alexander :surkov from comment #21)
> > > [Not exposing the invisible state] worries me, since we report visible for something not rendered,
> > > which has no sense with me
> 
> This is one of those tricky areas where presentation and semantics conflict.
> Obviously, the list doesn't render the box it normally would. However, its
> content is rendered, which essentially means the list is rendered; it's just
> rendered in an author defined way rather than via normal rendering. It's not
> "invisible" semantically speaking.
> 
> > Jamie, are you sure about 'invisible' part, especially in the light of it
> > doesn't have any effect on NVDA?
> 
> I'm sure. I'd be fine with exposing the off-screen state if that makes you
> happier, but not invisible. Exposing invisible would be just one more reason
> clients can't trust the invisible state. That's the whole reason NVDA has
> the width > 0 && height >0 hack.
> 
> It doesn't affect NVDA browse mode, but NVDA's object navigation
> functionality with "simple review" enabled (default) will skip any object
> (and its descendants) with the invisible state.

I'm with Jamie on this one.
Attached patch patch2Splinter Review
+jamie's one+tests
Attachment #8974393 - Attachment is obsolete: true
Attachment #8974874 - Attachment is obsolete: true
Attachment #8975563 - Flags: review?(mzehe)
Comment on attachment 8975563 [details] [diff] [review]
patch2

Review of attachment 8975563 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits fixed, thanks!

::: accessible/base/nsAccessibilityService.cpp
@@ +1054,4 @@
>    // Check frame and its visibility. Note, hidden frame allows visible
>    // elements in subtree.
>    if (!frame || !frame->StyleVisibility()->IsVisible()) {
> +    // display:contents element doesn't have a frame, but retains the semantcis.

Nit: "semantics", c and i are swapped.

@@ +1054,5 @@
>    // Check frame and its visibility. Note, hidden frame allows visible
>    // elements in subtree.
>    if (!frame || !frame->StyleVisibility()->IsVisible()) {
> +    // display:contents element doesn't have a frame, but retains the semantcis.
> +    // All its children are uneffected.

Nit: "unaffected".

::: accessible/generic/Accessible.cpp
@@ +324,5 @@
>  Accessible::VisibilityState() const
>  {
>    nsIFrame* frame = GetFrame();
> +  if (!frame) {
> +    // Element having display:conents is considered visible semantically,

Nit: "display:contents", missing t in the middle.

@@ +325,5 @@
>  {
>    nsIFrame* frame = GetFrame();
> +  if (!frame) {
> +    // Element having display:conents is considered visible semantically,
> +    // despite it doesn't have a virusal box.

Nit: "visual box" or "visually visible box".
Attachment #8975563 - Flags: review?(mzehe) → review+
I need to set up a spelling add-on :) Thanks for the catches!
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c199ef8f3207
Setting grid item to display:contents resets its accessible role, patch=surkov,jamie, r=marcoz
https://hg.mozilla.org/mozilla-central/rev/c199ef8f3207
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Verified fixed in Firefox 62.0a1 (20180515220059).
Status: RESOLVED → VERIFIED
Depends on: 1470838
See Also: → 1494196
In Firefox 62.0.3 Windows and Mac, plus 64 Nightly, this issue appears to either not be fixed or regressed. The button, with or without an ARIA role, is not exposed to AT (JAWS 2018 nor NVDA) as a button. In the Firefox accessibility inspector it reports as a "text leaf".

I used a test page from Scott O'Hara: https://s.codepen.io/scottohara/debug/GYoBgZ/WPALYNWgeOJk and a minimal test page of my own: https://s.codepen.io/aardrian/debug/KGBbQd

Others on different systems have confirm the what I am experiencing.
It appears we haven't yet caught all instances where our code needs to deal with display:contents;. The cases fixed in this bug are still fixed, our tests would have indicated otherwise if things got broken. We'll deal with these new findings in a new bug. Sorry about that, but it appears there are differences in when display:contents; is applied to what elements. :(
You need to log in before you can comment on or make changes to this bug.