Closed Bug 1135805 Opened 9 years ago Closed 9 years ago

[Accessibility] App chrome urlbar title is visible to the screen reader user even though it is hidden off screen.

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S7 (6mar)
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

Details

(Keywords: access, Whiteboard: [b2ga11y p=1][b2ga11y stability])

Attachments

(1 file, 2 obsolete files)

Screen reader user should not be able to swipe to the titlebar in full screen apps for example.
Steps to reproduce with screen reader:
* Open a Gallery app
* Swipe all the way right until the screen reader reaches the end.
* Expected: screen reader should stop on the last visible item on the screen.
* Observed: screen reader steps into the invisible "Search the web" item.
Attachment #8568581 - Flags: review?(alive)
Whiteboard: [b2ga11y p=1] → [b2ga11y p=1][b2ga11y stabilitiy]
Whiteboard: [b2ga11y p=1][b2ga11y stabilitiy] → [b2ga11y p=1][b2ga11y stability]
Comment on attachment 8568581 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

Looks good to me, but I am delivering this review to sysFE team since they are taking care app chrome now.
Attachment #8568581 - Flags: review?(alive) → review?(mhenretty)
Comment on attachment 8568581 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

Change looks good, things seem more consistent now that we add/remove the visible class both when showing and hiding. I tested the patch, and couldn't find anything broken with the date in the utility. Glad to see tests for this too, so we don't break a11y so easily.
Attachment #8568581 - Flags: review?(mhenretty) → review+
Comment on attachment 8568581 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

Whoops, that was for the wrong bug. I'm reviewing this now.
Attachment #8568581 - Flags: review+ → review?(mhenretty)
Comment on attachment 8568581 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

Code looks reasonable, and it seems to work well too. However, I don't feel entirely comfortable reviewing app_chrome just yet. Forwarding to Kevin for super review.
Attachment #8568581 - Flags: review?(mhenretty)
Attachment #8568581 - Flags: review?(kgrandon)
Attachment #8568581 - Flags: feedback+
Comment on attachment 8568581 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

Looks good to me. Thanks!
Attachment #8568581 - Flags: review?(kgrandon) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 8568581 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

[Approval Request Comment] This pull request fixes screen reader visibility issues around chrome bar.
[Bug caused by] (feature/regressing bug #): improvement, not a bug
[User impact] if declined: if declined, screen reader users will be able to land on content hidden otherwise (in chrome bar)
[Testing completed]: unit tests + on device
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none
Attachment #8568581 - Flags: approval-gaia-v2.2?
Comment on attachment 8568581 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

Clearing for now for causing bug 1137074?
Attachment #8568581 - Flags: approval-gaia-v2.2?
Backed out for causing bug 1137074: https://github.com/mozilla-b2g/gaia/commit/9c0dcfb27be96e09453a447dd2714e7b1d991e7a

Let's fix and re-land so we don't have to uplift multiple things.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yura - please create a new patch for this bug + the lockscreen fix and flag me for review. Thanks!
Flags: needinfo?(yzenevich)
No longer blocks: 1137074
Attachment #8568581 - Attachment is obsolete: true
Comment on attachment 8569640 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

Merged the fix into the original PR.
Flags: needinfo?(yzenevich)
Attachment #8569640 - Flags: review?(kgrandon)
Comment on attachment 8569640 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

This seems fine to me again. Thanks!
Attachment #8569640 - Flags: review?(kgrandon) → review+
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Comment on attachment 8569640 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

[Approval Request Comment] This pull request fixes screen reader visibility issues around chrome bar.
[Bug caused by] (feature/regressing bug #): improvement, not a bug
[User impact] if declined: if declined, screen reader users will be able to land on content hidden otherwise (in chrome bar)
[Testing completed]: unit tests + on device
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none
Attachment #8569640 - Flags: approval-gaia-v2.2?
Attachment #8569640 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Target Milestone: 2.2 S7 (6mar) → ---
Depends on: 1138688
If we don't write tests here, let's make sure we file some follow-ups to write tests for *everything* that this has broken that we didn't catch in CI.
Depends on: 1137074
Attachment #8569640 - Flags: approval-gaia-v2.2+
Attachment #8569640 - Attachment is obsolete: true
Comment on attachment 8571981 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

Ensuring that the urlbar visibility is fully consistent with the rules described for its container (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/chrome/chrome.css#L63-L70)
Attachment #8571981 - Flags: review?(kgrandon)
See Also: → 1138965
Comment on attachment 8571981 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

The selectors are starting to get a little too crazy/duplicated for me. I'd like to see if we can explore a more clean way of doing things.

Yura - I left a comment on github. Would it be possible to only have an override for the fullscreen-app case?

Also why is it bad that the screenreader hits the rocketbar when it's offscreen?
Flags: needinfo?(yzenevich)
Attachment #8571981 - Flags: review?(kgrandon)
(In reply to Kevin Grandon :kgrandon from comment #22)
> Comment on attachment 8571981 [details] [review]
> [gaia] yzen:bug-1135805 > mozilla-b2g:master
> 
> The selectors are starting to get a little too crazy/duplicated for me. I'd
> like to see if we can explore a more clean way of doing things.
> 
> Yura - I left a comment on github. Would it be possible to only have an
> override for the fullscreen-app case?
> 
> Also why is it bad that the screenreader hits the rocketbar when it's
> offscreen?

The general rule is that screen reader must only have access to elements that are currently present on screen: to avoid unexpected interactions, etc. Also screen readers are not necessarily used by completely blind people meaning that it will be highly confusing to reach things that they can't see.
Flags: needinfo?(yzenevich)
Comment on attachment 8571981 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

Styling just the fullscreen-app chrome did that trick, thanks for the suggestion!
Attachment #8571981 - Flags: review?(kgrandon)
Comment on attachment 8571981 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

I think this looks fine to me, thanks! 

It appears that there was some kind of rebase error here. It looks like the author is currently labeled as "Autolander" instead of yourself. It's probably ok to land as-is, but you might want to take a minute to fix it.
Attachment #8571981 - Flags: review?(kgrandon) → review+
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Comment on attachment 8571981 [details] [review]
[gaia] yzen:bug-1135805 > mozilla-b2g:master

Re-requesting once more, hopefully for good:
[Approval Request Comment] This pull request fixes screen reader visibility issues around chrome bar.
[Bug caused by] (feature/regressing bug #): improvement, not a bug
[User impact] if declined: if declined, screen reader users will be able to land on content hidden otherwise (in chrome bar)
[Testing completed]: unit tests + on device
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none
Attachment #8571981 - Flags: approval-gaia-v2.2?
Attachment #8571981 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: