1122463 - It's impossible to focus input from submit button click handler in B2G

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Oleg Zasypkin [:azasypkin]]

Attachment: test-case.html

In bug 1119013 we found that the root cause was that we call "input.focus()" right inside submit button click handler and it doesn't have any effect (to prevent submission we have form.addEventListener('submit', (e) => e.preventDefault())). 

Using "submit" in our case isn't really correct and we'll change it to "button" type to resolve this issue. But it would be great to understand if it's correct behaviour or not since test-case (please, see attachment) works in Fx Nightly and doesn't in B2G browser.

There are 4 buttons in test case that try to focus input field:
- 1st one is button with default type located outside of the form (form is rectangle with blue background) - works as expected in both Fx Nightly and B2G;
- 2nd one is button with default type located inside the form - works as expected in Fx Nightly and doesn't in B2G;
- 3rd one is button with default type located inside the form that wraps "focus" call into "setTimeout" - works as expected in both Fx Nightly and B2G;
- 4th one is plain div with click handler inside the form - works as expected in both Fx Nightly and B2G;

Comment 1

[Oleg Zasypkin [:azasypkin]]

Hey Ehsan,

Could you please help to understand the issue from comment 0 we see in B2G?

Thanks!

Comment 2

[(no longer active)]

Not sure what kind of info you need.  Yes, this is a bug.  But I don't have any idea why it happens.  Someone needs to debug it.

Comment 3

