977276 - Crash reporter is visible to screen reader when screen is locked.

Status: RESOLVED FIXED

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

Description

[Eitan Isaacson [:eeejay]]

It is possible to navigate to the crash screen dialog even when the lockscreen is showing. This should not be the case!

Comment 1

[Eitan Isaacson [:eeejay]]

Re-aligning priorities with 2.1 accessibility goals.

Comment 2

[Eitan Isaacson [:eeejay]]

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

Pretty straightforward. The assumption here is that any dialog in dialog-overlay is hidden when the screen is locked.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

This is trivial but not sure if this is correct. First, I think the selector is better to be written as |#screen.dialog:not(.locked) #dialog-overlay| here.

https://github.com/eeejay/gaia/blob/bug-977276/apps/system/style/system/system.css#L284-L287

However, I am not sure if it's possible we encounter a situation where we need to show the dialog on top of the lock screen -- your CSS rule will likely break that. Flagging :alive to confirm this.

Comment 4

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

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

What Tim said.
Since crash dialog lives in dialog-overlay it's impossible to show it on lockscreen now.

I don't know what we do to b2g crash on boot though. I always see the crash dialog shows without lockscreen if b2g crashed but I am not sure how we do this..

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Alive, this patch is about setting visibility hidden to #dialog-overlay, so it will make ALL dialogs inside dialog-overlay (not just the crash dialog) not being able to show on lock screen.

Just want to confirm with you that no dialog in #dialog-overlay will ever appear on top of the lock screen.

PS Crash dialog is the dialog confirming with user on sending crash reports.

Comment 6

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

Dialog overlay includes
* Permission dialog
* App install dialog
* Value selector (will be removed after bug 962434
* System modal dialog
* Emergency callback dialog

Now with current zindex.css it's impossible to display anything under dialog-overlay on lockscreen.
If there's any new UX spec change we could move some of them out of dialog-overlay, but I don't think the patch will cause regression.

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #3)
> Comment on attachment 8452701 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21505
> 
> This is trivial but not sure if this is correct. First, I think the selector
> is better to be written as |#screen.dialog:not(.locked) #dialog-overlay|
> here.
> 

The crash reporter does not give the screen a class of 'dialog', only subclasses of SystemDialog do that, so changing that rule will not work.

Comment 8

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

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

After checking again, I am confident anything under #dialog-overlay will not be shown according to z-index.css

Permission has its own z-index and it's out of dialog overlay.

BTW for the long term should be let visibility manager to change UI visibility as well, if we end up having lots of css rules to change visibility..

Comment 9

[Eitan Isaacson [:eeejay]]

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

Reflagging timdream for review after feedback+ from alive.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

You did not address my review comment on comment 3 with the |:not()| selector, but I don't think it's something worthy blocking landing of this patch for.

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #10)
> Comment on attachment 8452701 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/21505
> 
> You did not address my review comment on comment 3 with the |:not()|
> selector, but I don't think it's something worthy blocking landing of this
> patch for.

I tried to address that, sorry if it was not clear. The selector won't work for the crash reporter since the screen won't get a class of dialog.

Comment 12

[Eitan Isaacson [:eeejay]]

https://github.com/mozilla-b2g/gaia/commit/9c93a08fd3ecc3781c531b46017a563254ce9c90