1460244 - Tables with CSS display properties no longer participate in layout table calculation

Status: RESOLVED FIXED

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

Description

[Marco Zehe (:MarcoZ)]

Attachment: Test case

After fix for bug 1005271, CSS display properties affect how tables are being perceived by screen readers. The attached example is perceived as a layout table by NVDA without CSS, but not as one if these CSS properties are present. I also have a feeling this second table has no proper nsIAccessibleTable and nsIAccessibleTableCell interfaces.

To reproduce:
1. Open the attached testcase in Nightly with NVDA running.
2. Let NVDA read the page.

Expected: No table should be present, since a table with just 1 row and 2 cells is clearly a layout table.

Actual: The second table is read by NVDA as a table.

3. Notice also that NVDA speaks "cell" for the cells in table 2, which it does not do for normal tables. It also does not give row and column numbers. This may be a separate bug, though, but I am mentioning it for completeness.

Real-world example: The new Gmail design shows tables like this in the conversation view for the from, date, and recipients at the top of a message. Google is using newer display properties for tables to get better results in different screen resolutions.

Comment 1

[alexander :surkov (:asurkov)]

Marco, would you be interested to turn the test case into a mochitest, so it will be easier to see what is a bug.

Also, do I understand correct, that this bug doesn't cover the case of wrong column/rowheader roles for td@scope/th elements and we need a new one?

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #1)
> Marco, would you be interested to turn the test case into a mochitest, so it
> will be easier to see what is a bug.

The bug is that thee second table doesn't get the layout-guess:true object attribute set, probably because we don't view it as a layout table.

> Also, do I understand correct, that this bug doesn't cover the case of wrong
> column/rowheader roles for td@scope/th elements and we need a new one?

Correct.

Comment 3

[Rares Doghi, Desktop QA]

Hello, Here are the results from mozregression :

 Last good revision: f6571a82973ae57027019690d26c53f4cea06d59
 First bad revision: 1abaa57e467e72e566b4f74485ba10322bde6921

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f6571a82973ae57027019690d26c53f4cea06d59&tochange=1abaa57e467e72e566b4f74485ba10322bde6921

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #2)
> (In reply to alexander :surkov from comment #1)
> > Marco, would you be interested to turn the test case into a mochitest, so it
> > will be easier to see what is a bug.
> 
> The bug is that thee second table doesn't get the layout-guess:true object
> attribute set, probably because we don't view it as a layout table.

You also mentioned that there's problems with table interfaces, basically I try to figure out what is a scope of them problem.

> > Also, do I understand correct, that this bug doesn't cover the case of wrong
> > column/rowheader roles for td@scope/th elements and we need a new one?
> 
> Correct.

I filed bug 1460669.

Comment 5

[James Teh [:Jamie]]

I think we have far bigger problems than just layout tables here. I'm not convinced bug 1005271 is working as intended.

In this example:
data:text/html,<table style="display: block;"><tr><td>blah</td></tr></table>
QueryInterface for IAccessibleTable2 on the table gives me E_NOINTERFACE. Same for IAccessibleTableCell on the cell.
The tree and roles are correct, but the table interfaces aren't exposed.

Note that the above example would normally be treated as a layout table. If you change the td to a th:
data:text/html,<table style="display: block;"><tr><th>blah</th></tr></table>
we'd normally treat that as a data table. Same situation there, though.

Comment 6

[James Teh [:Jamie]]

Attachment: For tables with CSS display properties, correctly expose table interfaces to a11y clients.

I'm not sure if this patch really belongs on this bug, since it's not specific to layout tables. It essentially fixes bug 1005271 to work as expected and expose the table interfaces to clients.
This actually doesn't fix the layout guess problem. That code lives in HTMLTableAccessible::IsProbablyLayoutTable, but that obviously doesn't get used in this case because we use ARIAGridAccessible. I think we need to move most of that code into TableAccessible and make it apply to these display property cases without applying to ARIA grids/tables with explicit roles.
Alex, are you able to take this further?

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 8974875 [details] [diff] [review]
For tables with CSS display properties, correctly expose table interfaces to a11y clients.

Sorry, should have caught that we weren't going for the Wrap classes. :(

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 8974875 [details] [diff] [review]
For tables with CSS display properties, correctly expose table interfaces to a11y clients.

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

this is one is ready to land independently, thanks for the catch, r=me

Comment 9

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #6)
> Created attachment 8974875 [details] [diff] [review]

> This actually doesn't fix the layout guess problem.

Is there a such problem in this case? I suppose ARIAGridAccessible does never report layout table.

> That code lives in
> HTMLTableAccessible::IsProbablyLayoutTable, but that obviously doesn't get
> used in this case because we use ARIAGridAccessible. I think we need to move
> most of that code into TableAccessible and make it apply to these display
> property cases without applying to ARIA grids/tables with explicit roles.

Right, ARIAGridAccessible hot fix has downsides. It should be addressed it of course, along with bug 1460669.

> Alex, are you able to take this further?

I could keep it on my plate for sure, but do we know if any real life examples where it hurts?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02854660533c
For tables with CSS display properties, correctly expose table interfaces to a11y clients. r=surkov

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/02854660533c

Comment 12

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #9)
> > This actually doesn't fix the layout guess problem.
> Is there a such problem in this case? I suppose ARIAGridAccessible does
> never report layout table.

Right.

> I could keep it on my plate for sure, but do we know if any real life
> examples where it hurts?

See comment 0 (Marco's original report):

> Real-world example: The new Gmail design shows tables like this in the conversation view for the from, date, and recipients at the top of a message. Google is using newer display properties for tables to get better results in different screen resolutions.

Comment 13

[James Teh [:Jamie]]

Another real world example:
I now see tables in all comment bodies on GitHub. These are definitely layout tables and are incredibly annoying when presented to users.

Comment 14

[alexander :surkov (:asurkov)]

let's bump a priority since it regresses real life web.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 16

[Marco Zehe (:MarcoZ)]

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

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

Yes, that should work. If that one table is recognized as a layout table, it is a good bet one with one or two rows with only 2 columns, like in Gmail, but without headers, will be recognized, too. So that one test should suffice. Thanks!

Comment 17

[alexander :surkov (:asurkov)]

mm, there are some jsat failures [1]. Yura, may I ask you to look at these? Do we break layout-guess for MathML tables somehow?

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=179043578&repo=try&lineNumber=4801

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch2

with mathml tables fixed

thanks to Yura for finding out a cause!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50a6c7986000
Tables with CSS display properties no longer participate in layout table calculation, r=marcoz

Comment 20

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/50a6c7986000