1005271 - Data Table with display:block exposes no table semantics

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Nurthen]

Attachment: table_acc_scrolling.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36

Steps to reproduce:

Run the testcase. The table has display:block and various other cells within the table have other display attributes. This is to allow the table body to be scrollable. Note that the scripting to scroll the table header is not present in this example.


Actual results:

The a11y API does not show a table. 
Both Chrome and IE expose a table structure to the accessibility APIs for this table.


Expected results:

I expect the table to be exposed to the accessibility APIs with the correct headers etc  as specified.

Comment 1

[Marco Zehe (:MarcoZ)]

The issue is actually more serious than that. If we would just turn this into a layout table, accProbe would still see a table structure, but with an object attribute denoting that we guessed this to be a layout table. The fact that we don't create accessible objects with a table interface at all means that something prevents us from recognizing this as a real table at all. Sounds pretty serious to me. Alex, can you investigate?

Comment 2

[alexander :surkov (:asurkov)]

I don't have strong opinion on this example. display style affects on accessible type, here HTML table has display:block, it's not a table from layout perspective and thus it's not a table for accessible.

Comment 3

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #2)
> I don't have strong opinion on this example. display style affects on
> accessible type, here HTML table has display:block, it's not a table from
> layout perspective and thus it's not a table for accessible.

OK, so, our layout engine doesn't consider this to be a data table?

