Dialogs that come up are not exclusively visible to the screen reader.

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::FMRadio
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: MarcoZ, Assigned: Ross)

Tracking

({access})

unspecified
2.2 S8 (20mar)
ARM
Gonk (Firefox OS)
access
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [b2ga11y p=1])

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
Any dialogs brought up by FMRadio are not exclusively visible, other items that should be hidden from the screen reader, are not.
Assignee: nobody → bugzilla
Created attachment 8564461 [details] [review]
[gaia] FunkTron:Bug1069221 > mozilla-b2g:master
(Assignee)

Comment 2

3 years ago
I've been having trouble with WebIDE not showing me any of the CSS rules while debugging my Flame device ever since an update occurred around 2 weeks ago, so I'm hoping I didn't miss anything, but I think I've resolved the visibility issues.  Looks like Autolander nabbed the PR so give it a look and let me know! :)
Flags: needinfo?(yzenevich)
(In reply to Ross from comment #2)
> I've been having trouble with WebIDE not showing me any of the CSS rules
> while debugging my Flame device ever since an update occurred around 2 weeks
> ago, so I'm hoping I didn't miss anything, but I think I've resolved the
> visibility issues.  Looks like Autolander nabbed the PR so give it a look
> and let me know! :)

Yeah, WebIDE acts out somtimes, see if updating to the latest firefox nightly helps.
Flags: needinfo?(yzenevich)
Hi Ross, I added some comments in the PR. Looks pretty good, see if it makes sense now (instead of updating test HTML in 2 places) to just loadHTML just like we did in you first bug and avoid any further work when updating the tests. Thanks!
(Assignee)

Comment 5

3 years ago
Okay, updated the PR with a new version.  I tried to leave some line notes on the PR in the way that you usually do but it looks like I clicked the wrong thing -- think I was supposed to click "Files Changed" at the bottom but instead clicked the commit link.  Haha, sorry about that, hopefully you can browse over to the commit page and see those points-of-reference relatively easily though.
Flags: needinfo?(yzenevich)
Left some comments, generally all looks good! thanks! mark me for the final a11y-review once you cleaned it all up.
Flags: needinfo?(yzenevich)
(Assignee)

Comment 7

3 years ago
Okay, think we're set.  I ended up just refreshing the body in all suites where new tempNodes had been declared. I think that should keep things safe for future additions to tests, etc. (should there be any).
(Assignee)

Updated

3 years ago
Flags: a11y-review?
(Assignee)

Comment 8

3 years ago
I noticed that when Autolander auto-creates the PR attachment, I'm not given any permissions to set the flags on the attachment, so I ended up just setting the a11y-review flag on the bug itself.  Not sure who that goes to however, so using NI here as well.
Flags: needinfo?(yzenevich)

Updated

3 years ago
Attachment #8564461 - Flags: review?(timdream)
Attachment #8564461 - Flags: a11y-review+
Flags: needinfo?(yzenevich)

Updated

3 years ago
Flags: a11y-review?
Thanks, Ross! Looks really good, forwarding the review to Timothy. I will check tomorrow your bugzilla permissions so you could set the flags without problems.
Meanwhile, Ross, let me know if you would be interested in this bug 1134097? It's something I did not have time to finish up in Gaia but it should be fun!
(Assignee)

Comment 11

