Eitan Isaacson [:eeejay] Reporter
	Description • 10 years ago

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..

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 1 • 10 years ago

It looks like at this point all of lockscreen is screen reader inaccessible.

	Marco Zehe (:MarcoZ)
	Comment 2 • 10 years ago

Definitely confirmed. I can no longer unlock my phone in current nightly builds.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 3 • 10 years ago

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

	Eitan Isaacson [:eeejay] Reporter
	Comment 4 • 10 years ago

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.

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 5 • 10 years ago

Ran the tests using Travis and they seem to pass.. Is it possible the built or the profile is somewhat different?

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 6 • 10 years ago

Comment on attachment 8406474 [details]
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!

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 7 • 10 years ago

c.c. Gred as well since this involves lockscreenWindow change.

	Eitan Isaacson [:eeejay] Reporter
	Comment 8 • 10 years ago

Comment on attachment 8406474 [details]
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.

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 9 • 10 years ago

Comment on attachment 8406474 [details]
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).

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 10 • 10 years ago

Comment on attachment 8406474 [details]
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.

	Eitan Isaacson [:eeejay] Reporter
	Comment 11 • 10 years ago

(In reply to Alive Kuo [:alive][NEEDINFO!][God bless Taiwan.] from comment #10)
> Comment on attachment 8406474 [details]
> 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).

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 12 • 10 years ago

Eitan, that sounds a appWindow bug, I will look into it deeply today or tomorrow. Thanks.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 13 • 10 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 14 • 10 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 15 • 10 years ago

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.

	Eitan Isaacson [:eeejay] Reporter
	Comment 16 • 10 years ago

Comment on attachment 8410172 [details]
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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 17 • 10 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #16)
> Comment on attachment 8410172 [details]
> 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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 18 • 10 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 19 • 10 years ago

Comment on attachment 8410172 [details]
https://github.com/mozilla-b2g/gaia/pull/18518

Fixed the homescreen is foreground after device being booted.

	Eitan Isaacson [:eeejay] Reporter
	Comment 20 • 10 years ago

Comment on attachment 8410172 [details]
https://github.com/mozilla-b2g/gaia/pull/18518

Tested, and works great!

	Kevin Grandon :kgrandon
	Comment 21 • 10 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 22 • 10 years ago

(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.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 23 • 10 years ago

Comment on attachment 8410172 [details]
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.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 24 • 10 years ago

Comment on attachment 8410172 [details]
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.

	Eitan Isaacson [:eeejay] Reporter
	Comment 25 • 10 years ago

What chance is there for this to be uplifted into 1.4? It is unfortunate we missed the window for that :(

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 26 • 10 years ago

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #24)
> Comment on attachment 8410172 [details]
> 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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 27 • 10 years ago

https://github.com/mozilla-b2g/gaia/commit/ddd3092cd9340a5b89d5bf715dbb0f8c8aad9634

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 28 • 10 years ago

(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?