James, is this construct even supposed to resemble a data table?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #3)
> (In reply to alexander :surkov from comment #2)
> > I don't have strong opinion on this example. display style affects on
> > accessible type, here HTML table has display:block, it's not a table from
> > layout perspective and thus it's not a table for accessible.
> 
> OK, so, our layout engine doesn't consider this to be a data table?

layout doesn't operate with data and layout terms at all, that's pure a11y terms. but layout doesn't consider this as a table at all and thus we do the same on a11y side.

Comment 5

[James Nurthen]

(In reply to Marco Zehe (:MarcoZ) from comment #3)
> (In reply to alexander :surkov from comment #2)
> > I don't have strong opinion on this example. display style affects on
> > accessible type, here HTML table has display:block, it's not a table from
> > layout perspective and thus it's not a table for accessible.
> 
> OK, so, our layout engine doesn't consider this to be a data table?
> 
> James, is this construct even supposed to resemble a data table?

Yes - this is part of a table component of a UI component library. It is a scrollable data table.

Comment 6

[Marco Zehe (:MarcoZ)]

OK, so as far as I understand it, this is not an a11y bug at all in the first place, but a layout one. Because the CSS applied here tells our layout engine that this is not a table, and that this shouldn't be constructed as such when layouting the frames. a11y only picks up what frames tell us, we don't do a strict HTML parsing without considering CSS also, so we ask our layout engine rather than also parsing the CSS.

So we need to find out if a) this is supposed to be a table frame construct, and thus our layout engine has a bug, or b) the markup is incorrect.

Comment 7

[alexander :surkov (:asurkov)]

it's not layout bug, imo it's another example of craziness of the wild web, I guess the author can find out the accessible implementation of scrollable table but that example is what we deal with: we have inaccessible implementation of scrollable table in the web. I'm totally fine if Firefox was smart enough to process this case properly but the problem is it's not evident how to distinguish this case from mirroring case where the author doesn't want the table to be a table.

Comment 8

[James Nurthen]

If we (In reply to alexander :surkov from comment #7)
> it's not layout bug, imo it's another example of craziness of the wild web,
> I guess the author can find out the accessible implementation of scrollable
> table but that example is what we deal with: we have inaccessible
> implementation of scrollable table in the web. I'm totally fine if Firefox
> was smart enough to process this case properly but the problem is it's not
> evident how to distinguish this case from mirroring case where the author
> doesn't want the table to be a table.

If we didn't want it to be a table we would add role="presentation" to it. I find it crazy that the CSS (presentation) affects the objects exposed to the a11y APIs and that you ignore the semantics we have specified in the HTML file.

Comment 9

[alexander :surkov (:asurkov)]

a11y should expose what's on the screen, that's a primary reason why we are layout based, so if HTML table is not a table from layout perspective then we don't expose it as a table for a11y. I guess we could expose any HTML table, which is styled as non table, as a table if the author doesn't have any reason to use HTML table for non tables but I can neither confirm nor refute it. Nevertheless the change will be rather untrivial since we need to validate HTML tables on a11y side (like table can include tr, tr can include td, th, etc).

I'm curious how the given example is widespread in the web.

Comment 10

[James Nurthen]

I disagree. a11y should expose what the author intends the semantics to be. This is what is defined in the HTML not in the HTML combined with CSS.

This is in a component library which is currently under development. If this isn't resolved then we will have to fall back to a different renderer for firefox (the same as IE9) which doesn't look as nice and loses some of the scrolling functionality.

If there was another way to do what we need to do we would do so.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to James Nurthen from comment #10)
> I disagree. a11y should expose what the author intends the semantics to be.
> This is what is defined in the HTML not in the HTML combined with CSS.

I see. It's not easy issue.

> If there was another way to do what we need to do we would do so.

it doesn't relate to this bug but I don't understand why that control is considered as a table at all because there's no visual relation between headers and table data, if so then I would use HTML table for data and bold labels on top for headers plus aria-labelledby for a11y relations.

Comment 12

[James Nurthen]

There is extra scripting in the real code which scrolls the table headers when the body is scrolled to keep that relationship visible. I mentioned this in my original comment.

There is also additional styling which makes it look more like a data table.

A few more examples of this sort of thing

http://www.imaputz.com/cssStuff/bigFourVersion.html
http://tjvantoll.com/2012/11/10/creating-cross-browser-scrollable-tbody/

Comment 13

[alexander :surkov (:asurkov)]

(In reply to James Nurthen from comment #12)

> http://www.imaputz.com/cssStuff/bigFourVersion.html
> http://tjvantoll.com/2012/11/10/creating-cross-browser-scrollable-tbody/

from what I can tell these are different from original example, because I can see table accessible there. There's no table cell accessibles as well what looks like a bug. 

In ordinal tables we don't expose tbody and etc in accessible tree. Here they get accessible objects and that prevents td and tr to have accessibles (because of wrong hierarchy). We could ignore tbody and do not create accessible for them what would fix the issue I guess. But I'm curious if AT needs to have tbody accessible because it's scrollable and if so then what role should it have.

Comment 14

[alexander :surkov (:asurkov)]

It's helpful to have feedback from Jamie (comment #13).

Comment 15

[James Teh [:Jamie]]

The only use I can think of for the tbody accessible (and it's a long shot) is if it exposed some programmatic method for scrolling, which might be useful for touch screen a11y.

Comment 16

[alexander :surkov (:asurkov)]

We never exposed scrollbar accessibles for scrollable area but we could do that (bug 285167) as alternative to designing the new interface.

On the another hand the user can scroll the area by keyboard. I'm curious if existence of accessible for scrollable area is helpful to say to the user he can scroll.

If we had an accessible for scrollable then what role should it have? Could it be a problem for NVDA some way?

Comment 17

[alexander :surkov (:asurkov)]

James N, are you ok to change bug summary to "scrollable tbody breaks table hierarchy"?

Comment 18

[James Nurthen]

So long as we address the original testcase I don't really mind what the summary is.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to James Nurthen from comment #18)
> So long as we address the original testcase I don't really mind what the
> summary is.

ok, I'm asking because the proposed bug fix won't be a fix for the bug as it was reported originally

Comment 20

[James Nurthen]

Ah - then I guess I do mind. If it doesn't address the original issue then it isn't much use to me.

Comment 21

[alexander :surkov (:asurkov)]

so original bug was about to create table hierarchy for tables having display:table style. The example you provided (comment #12) don't work because of scrollable tbody problem and its correct work isn't looking related with the original bug report.

So basically we deal here with two separate issues. I have concerns about original bug report but I'm fine to fix tbody issue. So if you still need the former issue fixed then perhaps it makes sense to open another bug for it. Sounds good?

Comment 22

[alexander :surkov (:asurkov)]

I filed bug 1020510 for scrollable tbody issue

Comment 23

[Joseph Scheuhammer (:clown)]

I see two extremes here.

1.  If the table is truly a data table (and it is by assumption), then it should be exposed as a data table in the a11y API.

2. However, authors can do wacky things with CSS.  For example, if the author uses CSS to make a data table look and behave like an unordered list, then, for all intents and purposes, it is an unordered list, and it should be exposed in the a11y tree as a list.  But, the only way that will happen is if the author also uses aria to re-purpose the table markup and "force" a11y list semantics.  BTW, I'd advise the author to not use table markup with CSS to make a list, but to use normal list markup in the first place.

I'm not sure where a data table with CSS display:block falls between these two extremes.  Looking at James' first example (table_acc_scrolling.html) there are clearly column headers, rows, and a relationship between them and the cells.  For example, the department with ID 30 has the Name "Purchasing", and location and manager IDs of 200 and 300, respectively.

Aside: The column headers don't scroll horizontally -- is this a bug in terms of scrolling the thead? Or the result of display:block?

Finally, along the lines of using aria to force the semantics, I modified the example to include aria roles -- attachment forthcoming:
* table: role="grid"
* thead: role="rowgroup"
* tbody: role="rowgroup"
* tr: role="row"
* th: role="columnheader"
* td: role="gridcell"

FF exposes table semantics in the a11y API with the addition of the above roles.  This could be used as a stop-gap measure, although it feels redundant to me.

Note that the closest supported aria role for <table> and <td> involve grid, which technically implies this is an interactive grid.  Is it James?

Comment 24

[Joseph Scheuhammer (:clown)]

Attachment: grid_acc_scrolling.html

Attachment referenced from comment 23

Comment 25

[James Nurthen]

(In reply to Joseph Scheuhammer from comment #23)

> 
> Aside: The column headers don't scroll horizontally -- is this a bug in
> terms of scrolling the thead? Or the result of display:block?

This is a simplification in this sample. I was trying to pare it down and removed the scripting that takes care of making sure the headers get scrolled with the body.

> 
> Finally, along the lines of using aria to force the semantics, I modified
> the example to include aria roles -- attachment forthcoming:
> * table: role="grid"
> * thead: role="rowgroup"
> * tbody: role="rowgroup"
> * tr: role="row"
> * th: role="columnheader"
> * td: role="gridcell"
> 
> FF exposes table semantics in the a11y API with the addition of the above
> roles.  This could be used as a stop-gap measure, although it feels
> redundant to me.
> 
> Note that the closest supported aria role for <table> and <td> involve grid,
> which technically implies this is an interactive grid.  Is it James?

No - this is not an interactive grid - we have a different component for that. This is a scrollable table.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Joseph Scheuhammer from comment #23)

> I'm not sure where a data table with CSS display:block falls between these
> two extremes.  Looking at James' first example (table_acc_scrolling.html)
> there are clearly column headers, rows, and a relationship between them and
> the cells.  For example, the department with ID 30 has the Name
> "Purchasing", and location and manager IDs of 200 and 300, respectively.

I guess anonymous table is created there so if we had an accessible for it then it would resolve a problem for the user but probably it would not resolve James problem as ordinal display:block table element would have not a table accessible.

Comment 28

[Marco Zehe (:MarcoZ)]

Alex, it seems this bug is regaining some traction due to the new CSS grid and flex layout stuff that also breaks table accessibles when applied to table elements. It is becoming a more pressing matter.

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #28)
> Alex, it seems this bug is regaining some traction due to the new CSS grid
> and flex layout stuff that also breaks table accessibles when applied to
> table elements. It is becoming a more pressing matter.

Can you please confirm that bug summary is "expose accessible table for HTML:table having no display:table"? Or is it something else?

Comment 30

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #29)
> (In reply to Marco Zehe (:MarcoZ) from comment #28)
> > Alex, it seems this bug is regaining some traction due to the new CSS grid
> > and flex layout stuff that also breaks table accessibles when applied to
> > table elements. It is becoming a more pressing matter.
> 
> Can you please confirm that bug summary is "expose accessible table for
> HTML:table having no display:table"? Or is it something else?

Basically yes. We want table semantics if they're in the DOM, even when, through CSS, layout might think differently. Jen Simmons has a few more current examples of that, and we should guard against all these. Jen, could you point us to some good short and sweet examples? Thanks!

Comment 31

[alexander :surkov (:asurkov)]

Ok, I think adding entrees for table, td, tr elements into MarkupMap.h [1], and using ARIAGridAccessible and other class for them, should work. I'll put myself into mentors in case if somebody will want to pick up this bug.

https://searchfox.org/mozilla-central/source/accessible/base/MarkupMap.h

Comment 32

[Marco Zehe (:MarcoZ)]

Alex, since nobody seems eager to pick up this bug, can you guide me how to fix this thing once and for all? I just received two more examples of responsive tables, that, if the screen is big enough, are proper tables, but if the screen is made smaller, they just lose their semantics even though that's not intended. These tables are tables for screen readers no matter what.
See https://codepen.io/jensgro/full/dPPEqy
and https://codepen.io/jensgro/full/QbxwKv

And I've had it!

Comment 33

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #32)
> Alex, since nobody seems eager to pick up this bug, can you guide me how to
> fix this thing once and for all?

my pleasure. Basically the idea is outlined in comment #31. You'd need to add maps for table and tr elements like:

MARKUPMAP(
  table,
  [](nsIContent* aContent, Accessible* aContext) -> Accessible* {
    return new ARIAGridAccessible(aContent, aContext->Document());
  }
)

Note, it shouldn't break normal tables, because we check frame type before the markup table, when creating an accessible.

Also you'd need to adjust td and th elements, so they will use proper ARIAGrid classes. You could query mContent->GetPrimaryFrame() to nsTableCellFrame* for this like

nsTableCellFrame* cellFrame = do_QueryFrame(mContent->GetPrimaryFrame());

and if it fails, then use ARIAGrid classes.

Comment 34

[Marco Zehe (:MarcoZ)]

OK, after playing with this for a few days, trying both Alex's proposed fixes, as well as re-using some of the already existing NEW_HTMLTableCellAccessible and friends that were introduced for the Math elements, I am getting no improvements. I must be greatly overlooking something which I cannot figure out. All the examples from this bug as well as others I was sent don't show any improvement no matter what I tried.

I am giving up on this for now, also not attaching any WIPs since this would be pointless, they just don't work. Alex, jamie, feel free to pick this up, I am happy to test and possibly review.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) [On PTO until May 4th] from comment #34)

> I am giving up on this for now, also not attaching any WIPs since this would
> be pointless, they just don't work.

Marco, it sill makes sense to have WIPs attached, I might be able to say how to fix it.

Comment 36

[alexander :surkov (:asurkov)]

Attachment: patch

Community pressure.. :) Marco, please be extra careful about the tests, in case if something is not covered or wrong. Note, HTML:th and scoped HTML:td proper role computation is not supported by this patch, I will followup on this.

Comment 37

[Marco Zehe (:MarcoZ)]

Hi Alex, I think examples like this aren't covered in the tests yet. The smaller the screen, the less table-ish the layout is from a visual standpoint. Like display: block; and the float thing. Look at these two examples:

https://codepen.io/jensgro/pen/dPPEqy

and

https://codepen.io/jensgro/pen/QbxwKv

Note for the second one: Jens has aria-fied the table (with some errors still) to fix the problems that these were previously not rendered as table cells at all. For our testcase, we should obviously make sure to have a non-aria-fied version.

He took these from real-world examples.

Comment 38

[Marco Zehe (:MarcoZ)]

Comment on attachment 8972905 [details] [diff] [review]
patch

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

r=me with nits fixed/some further tests added.

::: accessible/base/MarkupMap.h
@@ +344,5 @@
> +MARKUPMAP(
> +  td,
> +  [](Element* aElement, Accessible* aContext) -> Accessible* {
> +     if (aContext->IsTableRow() && aContext->GetContent() == aElement->GetParent()) {
> +       // If HTML:td elements is part of its HTML:table, which has CSS

Nit: "element is", not "elements is".

::: accessible/tests/mochitest/tree/test_brokencontext.html
@@ -68,5 @@
> -
> -    // HTML table display:block inside table.
> -    checkIfNotAccessible("block_table");
> -    checkIfNotAccessible("tr_in_block_table");
> -    checkIfTDGeneric("td_in_block_table");

Nice to see all of these go away. :)

::: accessible/tests/mochitest/tree/test_table.html
@@ +215,5 @@
> +              ] },
> +            ] }
> +          ] }
> +        ] };
> +      testAccessibleTree("table_containing_inlinetable", accTree);