3 years ago
Hey Yura, okay, sounds good!  And yeah, I checked out Bug 1134097 and it seems fine to me -- feel free to assign it to me when you get a chance (I don't appear to have appropriate permissions to "Take" bugs either at the moment) and I'll get started on it!
Hi Tim, would you have time to take a look at this one? If not, no worries, I can flag someone else.
Flags: needinfo?(timdream)
(In reply to Ross from comment #8)
> I noticed that when Autolander auto-creates the PR attachment, I'm not given
> any permissions to set the flags on the attachment, so I ended up just
> setting the a11y-review flag on the bug itself.  Not sure who that goes to
> however, so using NI here as well.

Kevin, are you aware of this? Where should we file autolander bugs to?
Flags: needinfo?(kgrandon)
Comment on attachment 8564461 [details] [review]
[gaia] FunkTron:Bug1069221 > mozilla-b2g:master

Thanks for the fix!
Flags: needinfo?(timdream)
Attachment #8564461 - Flags: review?(timdream) → review+
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #13)
> Kevin, are you aware of this? Where should we file autolander bugs to?

I just found out about this =/ Currently working on this in bug 1137368. Moving forward the bug assignee will be able to edit all attachments on the bug. I'll try to get an ETA on that bug.
Flags: needinfo?(kgrandon)
We will probably want to manually land this one for now. Ross - could you squash your commits into a single, and rename the commit to contain the bug number?
Flags: needinfo?(bugzilla)
(Assignee)

Comment 17

3 years ago
(In reply to Kevin Grandon :kgrandon from comment #16)
> We will probably want to manually land this one for now. Ross - could you
> squash your commits into a single, and rename the commit to contain the bug
> number?

Hey Kevin, I went ahead and squashed the commits and included the Bug# in the commit message.  Think you should be set to land, but let me know if you need anything else!
Flags: needinfo?(bugzilla)
(Assignee)

Comment 18

3 years ago
(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #14)

> Thanks for the fix!

Sure thing! :)
Landed in master: https://github.com/mozilla-b2g/gaia/commit/620210e07a4634008a33fe62f817f508258bd2c3

Bug 1137368 should be resolved soon which will make this workflow better in the future. Thank you for the patch.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

3 years ago
(In reply to Kevin Grandon :kgrandon from comment #19) 
> Bug 1137368 should be resolved soon which will make this workflow better in
> the future. Thank you for the patch.

Okay cool, sounds good! And no probs!
Comment on attachment 8564461 [details] [review]
[gaia] FunkTron:Bug1069221 > mozilla-b2g:master

[Approval Request Comment] This PR resolves dialog visibility issues in FM radio.
[Bug caused by] (feature/regressing bug #): improvement, not a bug
[User impact] if declined: if declined, screen reader users will be able to access content invisible otherwise
[Testing completed]: unit tests, on device
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none
Attachment #8564461 - Flags: approval-gaia-v2.2?
Blocks: 1141330
Reverting this for causing regressing bug 1141330. Yura or Ross - could either of you guys take a look at this? Thanks!

https://github.com/mozilla-b2g/gaia/commit/353558f34cc867e0f23b5cbe499c5f01a319d1ca
Status: RESOLVED → REOPENED
Flags: needinfo?(yzenevich)
Flags: needinfo?(bugzilla)
Resolution: FIXED → ---
Attachment #8564461 - Flags: approval-gaia-v2.2?
Ross would you mind taking a look at this one if you have time?
Flags: needinfo?(yzenevich)
Created attachment 8575492 [details] [review]
[gaia] FunkTron:Bug1069221-Fix > mozilla-b2g:master
(Assignee)

Comment 25

3 years ago
Hey Yura,

So it looks like the problem was related to the #container element not being able to render properly as a result of its 'hidden' attribute.  I solved the issue by going with a pre-existing CSS class that sets the 'visibility' style instead.  Works on v3.0 now.
Flags: needinfo?(bugzilla) → needinfo?(yzenevich)
Comment on attachment 8575492 [details] [review]
[gaia] FunkTron:Bug1069221-Fix > mozilla-b2g:master

Looks good and works as expected from a11y point of view when in airplane mode. Ross, mark Kevin for a review when you update the PR with the comment. Thanks!
Flags: needinfo?(yzenevich)
Attachment #8575492 - Flags: a11y-review+
(Assignee)

Updated

3 years ago
Attachment #8575492 - Flags: review?(kgrandon)
Comment on attachment 8575492 [details] [review]
[gaia] FunkTron:Bug1069221-Fix > mozilla-b2g:master

I don't have a headset with me now, so I can't easily test the FM radio app. I'm basically going to just carry the Tim's review from earlier, and trust that you have tested this and it won't regress again. Thank you for the patch!
Attachment #8575492 - Flags: review?(kgrandon) → review+
Fixed the commit name to include the bug number and landed in master: https://github.com/mozilla-b2g/gaia/commit/2bcb2253c3a98a6f5b4c4f6360984fd61bcb9d45
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Created attachment 8575630 [details]
Commit landed in master
Attachment #8575630 - Flags: review+
Comment on attachment 8575492 [details] [review]
[gaia] FunkTron:Bug1069221-Fix > mozilla-b2g:master

Hopefully it sticks in master , so asking for approval once more.
[Approval Request Comment] This PR resolves dialog visibility issues in FM radio.
[Bug caused by] (feature/regressing bug #): improvement, not a bug
[User impact] if declined: if declined, screen reader users will be able to access content invisible otherwise
[Testing completed]: unit tests, on device
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: none
Attachment #8575492 - Flags: approval-gaia-v2.2?

Updated

3 years ago
Attachment #8575492 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/de472129d4b113f2fa6357ec5b746715f53092cf
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Target Milestone: --- → 2.2 S8 (20mar)
You need to log in before you can comment on or make changes to this bug.