[Oleg Zasypkin [:azasypkin]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> Not sure what kind of info you need.  Yes, this is a bug.  But I don't have
> any idea why it happens.  Someone needs to debug it.

Thanks! I only wanted to know whether it's a bug (you confirmed that) and who can help to investigate and fix it since it blocks v2.2 blocker.

We can change button type to "button" and eventually we'll do, but having platform fix assuming it's not very complex is nice to have anyway.

Comment 4

[(no longer active)]

Andrew, can anyone look at this?  I don't think it requires specific skills, but I am juggling too many things at the moment.

Comment 5

[Andrew Overholt [:overholt]]

smaug said he can probably take a look.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Is there some way to reproduce this on desktop (desktop b2g or something) or does this need
to be tested on device?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Also, have you by any chance tested if you actually get the input element focused, but then
focus is moved back to button?

And does it help if you make the button <button type="button">?
<button>'s default type is after all submit.

Comment 8

[Oleg Zasypkin [:azasypkin]]

(In reply to Olli Pettay [:smaug] from comment #6)
> Is there some way to reproduce this on desktop (desktop b2g or something) or
> does this need
> to be tested on device?

I was able to repro this in Mulet [1] with the following steps:

1. Build gaia with "DEBUG=1 make";
2. Run mulet with the gaia profile built at step 1 - ./path_to_mulet/firefox-bin -profile /path_to_gaia/profile-debug --no-remote;
3. Open "chrome://b2g/content/shell.html" in mulet to run System;

It's reproducible in B2G Desktop as well.

> Also, have you by any chance tested if you actually get the input element
> focused, but then
> focus is moved back to button?

As far as I remember I only examined "document.activeElement" for all four buttons, at the beginning it's "body" after you click "submit" button it's still "body", for the rest it's "input". 

> And does it help if you make the button <button type="button">?
> <button>'s default type is after all submit.

Sure, changing type to "button" fixes the issue. We'll make it "button" anyway.

[1] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Different_ways_to_run_Gaia#Using_Gaia_in_Firefox_Mulet

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #8)
> As far as I remember I only examined "document.activeElement" for all four
> buttons, at the beginning it's "body" after you click "submit" button it's
> still "body", for the rest it's "input". 
well, would be better to check all the focus and blur events and their targets.


> Sure, changing type to "button" fixes the issue. We'll make it "button"
> anyway.
Interesting. submit does do some validation by default. I wonder if focus changes because of that.

So if you'll make it type="button",  does this bug need to be fixed for 2.2?

Comment 10

[Oleg Zasypkin [:azasypkin]]

(In reply to Olli Pettay [:smaug] from comment #9)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #8)
> > As far as I remember I only examined "document.activeElement" for all four
> > buttons, at the beginning it's "body" after you click "submit" button it's
> > still "body", for the rest it's "input". 
> well, would be better to check all the focus and blur events and their
> targets.

Will try to check later today.

> > Sure, changing type to "button" fixes the issue. We'll make it "button"
> > anyway.
> Interesting. submit does do some validation by default. I wonder if focus
> changes because of that.
> 
> So if you'll make it type="button",  does this bug need to be fixed for 2.2?

I'm testing all places (in Messages app) where we have that "form->button[type="submit"].onclick = () => ...focus.." at the moment. If it works with "button" type everywhere, then it's not a blocker for us (Messages app) anymore, I just suspect that other gaia apps can have similar uncovered yet issues. At least it'd be great to know why it doesn't work in B2G only.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Sure, which is why it would be great to know a bit more.
(I don't atm have the right dev environment so can't test, but when this needs to be fixed, will then of course set it up.)

Comment 12

[Oleg Zasypkin [:azasypkin]]

Just some intermediate results:

* For non-submit button, sequence of the events is the following
** FOCUS on button;
** BLUR on button;
** FOCUS on input;

* For submit button, we have this sequence:
** FOCUS on button;
** BLUR on button;
** FOCUS on input;
** BLUR on input;

I've attached "focus" and "blur" event handlers for every element in test case including window, document and body and I don't see who's stealing the focus. Strange... 

Just wondering if System app can somehow affect it, like System app detects that one of its nested iframes has form that's going to be submitted. That would explain why this issue is B2G only. Alive, do you know if that can be possible at all?

Comment 13

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

(In reply to Oleg Zasypkin [:azasypkin] from comment #12)
> Just some intermediate results:
> 
> * For non-submit button, sequence of the events is the following
> ** FOCUS on button;
> ** BLUR on button;
> ** FOCUS on input;
> 
> * For submit button, we have this sequence:
> ** FOCUS on button;
> ** BLUR on button;
> ** FOCUS on input;
> ** BLUR on input;
> 
> I've attached "focus" and "blur" event handlers for every element in test
> case including window, document and body and I don't see who's stealing the
> focus. Strange... 
> 
> Just wondering if System app can somehow affect it, like System app detects
> that one of its nested iframes has form that's going to be submitted. That
> would explain why this issue is B2G only. Alive, do you know if that can be
> possible at all?

System app does not steal your focus until the user is touching the UI of system app like rocketbar.

> like System app detects
> that one of its nested iframes has form that's going to be submitted

This is wrong to me because we don't know who is doing submitting. We event don't know which iframe is focus - but only who is the top most iframe and that's all. Sorry but I don't think this is related to system app.

Comment 14

[Oleg Zasypkin [:azasypkin]]

Thanks Alive, just wanted to exclude this possibility.

Comment 15

[Oleg Zasypkin [:azasypkin]]

Removing blocking flag here for now since we have small and low-risk workaround for this issue in bug 1119013 (changing "submit" button type to "button"). Still it would be good to know the root cause.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Something in b2g steals focus from input element.
Anyone with a b2g could a breakpoint to check what is on the stack when blur event for the input is dispatched.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I think I found it.
https://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js?rev=d4b4cca22c19&mark=384-386#383
I have no idea why we have that code.

Comment 18

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

Will respond tomorrow office hours. Thanks for investigating.

Comment 19

[Julien Wajsberg [:julienw]]

So this comes from bug 1057898.

Comment 20

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

OK.

So I did the change in bug 1057898 because without the change forms.js simply hides the keyboard (actually, send out the async ipc message resulting keyboard being hidden) without removing the focus, and I consider that as a state disparity -- we *always* require a on-screen keyboard when the focus is kept no matter what.

The submit (and pagehide) event listener was added in bug 820057. Maybe we should just remove the submit event listener here and have the page only try to hide the keyboard with pagehide/beforeunload event? Would that be considered a regression of the STR in bug 820057 since the timing of keyboard hiding will be a little bit late?

Once we decide what to do the patch is pretty trivial...

Comment 21

[Julien Wajsberg [:julienw]]

Would this help if you'd use a listener in "capture" mode, so that you call "blur" before "normal" submit handlers are run ?

Comment 22

[Julien Wajsberg [:julienw]]

Another better possibility is to not blur if e.defaultPrevented is true (which would mean we won't change the page).

Comment 23

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #22)
> Another better possibility is to not blur if e.defaultPrevented is true
> (which would mean we won't change the page).

I think the same, if submit is prevented then  we shouldn't blur anything.

Comment 24

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

(In reply to Julien Wajsberg [:julienw] from comment #21)
> Would this help if you'd use a listener in "capture" mode, so that you call
> "blur" before "normal" submit handlers are run ?

We are indeed binding the event listener in capturing phase.
https://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#196
So if this works it would have already working.

I am going to try to work on probing |evt.defaultPrevented| to see if that could fix the test case.

Comment 25

[Julien Wajsberg [:julienw]]

If we're in capture mode then you won't be able to access defaultPrevented. So I think you need to use "normal" mode instead.

I think that capture mode doesn't work because I think we actually call focus on the "click" event. So it happens before the "submit" event anyway.

Comment 26

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

(In reply to Julien Wajsberg [:julienw] from comment #25)
> If we're in capture mode then you won't be able to access defaultPrevented.
> So I think you need to use "normal" mode instead.
> 
> I think that capture mode doesn't work because I think we actually call
> focus on the "click" event. So it happens before the "submit" event anyway.

Thanks for the tip.

Comment 27

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

I am blocked on bug 1129344 appearing in latest master build today. Will pick up this bug later.

Comment 28

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

Attachment: bug1122463.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=180ad96aa91f

Comment 29

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8562573 [details] [diff] [review]
bug1122463.patch

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

It is preferable to have a test:)

Comment 30

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

Attachment: Patch for commit

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11b4c8d470ab

Comment 32

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6562463&repo=mozilla-inbound

Comment 33

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

No worries. I will figure out a new patch for this. ... we might need to change Gaia too.

Comment 34

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

Update: this bug will be effectively fixed by bug 1162360 when it lands.

Comment 35

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

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #34)
> Update: this bug will be effectively fixed by bug 1162360 when it lands.

Oh wait, the two are unrelated....

Comment 36

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

I tried to re-test this bug 1166193 breaks m-c...

Comment 37

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

I can now verify patch in bug 1162360 will not fix this. Going to fix the test errors of the patch here and try to re-land it.

Comment 38

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

It turned out that Rocketbar is expecting blur we want to turn off here:

https://github.com/mozilla-b2g/gaia/blob/01938468cf518009e08262532c6dfc56b6cb2210/apps/system/js/rocketbar.js#L582-L583

These Gij tests failure because the keyboard does not close as expected.

I am going to patch Rocket Bar first to make it blur the input manually.

Comment 39

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

Will try to re-land this Gecko patch after bug 1166615 reaches Gaia master and after another pass of Try run.

Comment 40

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca264815b361

Try run that includes the Gaia patch.

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb30fa0f7fc

Comment 42

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

(In reply to Pulsebot from comment #41)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb30fa0f7fc

This is bad... this patch is supposed to go to b2g-inbound. I don't think mozilla-inbound have already picked up bug 1166615.

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ffb30fa0f7fc