As mentioned in comment #37, I would like to see such tables tested with crazy cell formatting as well, to make sure we don't trip over these.

Comment 39

[alexander :surkov (:asurkov)]

Attachment: patch2

updated version

Comment 40

[alexander :surkov (:asurkov)]

Attachment: patch3

forgot to add testcases

Comment 41

[Marco Zehe (:MarcoZ)]

Comment on attachment 8973803 [details] [diff] [review]
patch3

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

r=me, thanks!

The icing on the cake would be if we could test the different media queried widths and make sure these really don't have an impact on the accessibility tree, but the tables are crazy enough as they are. And I guess these different widths would create variables of other sorts that would not be in the scope for this bug. So please check in as is. :)

Comment 42

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #41)

> The icing on the cake would be if we could test the different media queried
> widths and make sure these really don't have an impact on the accessibility
> tree,

agreed, I'll file a bug

Comment 43

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #42)
> (In reply to Marco Zehe (:MarcoZ) from comment #41)
> 
> > The icing on the cake would be if we could test the different media queried
> > widths and make sure these really don't have an impact on the accessibility
> > tree,
> 
> agreed, I'll file a bug

I will remove those media styles as they indeed may affect on the tree and thus lead to failures when running on different platforms, so a separate bug for those.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1abaa57e467e
HTML table with display:block should expose table semantics, r=marcoz

Comment 45

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/1abaa57e467e

Comment 46

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #43)
> (In reply to alexander :surkov from comment #42)
> > (In reply to Marco Zehe (:MarcoZ) from comment #41)
> > 
> > > The icing on the cake would be if we could test the different media queried
> > > widths and make sure these really don't have an impact on the accessibility
> > > tree,
> > 
> > agreed, I'll file a bug
> 
> I will remove those media styles as they indeed may affect on the tree and
> thus lead to failures when running on different platforms, so a separate bug
> for those.

Marco, as far as I can tell, there's no difference between small-size-screen of [1] and [2] examples. Could you double check please if we indeed need separate testcases for @media styles cases?

[1] https://codepen.io/jensgro/pen/dPPEqy
[2] https://codepen.io/jensgro/pen/QbxwKv

Comment 47

[Marco Zehe (:MarcoZ)]

Yes, I think so, since if you resize the window in example 1 to be smaller, the table gets rerendered in a way that NVDA no longer sees it as a table. This prevents the use of its ctrl+alt+ArrowKey table commands to navigate to next cell, previous cell, cell above or cell below. Only if the table is wide enough do they start working.

Comment 48

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #47)
> Yes, I think so, since if you resize the window in example 1 to be smaller,
> the table gets rerendered in a way that NVDA no longer sees it as a table.
> This prevents the use of its ctrl+alt+ArrowKey table commands to navigate to
> next cell, previous cell, cell above or cell below. Only if the table is
> wide enough do they start working.

I'm pretty sure I saw a table accessible when @media is applied. Would you mind to file a bug then please to outline what else is left?

Comment 49

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #48)
> (In reply to Marco Zehe (:MarcoZ) from comment #47)
> > Yes, I think so, since if you resize the window in example 1 to be smaller,
> > the table gets rerendered in a way that NVDA no longer sees it as a table.
> > This prevents the use of its ctrl+alt+ArrowKey table commands to navigate to
> > next cell, previous cell, cell above or cell below. Only if the table is
> > wide enough do they start working.
> 
> I'm pretty sure I saw a table accessible when @media is applied. Would you
> mind to file a bug then please to outline what else is left?

I filed bug 1460927

Comment 50

[James Teh [:Jamie]]

Just to make the example easier to find: https://codepen.io/jensgro/pen/dPPEqy

Confirmed. thead/tfoot/tbody are involved, so I suspect this might be at least impacted (if not fixed) by bug 1461244.

Comment 51

[James Teh [:Jamie]]

(In reply to James Teh [:Jamie] from comment #50)
> Confirmed. thead/tfoot/tbody are involved, so I suspect this might be at
> least impacted (if not fixed) by bug 1461244.

Posted this on the wrong bug. A manifestation of extreme sleep deprivation yesterday. :(