Lockscreen is completely inaccessible to screen reader

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: eeejay, Assigned: alive)

Tracking

({access})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b2ga11y 1.4])

Attachments

(2 attachments)

STR:
1. Enable screen reader.
2. In lockscreen, notice that putting finger over the clock will highlight and speak it.
3. Put finger over unlock or camera icon.

Result: Nothing happens.
Expected: The icons should highlight and be spoken.

This is due to lockscreen-area being higher in the z-index and swallowing pointer events. I have no simple fix for this since the slider code is pretty complex..
Component: Gaia → Gaia::System::Lockscreen
It looks like at this point all of lockscreen is screen reader inaccessible.
Definitely confirmed. I can no longer unlock my phone in current nightly builds.
Summary: Explore by touch does not work on unlock icons → Lockscreen is completely inaccessible to screen reader
Whiteboard: [b2ga11y 1.4]
Looks like aria-hidden='true' is added to #windows selector by the new windows manager when lockscreen is present and is locked:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window_manager.js#L204-L206
Assignee: nobody → yzenevich
Here is a proposed fix. The new window management is great and lets us remove the nested aria-hidden flags which were trouble.

Need to followup with some tests.
Attachment #8406474 - Flags: review?(alive)
Assignee: yzenevich → eitan
Ran the tests using Travis and they seem to pass.. Is it possible the built or the profile is somewhat different?
Comment on attachment 8406474 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18304

Hi, nice catch!
I have a proposal from architecture side concern on github. If you have any question feel free to discuss with me.

Thank you very much to keep screen reader work with window management!
Attachment #8406474 - Flags: review?(alive)
c.c. Gred as well since this involves lockscreenWindow change.
Component: Gaia::System::Lockscreen → Gaia::System::Window Mgmt
Depends on: lockscreen-window
Comment on attachment 8406474 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18304

Alive,

I tried to follow your architectural suggestions, but reached a dead end. Either I didn't understand what you meant, or it is not possible.

I made a new version that overrides setVisible to LockScreenWindow.
Attachment #8406474 - Flags: review?(gweng)
Depends on: 998121
Comment on attachment 8406474 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18304

According to Eitan's last comment, I think Alive should be consulted (and to do so via the new flag).
Attachment #8406474 - Flags: review?(alive)
Comment on attachment 8406474 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18304

I changed something based on your patch.
Let me know if this works for you.

https://github.com/alivedise/gaia/commit/1a24bc42059907a8fa39c9a6f5e9f38aee6eaa44

