863466 - [system] focus the alert/confirm/etc dialogs so that the keyboard is dismissed

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

When showing an alert from an application, we don't put the focus on the alert box. This has 2 consequences:

- if the keyboard was displayed for some reason, it is not dismissed
- it has accessibility problems

patch is coming

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: patch v1

---
 apps/system/js/modal_dialog.js |    4 ++++
 1 file changed, 4 insertions(+)

Comment 2

[Julien Wajsberg [:julienw]]

blocks bug 843511 -> tef+

Comment 3

[Alex Keybl [:akeybl]]

Please renominate if the alert is not readable, the alert can't be dismissed, or the keyboard is nonfunctional after this occurs.

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: screenshot of the problem

Comment 5

[Julien Wajsberg [:julienw]]

Here is the screenshot of what's happening.

However the keyboard is easily dismissable by tapping in the alert.

Comment 6

[Julien Wajsberg [:julienw]]

Josh, could you check the choice that I made here ?

* alert box : the focus is on OK button
* prompt box : the focus is on the input
* confirm box : the focus is on the cancel button
* select one : the focus is on the cancel button

The rationale is to not focus the "action" button by default (except for the alert box because this is the only button).

The other sane approach is to focus the dialog instead, so that the impaired user always have to do at least one navigation action to act.

Comment 7

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

Comment on attachment 739297 [details] [diff] [review]
patch v1

Per discussion with :julienw on IRC, let's cancel this review first.
My opinion without UX decision on this would be focusing on the whole dialog instead of button only.

Comment 8

[Josh Carpenter [:jcarpenter]]

Does the non-impaired user see the difference between which element is focused? If not, I don't have a strong opinion here. Focusing on the whole prompt sounds okay, but keep in mind that we use some prompts with keyboards. Will this patch, or focusing on the prompt, cause problems?

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

With the attached screenshot it does look like the user would not be able to read the entire alert and might not know they can tap in the alert box to dismiss the keyboard, so blocking.

Comment 10

[Julien Wajsberg [:julienw]]

Josh, theorically, the non-impaired user should still see which elements are focused, but I think the visual clues have been removed in Gaia (which makes sense in the mobile context).

I'll need to test the various prompts to see how this behaves, I admit I only tested the alert box for now.

Patch to come next week.

Comment 11

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

(In reply to Julien Wajsberg [:julienw] from comment #10)
> Josh, theorically, the non-impaired user should still see which elements are
> focused, but I think the visual clues have been removed in Gaia (which makes
> sense in the mobile context).

Yes, I think it's
button::-moz-focus-inner {
  border: 0;
}

> 
> I'll need to test the various prompts to see how this behaves, I admit I
> only tested the alert box for now.
> 
> Patch to come next week.

;)

Comment 12

[Josh Carpenter [:jcarpenter]]

