Closed Bug 1780239 Opened 1 year ago Closed 1 year ago

Colorways landmark bisects main page content - expect colorways to be focused on / read by screenreader last

Categories

(Firefox :: Firefox View, defect, P3)

Desktop
All
defect
Points:
3

Tracking

()

RESOLVED FIXED
107 Branch
Accessibility Severity s3
Tracking Status
firefox104 --- disabled
firefox107 --- fixed

People

(Reporter: jberman, Assigned: bigiri)

References

(Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: [fidefe-2022-mr1-firefox-view])

Attachments

(2 files)

Actual: Colorways content is focused on/read after tab pickup and before Recently closed.
Expected: Colorways content should be focused on/read after Tab pickup and Recently closed.

Summary: Firefox View → Colorways landmark bisects main page content - expect colorways to be focused on / read by screenreader last

Note in case anyone is thinking about picking this up: I raised this in #accessibility on slack to work out if there's a solution other than reordering the DOM (which would require some JS and, IMHO, be a little ugly compared to the elegance of the CSS grid + media queries we're using now). Worth checking if there's any better suggestions there.

Component: General → Firefox View

access-s3 for inconsiderate tab order.

Whiteboard: [fidefe-2022-mr1-firefox-view] → [fidefe-2022-mr1-firefox-view][access-s3]

The severity field is not set for this bug.
:sfoster, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(sfoster)
Severity: -- → S3
Flags: needinfo?(sfoster)
Priority: -- → P3
Assignee: nobody → bigiri
Status: NEW → ASSIGNED

FWIW I made a suggestion how to fix this in the dupe:

(In reply to :Gijs (he/him) from bug 1785242 comment #4)

It's ugly but I think the way to fix this would be to use media query observers that then actually reorder the DOM. Would need some testing + code auditing to ensure event listeners etc. stay intact and the custom components don't freak out too much if they get unattached and reattached to the DOM (in theory, it should all work already. In practice...).

Moved the Colorway Landmark section to the end of the FxView HTML so that it will appear last in the screen reader and the tab ordering as expected by our users.

Attachment #9294515 - Attachment description: Bug 1780239 - Fix screen reader order of Colorway Landmark inn FxView r=jamie! → Bug 1780239 - Fix screen reader order of Colorway Landmark inn FxView r=gijs!,jamie!
Attachment #9294515 - Attachment description: Bug 1780239 - Fix screen reader order of Colorway Landmark inn FxView r=gijs!,jamie! → Bug 1780239 - Fix screen reader order of Colorway Landmark in FxView r=gijs!,jamie!
Points: --- → 3

Bernard, is this ready to land? The last update was a while back and it's not clear to me what, if anything, this is waiting for.

Flags: needinfo?(bigiri)

(In reply to :Gijs (back Fri Oct 7; he/him) from comment #9)

Bernard, is this ready to land? The last update was a while back and it's not clear to me what, if anything, this is waiting for.

Your last comment on the patch was:

I don't know. If I had to guess, maybe push health doesn't consider test-verify jobs? @sclements might know. Either way, the fact is that the test failed... Your push doesn't contain "normal" mochitest browser runs so we don't know if those would also have failed. Still, the point of TV jobs is to flag up if tests are flakey, so it'd be worth at least having a look into why the TV job failed.

Since then I've rebased it and reran treeherder (https://treeherder.mozilla.org/jobs?repo=try&revision=2f841ed0241a162433edb0d3060973ac2f2a3ba6). I have no idea what to do about the TV tests, it looks good to me. Since you've requested a comment from Sarah, I've added her as a blocking reviewer. If that isn't necessary then I can land it as is.

Flags: needinfo?(sclements)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bigiri)

Sorry, I had missed Gijs' comment about Push Health but I don't have much to add. I don't know if anything has changed with Push Health in the last 8 months but its an imperfect system that is taking a best guess at what are actual regressions and what aren't. I don't think it has complete data since it relies on errorsummary.json artifacts, which not all jobs provide. (Last I heard :marco and maybe :ahal were looking into using a machine learning tool (mozci, I think) to improve Push Health but no idea the status of that.)

Flags: needinfo?(sclements)

(In reply to Bernard Igiri from comment #10)

(In reply to :Gijs (back Fri Oct 7; he/him) from comment #9)

Bernard, is this ready to land? The last update was a while back and it's not clear to me what, if anything, this is waiting for.

Your last comment on the patch was:

I don't know. If I had to guess, maybe push health doesn't consider test-verify jobs? @sclements might know. Either way, the fact is that the test failed... Your push doesn't contain "normal" mochitest browser runs so we don't know if those would also have failed. Still, the point of TV jobs is to flag up if tests are flakey, so it'd be worth at least having a look into why the TV job failed.

Since then I've rebased it and reran treeherder (https://treeherder.mozilla.org/jobs?repo=try&revision=2f841ed0241a162433edb0d3060973ac2f2a3ba6). I have no idea what to do about the TV tests, it looks good to me.

The new test you added is still failing on this new trypush. This trypush too lacks "normal" browser mochitests so it's unclear if those are also failing or not, but looking at the logs for the failing TV jobs, it seems the very first run of the test fails immediately. So presumably the test is still broken and needs fixing. You can run the test in TV mode locally to see if that reproduces the problem for you. Considering all the failing tests on infra are running against opt builds, if you can't reproduce the test failing locally it would also be worth checking if the test maybe passes for you locally on debug but fails on opt builds, or something. Either way, if the test is failing reliably on infra you can't land the patch as-is and will need to figure out why it's failing...

Since you've requested a comment from Sarah, I've added her as a blocking reviewer. If that isn't necessary then I can land it as is.

I thought Sarah could comment on the push health thing (which she has done, thank you Sarah!) but not necessarily on the patch itself. It sounds like it'd be worth filing an orthogonal bug report about TV jobs not producing errorsummary.json files and/or for some other reason not being included in push health.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bigiri)
Attachment #9294515 - Attachment description: Bug 1780239 - Fix screen reader order of Colorway Landmark in FxView r=gijs!,jamie! → Bug 1780239 - Fix screen reader order of Colorway Landmark in FxView r=gijs!

The tests should pass now. The treeherder link is on Phabricator.

Flags: needinfo?(bigiri)
Pushed by bigiri@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fff0a3464fae
Fix screen reader order of Colorway Landmark in FxView r=Gijs
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch
Regressions: 1805908
Accessibility Severity: --- → s3
Whiteboard: [fidefe-2022-mr1-firefox-view][access-s3] → [fidefe-2022-mr1-firefox-view]
You need to log in before you can comment on or make changes to this bug.