If so please take it.
Attachment #8406474 - Flags: review?(gweng)
Attachment #8406474 - Flags: review?(alive)
(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment #10)
> Comment on attachment 8406474 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18304
> 
> I changed something based on your patch.
> Let me know if this works for you.
> 
> https://github.com/alivedise/gaia/commit/
> 1a24bc42059907a8fa39c9a6f5e9f38aee6eaa44
> 
> If so please take it.

This patch does not resolve all the visibility issues. At any given time, there should only be one child of #windows with aria-hidden=false. When Gaia first starts, you will notice that both the lockscreen and homescreen have aria-hidden=false. That is a bug my patch fixes, and this one does not.

(they also both have a class of |active|, which is probably a bug too that none of these patches resolve).
Eitan, that sounds a appWindow bug, I will look into it deeply today or tomorrow. Thanks.
I found there's two audiochannelchange event 1. none 2. normal but never goes back to none.
So VisibilityManager thinks there's music playing right now, that's why current app is not sent to background when screen is locked.

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/visibility_manager.js#L70

I am going to file another gecko bug for this and workaround in gaia side first.
Taken.
:eeejay, I hope you don't mind I am taking this bug since I made some big change to give a proper fix. I will feedback you when I finished the patch.
Assignee: eitan → alive
1. Fix the normal audio channel by workaround
2. Lockscreen should be only unlock when active app is ready.
3. VisibilityManager should re-display lockscreen if attention screen is closed.
4. Unit tests.
Attachment #8410172 - Flags: review?(timdream)
Attachment #8410172 - Flags: feedback?(eitan)
Comment on attachment 8410172 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18518

Thanks! This looks good, but it is not complete.

You will notice that in the initial state after starting gaia both children of #windows have aria-hidden=false.

The lockscreen is created (and locked) before the homescreen. So when the homescreen setVisible(true) is called it displays both windows.

That is why I had a check in the setVisible function to not do aria-hidden=false if the lockscreen is already engaged.
Attachment #8410172 - Flags: feedback?(eitan)
(In reply to Eitan Isaacson [:eeejay] from comment #16)
> Comment on attachment 8410172 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/18518
> 
> Thanks! This looks good, but it is not complete.
> 
> You will notice that in the initial state after starting gaia both children
> of #windows have aria-hidden=false.
> 
> The lockscreen is created (and locked) before the homescreen. So when the
> homescreen setVisible(true) is called it displays both windows.

This sounds a race condition.
setVisible(true) should be called while lockscreen window is active. I will fix it.

> 
> That is why I had a check in the setVisible function to not do
> aria-hidden=false if the lockscreen is already engaged.
Attachment #8410172 - Flags: review?(timdream)
It's because homescreenWindow is launched after ftuskip is dispatched but at the same time lockscreenWindow is already launched.
Proposed fix:
(1) Launch homescreenWindow after ftuskip event and then lockscreenWindow.
(2) In ftuskip event handler in AppWindowManager, check Lockscreen.locked before calling display.

Method 1 may cause some timing change so I prefer to have (2) and fix all of launching sequence issues in system app v2 stage 2.

The reason that I don't want to add lockscreen.locked check in appWindow is
(A) I'd like to keep appWindow as clean as possible to be re-used.
(B) lockscreen.locked will be replaced by LockscreenWindow.isActive() in the future.
Comment on attachment 8410172 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18518

Fixed the homescreen is foreground after device being booted.
Attachment #8410172 - Flags: review?(timdream)
Attachment #8410172 - Flags: feedback?(eitan)
Comment on attachment 8410172 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18518

Tested, and works great!
Attachment #8410172 - Flags: feedback?(eitan) → feedback+
Interestingly enough we've been investigating something similar for the last few days as well. In our case we noticed that apps were being launched before the homescreen via the runapp CLI command. We have a WIP fix in bug 999574, and this patch likely collides with it a bit.
See Also: → 999574
(In reply to Kevin Grandon :kgrandon from comment #21)
> Interestingly enough we've been investigating something similar for the last
> few days as well. In our case we noticed that apps were being launched
> before the homescreen via the runapp CLI command. We have a WIP fix in bug
> 999574, and this patch likely collides with it a bit.

I think we could co-exist.
Homescreen is launched via FTUskip/FTUopen/FTUdone -> AppWindowManager -> HomescreenLauncher.
But your app is launched via mozApps launch -> AppWindowFactory -> AppWindowManager.

If you have conflicts issues let me know.
Comment on attachment 8410172 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18518

Alive, unfortunately you need to rebase this patch as I didn't review it in time.

I will make sure this gets done this week. Sorry about that.
Attachment #8410172 - Flags: review?(timdream)
Attachment #8410172 - Flags: review?(timdream)
Blocks: 1002751
Comment on attachment 8410172 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18518

There are many changes seemly unrelated to this bug (I could be wrong).

Preferably we can better reduce the risk if changes are being landed in different patches. But let's just land and see what happen this time.
Attachment #8410172 - Flags: review?(timdream) → review+
What chance is there for this to be uplifted into 1.4? It is unfortunate we missed the window for that :(
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #24)
> Comment on attachment 8410172 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/18518
> 
> There are many changes seemly unrelated to this bug (I could be wrong).
> 
> Preferably we can better reduce the risk if changes are being landed in
> different patches. But let's just land and see what happen this time.

It's related. If we don't fix the race we will see two window is visible to the screen reader at the same time again.
https://github.com/mozilla-b2g/gaia/commit/ddd3092cd9340a5b89d5bf715dbb0f8c8aad9634
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
(In reply to Eitan Isaacson [:eeejay] from comment #25)
> What chance is there for this to be uplifted into 1.4? It is unfortunate we
> missed the window for that :(

There's no lockscreenWindow in v1.4, why does it need this one?
Duplicate of this bug: 939732
Depends on: 1007510
Depends on: 1024483
You need to log in before you can comment on or make changes to this bug.