1088053 - KeyPad for emergency calls always visible to screen reader

Status: RESOLVED FIXED

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

Description

[Marco Zehe (:MarcoZ)]

This is a recent regression, introduced on October 21 or 22. The key pad for emergency calls is visible to the screen reader. it is either explorable at the bottom of the screen, partially visible, or it can be swiped to from the browser bar at the top of the home screen. it also gets in the way in Utility tray and a lot of other places.

STR:
1. Turn on screen reader.
2. Unlock phone.
3. Touch address bar at the top.
4. Swipe left.

Expected: Land on last status bar item as before.
Actual: Land on Undo button of the Key pad.

In addition, the Cancel button is not operable. Probably because it is normally supposed to be hidden behind other things/hidden completely.

5. Lock phone.
6. Press Power button to bring screen back up.
7. Touch where the Unlock button would be.

Result: wKey Pad is in the way, too.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25470

I gave the LockScreenInputWindow an initial state of aria-hidden which seems to me to be correct in all cases. There is no unit test coverage for that class, so I am not sure where tests would be necessary.

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8510638 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25470

I think we are calling normal setVisible (which remove/add the aria-hidden attribute) on LockscreenInputWindow as well?

Comment 3

[Eitan Isaacson [:eeejay]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #2)
> Comment on attachment 8510638 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25470
> 
> I think we are calling normal setVisible (which remove/add the aria-hidden
> attribute) on LockscreenInputWindow as well?

Indeed. With this patch, i see expected behavior: show/hide like all other windows.

Comment 4

[Greg Weng [:snowmantw][:gweng][:λ]]

While I think it's good to me, Alive, should I check why setVisible doesn't work for the window and fire another bug for it? Or we just keep this solution and close the bug?

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #4)
> While I think it's good to me, Alive, should I check why setVisible doesn't
> work for the window and fire another bug for it? Or we just keep this
> solution and close the bug?

I think it's due to we are not calling this.setVisible in initialization of LockscreenInputWindow.
Please open another bug to trace and let's close this one at first. (Unless you or Eitan want to fix it in this one..)

Comment 6

[Greg Weng [:snowmantw][:gweng][:λ]]

fired: Bug 1089449

Comment 7

[Eitan Isaacson [:eeejay]]

This is similar to our race conditions with initial home screen visibility and lockscreen visibility. I don't think initial visibility has ever been resolved well. I am tempted to simply have aria-hidden in this window since there is never a case when it is visible initially.

Comment 8

[Eitan Isaacson [:eeejay]]

Comment on attachment 8510638 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25470

This is a serious bug that renders the phone unusable with a screen reader. I realize that there may be a more complete solution for assuring that screen visibility is set correctly initially. But I think setting aria-hidden=true here by default is a first step.

Comment 9

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8510638 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25470

Yes, I understand the situation we're. Thanks for the fix, and let's move to submit a proper patch later. Although I think it would be better if we can submit a patch with a unit test, since as an untitled developer I've been directly yielded that why I give a r+ for a patch without unit tests, even though the patch is so trivial that IMO the test is unnecessary. But since you and Alive are peer and even owner, maybe it's OK and nobody would yield about that.

Comment 10

[Eitan Isaacson [:eeejay]]

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

Thanks!
I think we should test for initial visibility, and making sure that each window get setVisible called on it initially at startup. I guess the followup bug would be perfect for that.