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

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

blocks bug 843511 -> tef+

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

Here is the screenshot of what's happening. However the keyboard is easily dismissable by tapping in the alert.

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

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?

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.

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.

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

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

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

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.

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 on attachment 755383 [details] [diff] [review] patch v2 r=benfrancis? for the browser part

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.

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.

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 !

(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 on attachment 755383 [details] [diff] [review] patch v2 r=alive

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

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 on attachment 755383 [details] [diff] [review] patch v2 As Josh is happy with the change in UX, r+me

Landed on master https://github.com/mozilla-b2g/gaia/commit/bee87c84899f432e9ed7a434ecb552ba8f610f6b Needs uplift to v1-train

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

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

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

The conflicts was because of space changes in Bug 870739. v1-train: 0198c8980b454deb965830a729d5951952daf010

1.1hd: 0198c8980b454deb965830a729d5951952daf010

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.