Closed Bug 1122463 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: azasypkin, Assigned: timdream)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file 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;
Hey Ehsan,

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

Thanks!
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
(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.
Blocks: 1119013
Status: UNCONFIRMED → NEW
blocking-b2g: --- → 2.2?
Ever confirmed: true
Andrew, can anyone look at this?  I don't think it requires specific skills, but I am juggling too many things at the moment.
Flags: needinfo?(overholt)
smaug said he can probably take a look.
Flags: needinfo?(overholt) → needinfo?(bugs)
Is there some way to reproduce this on desktop (desktop b2g or something) or does this need
to be tested on device?
Flags: needinfo?(azasypkin)
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.
(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
Flags: needinfo?(azasypkin)
(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?
Flags: needinfo?(bugs) → needinfo?(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.
Flags: needinfo?(azasypkin)
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.)
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?
Flags: needinfo?(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.
Flags: needinfo?(alive)
Thanks Alive, just wanted to exclude this possibility.
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.
blocking-b2g: 2.2? → ---
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.
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.
Flags: needinfo?(timdream)
Will respond tomorrow office hours. Thanks for investigating.
Component: DOM: Events → DOM: Device Interfaces
No longer blocks: 1119013
So this comes from bug 1057898.
Blocks: 1057898
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...
Assignee: nobody → timdream
Flags: needinfo?(timdream)
Flags: needinfo?(felash)
Flags: needinfo?(azasypkin)
Would this help if you'd use a listener in "capture" mode, so that you call "blur" before "normal" submit handlers are run ?
Flags: needinfo?(felash)
Another better possibility is to not blur if e.defaultPrevented is true (which would mean we won't change the page).
(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.
Flags: needinfo?(azasypkin)
(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.
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.
(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.
I am blocked on bug 1129344 appearing in latest master build today. Will pick up this bug later.
Status: NEW → ASSIGNED
Comment on attachment 8562573 [details] [diff] [review]
bug1122463.patch

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

It is preferable to have a test:)
Attachment #8562573 - Flags: review?(xyuan) → review+
Attached patch Patch for commitSplinter Review
Attachment #8562573 - Attachment is obsolete: true
Attachment #8563234 - Flags: review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=6562463&repo=mozilla-inbound
Flags: needinfo?(timdream)
No worries. I will figure out a new patch for this. ... we might need to change Gaia too.
Flags: needinfo?(timdream)
Update: this bug will be effectively fixed by bug 1162360 when it lands.
Depends on: 1162360
(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....
No longer depends on: 1162360
I tried to re-test this bug 1166193 breaks m-c...
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.
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.
Will try to re-land this Gecko patch after bug 1166615 reaches Gaia master and after another pass of Try run.
(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.
https://hg.mozilla.org/mozilla-central/rev/ffb30fa0f7fc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.