Closed Bug 1461244 Opened 8 years ago Closed 7 years ago

Tables with thead/tbody/tfoot and CSS display properties are broken

Categories

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

Product:
Core
Component:
Disability Access APIs
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- wontfix
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: Jamie, Assigned: MarcoZ)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1461244 - Take into account row groups when creating ARIAGridRowAccessibles, r=Jamie
47 bytes, text/x-phabricator-request
lizzard
: approval-mozilla-beta+
Details | Review
Spotted this use of tbody in the headers of messages in a conversation in the new Gmail interface. STR: 1. Open this test case: data:text/html,<table style="display: block;"><tbody style="display: block;"><tr><td>1</td></tr></tbody></table> 2. Query the table for the number of rows and columns; e.g. IAccessibleTable2::nRows, IAccessibleTable2::nColumns. Expected: 1 row, 1 column. Actual: 0 rows, 0 columns. Educated guesswork, but I think: 1. thead, tbody and tfoot need to be mapped to role rowgroup when they have a non-standard display style. 2. The MarkupMap code which creates ARIARowAccessible for tr needs to account for this when looking to see whether it's in a table.
I now see tables with 0 rows and 0 columns in all comment bodies on GitHub. The 0 rows/0 cols is caused by this bug.
Taking a guess at P3 - please retriage as necessary.
P2 because this is a regression which is causing annoyance in the wild (new gmail, GitHub). We might be able to reduce the priority if the layout table fixes in bug 1460244 cause these tables to disappear.
Keywords: regression
Priority: -- → P2
Depends on: 1460244
Blocks: 1005271
No longer blocks: 1005271
Blocks: 1460927
(In reply to James Teh [:Jamie] from comment #3) > P2 because this is a regression which is causing annoyance in the wild (new > gmail, GitHub). We might be able to reduce the priority if the layout table > fixes in bug 1460244 cause these tables to disappear. They did indeed get marked as layout (and are thus no longer rendered by screen readers). Dropping to p3.
Priority: P2 → P3

Taking this.

Assignee: nobody → mzehe
Status: NEW → ASSIGNED

If all parts of a table are non-standard display types, like all elements being display:block;, we weren't properly determining table cell indices because we weren't always taking into account thead, tbody, or tfoot elements. This patch:

  • Exposes non-standard tbody, tfoot and thead elements as groupings, similar to ARIA rowgroup.
  • Adjusts the one instance in nsAccessibilityService::CreateAccessible that didn't account for the table not being the direct parent of the row node, but the grandparent instead.
Blocks: 1523905
Pushed by mzehe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/965b28c159d6 Take into account row groups when creating ARIAGridRowAccessibles, r=Jamie

https://hg.mozilla.org/mozilla-central/rev/965b28c159d6

Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Is this something which should be considered for Beta uplift or can it ride the trains?

status-firefox65: --- → wontfix
status-firefox66: --- → affected
status-firefox-esr60: --- → unaffected
Flags: needinfo?(mzehe)
Flags: in-testsuite+

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Is this something which should be considered for Beta uplift or can it ride the trains?

Hm, I am torn. This is an older regression, from bug 1005271, which landed in 62, but we only now got notified of the Gmail crashes documented in bug 1523905, and only on Linux. The fix has test coverage and has been baking on Nightly a bit, and also got verified to fix the crash by the reporter of bug 1523905.

So in theory it's quite safe to uplift to 66, to give our Linux screen reader and Firefox users the fix for a crash in Gmail one version earlier, but it is strictly not a regression introduced in 66.

Flags: needinfo?(mzehe)

Let's try the uplift to beta. Marco, can you request it?

Flags: needinfo?(mzehe)

Comment on attachment 9040671 [details]
Bug 1461244 - Take into account row groups when creating ARIAGridRowAccessibles, r=Jamie

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

bug 1005271

User impact if declined

Frequent crashes with new Gmail design on Linux when reading conversations with Orca screen reader.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

It makes sure strangely formed table grid constructs get their HTML thead, tbody, and tfoot parts exposed correctly so there can be proper row and column counting. It is limited to constructs that don't use normal table semantics, but are repurposed via CSS display properties.

String changes made/needed

None.

Flags: needinfo?(mzehe)
Attachment #9040671 - Flags: approval-mozilla-beta?

Comment on attachment 9040671 [details]
Bug 1461244 - Take into account row groups when creating ARIAGridRowAccessibles, r=Jamie

Fix for a crash with GMail and screen readers. Has tests, verified in nightly.
OK to uplift for beta 8.

Attachment #9040671 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Whiteboard: [qa-triaged]

We've unsuccessfully tried to reproduce this issue on Firefox 61.0b5 build, therefore we cannot confirm the fix.
Dropping the qe-verify+ flag based on this fact and due to the bug's automated coverage.

Flags: qe-verify+
QA Whiteboard: [qa-triaged]
Whiteboard: [qa-triaged]
Depends on: 1568728
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: