Closed
Bug 1122463
Opened 10 years ago
Closed 9 years ago
It's impossible to focus input from submit button click handler in B2G
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: azasypkin, Assigned: timdream)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
timdream
:
review+
|
Details | Diff | Splinter Review |
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;
Reporter | ||
Comment 1•10 years ago
|
||
Hey Ehsan,
Could you please help to understand the issue from comment 0 we see in B2G?
Thanks!
Flags: needinfo?(ehsan)
Comment 2•10 years ago
|
||
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)
Reporter | ||
Comment 3•10 years ago
|
||
(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•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
smaug said he can probably take a look.
Flags: needinfo?(overholt) → needinfo?(bugs)
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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.
Reporter | ||
Comment 8•10 years ago
|
||
(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)
Comment 9•10 years ago
|
||
(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)
Reporter | ||
Comment 10•10 years ago
|
||
(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)
Comment 11•10 years ago
|
||
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.)
Reporter | ||
Comment 12•10 years ago
|
||
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)
Comment 13•10 years ago
|
||
(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)
Reporter | ||
Comment 14•10 years ago
|
||
Thanks Alive, just wanted to exclude this possibility.
Reporter | ||
Comment 15•10 years ago
|
||
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? → ---
Comment 16•10 years ago
|
||
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•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
Will respond tomorrow office hours. Thanks for investigating.
Component: DOM: Events → DOM: Device Interfaces
Assignee | ||
Comment 20•10 years ago
|
||
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)
Comment 21•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
Another better possibility is to not blur if e.defaultPrevented is true (which would mean we won't change the page).
Reporter | ||
Comment 23•10 years ago
|
||
(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)
Assignee | ||
Comment 24•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 26•10 years ago
|
||
(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.
Assignee | ||
Comment 27•10 years ago
|
||
I am blocked on bug 1129344 appearing in latest master build today. Will pick up this bug later.
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8562573 -
Flags: review?(xyuan)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 29•10 years ago
|
||
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+
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8562573 -
Attachment is obsolete: true
Attachment #8563234 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 31•10 years ago
|
||
Keywords: checkin-needed
Comment 32•10 years ago
|
||
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)
Assignee | ||
Comment 33•10 years ago
|
||
No worries. I will figure out a new patch for this. ... we might need to change Gaia too.
Flags: needinfo?(timdream)
Assignee | ||
Comment 34•9 years ago
|
||
Update: this bug will be effectively fixed by bug 1162360 when it lands.
Depends on: 1162360
Assignee | ||
Comment 35•9 years ago
|
||
(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
Assignee | ||
Comment 36•9 years ago
|
||
I tried to re-test this bug 1166193 breaks m-c...
Assignee | ||
Comment 37•9 years ago
|
||
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.
Assignee | ||
Comment 38•9 years ago
|
||
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.
Assignee | ||
Comment 39•9 years ago
|
||
Will try to re-land this Gecko patch after bug 1166615 reaches Gaia master and after another pass of Try run.
Assignee | ||
Comment 40•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca264815b361
Try run that includes the Gaia patch.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 41•9 years ago
|
||
Keywords: checkin-needed
Assignee | ||
Comment 42•9 years ago
|
||
(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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•