Closed Bug 1460244 Opened 2 years ago Closed 2 years ago

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

Categories

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

x86
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 --- fixed

People

(Reporter: MarcoZ, Assigned: surkov)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached file 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.
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?
Priority: -- → P3
(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.
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
(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.
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.
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 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 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
Attachment #8974875 - Flags: review+
(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?
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02854660533c
For tables with CSS display properties, correctly expose table interfaces to a11y clients. r=surkov
Keywords: checkin-needed
(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.
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.
Keywords: regression
let's bump a priority since it regresses real life web.
Priority: P3 → P2
Attached patch patch (obsolete) — Splinter Review
Attachment #8976997 - Flags: review?(mzehe)
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!
Attachment #8976997 - Flags: review?(mzehe) → review+
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
Flags: needinfo?(yzenevich)
Blocks: 1461244
Attached patch patch2Splinter Review
with mathml tables fixed

thanks to Yura for finding out a cause!
Attachment #8976997 - Attachment is obsolete: true
Flags: needinfo?(yzenevich)
Attachment #8980351 - Flags: review?(mzehe)
Attachment #8980351 - Attachment is patch: true
Attachment #8980351 - Attachment mime type: text/x-patch → text/plain
Attachment #8980351 - Flags: review?(mzehe) → review+
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50a6c7986000
Tables with CSS display properties no longer participate in layout table calculation, r=marcoz
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.