(In reply to Julien Wajsberg [:julienw] from comment #10)
> Josh, theorically, the non-impaired user should still see which elements are
> focused, but I think the visual clues have been removed in Gaia (which makes
> sense in the mobile context).
> 
> I'll need to test the various prompts to see how this behaves, I admit I
> only tested the alert box for now.
> 
> Patch to come next week.

Thanks Julien. Can you attach a screenshot of the new version when it's ready and needsinfo Patryk?

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Are more patches forthcoming?  Does this still reproduce or did it get resolved in another bug?

Comment 14

[Julien Wajsberg [:julienw]]

so, yes, I was hoping to do it, and did not. Still should be easy to do, I'll take it in our next week's sprint.

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: patch v2

This changes the dialogs in both the browser and the system, for the apps
content and the web content.

In this patch we focus the whole dialog, so that no default action is selected
by default. This effectively fixes the initial bug, and probably makes
accessibility better, even if we'll need more work for this.
---
 apps/browser/index.html         |   12 ++++++++----
 apps/browser/js/modal_dialog.js |    6 +++++-
 apps/system/index.html          |    8 ++++----
 apps/system/js/modal_dialog.js  |    4 ++++
 4 files changed, 21 insertions(+), 9 deletions(-)

There is also a Github PR in https://github.com/mozilla-b2g/gaia/pull/10079

I used a testcase that I made in http://everlong.org/mozilla/testcase-alert/. When running it from the Browser it lets you try the Browser prompts, and if you bookmark it on the homescreen it lets you try the System prompts.

This is still not totally like on the Desktop. On the desktop, we have the focus on the dialogs, but when the dialog closes, the focus goes back to where it was before. I suggest that we file another bug if this proves to be important for accessibility or the device normal use.

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 755383 [details] [diff] [review]
patch v2

r=benfrancis? for the browser part

Comment 17

[Julien Wajsberg [:julienw]]

Need Info Patryk and Josh.

Patryk, in this patch, what this basically does is to focus the whole prompt (for alert/confirm/prompt dialogs) when it's displayed. The effect is :

* hides the keyboard if it was displayed. I suspect that most apps used workarounds to this in the past.
* probable accessibility improvements

Note that in the browser, the previous behaviour on prompt inputs was to focus on the input, which effectively showed the keyboard if it was displayed, but it was _hiding_ it with my use cases, due to a race condition. So in the new approach this problem would disappear, and the user just can tap the input to enter something.

For apps dialogs, we didn't focus anything.

Note that the prompt dialogs look different in the browser and for apps. I don't know if that's done on purpose, might make sense to file a new bug for this.

Comment 18

[Patryk Adamczyk [:patryk] UX]

Hey Julien, makes sense to me.
And yes the browser has a mix of errors. For the most part the dialogs should look the same. Is there a specific place where you see the dialogs looking off? I believe we have a bug to address this.

Comment 19

[Julien Wajsberg [:julienw]]

The input in the prompt dialogs look different; just use my testcase http://everlong.org/mozilla/testcase-alert/ in the browser and as a bookmark to find out.

Thanks !

Comment 20

[Josh Carpenter [:jcarpenter]]

(In reply to Julien Wajsberg [:julienw] (PTO 30th May -> 10th June with no access to my bugmail) from comment #17)
> Need Info Patryk and Josh.
> 
> Patryk, in this patch, what this basically does is to focus the whole prompt
> (for alert/confirm/prompt dialogs) when it's displayed. The effect is :
> 
> * hides the keyboard if it was displayed. I suspect that most apps used
> workarounds to this in the past.
> * probable accessibility improvements
> 
> Note that in the browser, the previous behaviour on prompt inputs was to
> focus on the input, which effectively showed the keyboard if it was
> displayed, but it was _hiding_ it with my use cases, due to a race
> condition. So in the new approach this problem would disappear, and the user
> just can tap the input to enter something.
> 
> For apps dialogs, we didn't focus anything.
> 
> Note that the prompt dialogs look different in the browser and for apps. I
> don't know if that's done on purpose, might make sense to file a new bug for
> this.

Hi Julien, your solution makes sense to me. I do not see any glaring downsides, and it addresses the problems, so let's proceed.

Comment 21

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

Comment on attachment 755383 [details] [diff] [review]
patch v2

r=alive

Comment 22

[Julien Wajsberg [:julienw]]

Ben, would you please commit it yourself if you're giving r+ as I'm leaving for holiday ?

Comment 23

[Ben Francis [:benfrancis]]

Comment on attachment 755383 [details] [diff] [review]
patch v2

Review of attachment 755383 [details] [diff] [review]:
-----------------------------------------------------------------

Browser part looks good to me but I have a question about prompt dialogs (inline).

Also, out of curiosity what are the tab indices for?

::: apps/browser/js/modal_dialog.js
@@ +113,5 @@
>        case 'prompt':
>          elements.prompt.hidden = false;
>          elements.promptInput.value = evt.detail.initialValue;
>          elements.promptMessage.innerHTML = message;
> +        elements.prompt.focus();

In the case of prompt, do we not want to focus on the text box rather than the whole prompt? In this case the user does actually need the keyboard.

Comment 24

[Ben Francis [:benfrancis]]

Comment on attachment 755383 [details] [diff] [review]
patch v2

As Josh is happy with the change in UX, r+me

Comment 25

[Ben Francis [:benfrancis]]

Landed on master https://github.com/mozilla-b2g/gaia/commit/bee87c84899f432e9ed7a434ecb552ba8f610f6b

Needs uplift to v1-train

Comment 26

[Julien Wajsberg [:julienw]]

(In reply to Ben Francis [:benfrancis] from comment #23)

> 
> Also, out of curiosity what are the tab indices for?

Without the tab indices, you can't focus this element (because it's not a usually focussable element)

> 
> ::: apps/browser/js/modal_dialog.js
> @@ +113,5 @@
> >        case 'prompt':
> >          elements.prompt.hidden = false;
> >          elements.promptInput.value = evt.detail.initialValue;
> >          elements.promptMessage.innerHTML = message;
> > +        elements.prompt.focus();
> 
> In the case of prompt, do we not want to focus on the text box rather than
> the whole prompt? In this case the user does actually need the keyboard.

I discussed a lot with myself and I about this, and also I've seen some race conditions (which would disappear if we put the focus call inside a setTimeout callback), and therefore we all settled on this solution.

If over time we think it's inconvenient, then we'd still be able to go back here.

Thanks !

Comment 27

[Julien Wajsberg [:julienw]]

Marking resolved/fixed because the patch landed, and this should hopefully make the uplifters see it.

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 bee87c84899f432e9ed7a434ecb552ba8f610f6b
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 29

[Julien Wajsberg [:julienw]]

The conflicts was because of space changes in Bug 870739.

v1-train: 0198c8980b454deb965830a729d5951952daf010

Comment 30

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd: 0198c8980b454deb965830a729d5951952daf010

Comment 31

[Jason Smith [:jsmith]]

Verified on b2g 18 using http://jds2501.github.io/webapi-permissions-tests/delayedAlert.html by selecting each type of test (alert, confirm, prompt), selecting the text field, and waiting for the prompt to fire. When it fires, verified that the focus goes to the prompt.