981248 - <input> has focus/blur issues when it has focus and its type changes to/from type=number

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

The first input in the test case switches its type from 'text' to 'number' in its onfocus handler, and switches back in onblur. However, after it switches to number, it no longer accepts any input. This happens because the focus is still on the parent element rather than on the (newly created) anonymous child element.

I think the block at [1] is supposed to fix this problem, but it doesn't actually work because the anonymous child is not bound to the document at that point, so nsFocusManager::SetFocus() fails. SetFocus() wouldn't work in any case, because assuming the anonymous child is focused, the parent input element will get a blur event; during the blur handler, the input type is switched back to text and everything is destroyed; we still don't get a focused number input.

I think we have to make some changes to nsFocusManager to fix this. 

[1] http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsNumberControlFrame.cpp?rev=63a4ad62401a#359

Comment 1

[Jonathan Watt [:jwatt]]

There are other related issues when changing the type. For example, bug 1057858 comment 3.

https://jsfiddle.net/5gc7upLj/

Comment 2

[Jimmy Gaussen]

Here is a version with all the current input types: http://jsfiddle.net/5gc7upLj/2/

This confirms that it only occurs with the number type.

Comment 3

[Mike Taylor [:miketaylr]]

This breaks the "Search by flight number" status functionality on westjet.com's mobile site.

(If I had to guess, sites do this because they want the "number" keypad (on mobile), but not the ugly spinners that they can't turn off.)

Comment 4

[Mike Taylor [:miketaylr]]

Bumping up to P3 because this breaks real sites (not sure anyone uses priority values, but all the same...)

Comment 5

[Bob Stein]

Wasted hours tracking this down. Rather subtle. Reported at https://stackoverflow.com/questions/38770152/firefox-blurs-when-changing-the-input-type where it was blamed on AngularJS.

However here's a demo of it happening with jQuery. The field can only be edited once in Firefox. https://jsfiddle.net/BobStein/4mL902vo/7/

Changing an input field from text to number causes the field to spontaneously blur.

Comment 6

[Mike Taylor [:miketaylr]]

Attachment: input-number-focus.html

Comment 7

[Francis]

This is quite an annoying issue, as DOM insertion includes changing the type. Our use case is for an INTL currency input:
- By default be input type="text" for INTL currency formatting,
- On focus/click change to input type="number" to benefit from browser number functionality.
- On blur change to input type="text" for INTL currency formatting again.

In Reactive UI programming this is much hit upon problem:
- https://github.com/facebook/react/issues/11062
- https://stackoverflow.com/questions/38770152/firefox-blurs-when-changing-the-input-type

And there appears to be multiple related issues on bugzilla:
https://bugzilla.mozilla.org/show_bug.cgi?id=981248
https://bugzilla.mozilla.org/show_bug.cgi?id=1012818

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Make nsNumberControlFrame extend nsTextControlFrame instead of creating an anonymous text control.

I had this WIP patch around, which simplifies a bunch of stuff and should fix this as well, but needs a bit more work.

I didn't want to lose track of it.

Comment 10

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating webcompat priority whiteboard tags to project flags.

Comment 11

[Thomas Wisniewski [:twisniewski]]

Attachment: wpt.html

I could not find a test like this already in the web platform tests (or the Blink or WebKit test suites), so here is a testharness.js WPT (like uievents/click/auxclick_event.html) which should do the trick.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I had a patch to stop using the hacky anonymous text control for number inputs somewhere in bugzilla, I should probably rescue that patch.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Oh, it was attached to this bug. :-)

I'll try to rebase it.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

I have https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b09ae8ee993fb9c9c055118153ef8b32e85b826, which kinda works. I just pushed it to try to see where it stands.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(Didn't want to drop the ni?)

Comment 16

[Emilio Cobos Álvarez (:emilio)]

So from the try run:

• Needs some a11y fixes (this should be easy-ish).
• I broke some font inflation stuff looks like, and need to check the placeholder / preview stuff. I probably need to tweak some reflow bits or some CSS there to make layout correct and consistent.
• There's an assert that needs investigation. Potentially related to the above.
• I broke localization of the number but that (a) it is untested (b) it is bogus and (c) no other browser does it.

So all in all seems a bit of work, but probably workable.

Comment 17

[Thomas Wisniewski [:twisniewski]]

This is also breaking Sunlife's page on Android Firefox browsers, because it causes the on-screen keyboard to never be brought up and users cannot type in their age/etc.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. r=masayuki!,jfkthame!,surkov!,jwatt!,ntim!

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9115915 [details]
Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. r=masayuki!,jfkthame!,surkov!,jwatt!,ntim!

Mats, Jonathan, interested in your thoughts on this.

I still need to fix a few things, mainly:

• Text has the wrong baseline (though input element uses the right one).
• ::placeholder / preview-div stuff needs to be fixed to be out of flow and positioned with CSS or something, instead of the current thing we do hiding them during the display list...

I still haven't decided whether I want the wrapper div everywhere. It may be worth simplifying the setup, plus it can be the containing block for the placeholder / preview, and thus may save quite a bit of trouble. But I haven't investigated it much.

I also need to look into some of the DOM bits (value sanitization and such), but off-hand the amount of code that goes away looks pretty neat.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Also note, it does change the layout a bit. In particular the buttons now appear inside the <input> background. But that's a feature IMO, in my experience number inputs at least on Linux look terrible, and with my patch they look nicer to me at least.

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 9115915 [details]
Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. r=masayuki!,jfkthame!,surkov!,jwatt!,ntim!

Looks great so far! And yes, we might want to merge them eventually, but it's probably better to do that as a follow-up bug.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Masayuki, ni? for https://phabricator.services.mozilla.com/D57193#1760448.

Also, over-all feedback on the patch would be much appreciated, I do like that it removes a bunch of special code in layout and focus management.

Here's a try run with the latest bits of test fixes and such: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cfaca2e461b8471c10cd0ee0ef8a2bbbfceab04

Apart of the input events that I'm asking you about I need to do some work on the a11y bits. But I've discussed that with the a11y folks and it looks straight-forward.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 981248 - Fix test_input_number_mouse_events.html to not dispatch mouse events from input events. r=masayuki

The test fails with my <input type=number> rewrite because editor fails to
dispatch one input event caused by these mouse events.

The reason for this is that we schedule them from an input event, which fires
from here:

https://searchfox.org/mozilla-central/rev/6305f6935f496b3a302c7afcc579399a4217729c/editor/libeditor/EditorBase.cpp#965

Note how at that point we still haven't decremented mPlaceholderBatch. That means
that other stuff that triggers input events from there will not dispatch events.

I think that's a bit unexpected, but it is a preexisting problem, and can't
happen for users because mouse events go through the event loop.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

This one should be all green :-)

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=38a675afdc6b50c87ca971d4703bd70a1742ac95

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9115915 [details]
Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. r=masayuki!,jfkthame!,surkov!,jwatt!,ntim!

Sent a review request instead, now that's green. Thanks y'all for your help!

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Tom, do you want to send the WPT in a separate patch? I verified locally that it passes as expected, but it'd be great if you preserved authorship. I can roll it into the patch above otherwise, but...

A minor nit: It'd be amazing to drop a <link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=981248"> on the test too, so people looking at the test can get all the context :)

Comment 27

[Thomas Wisniewski [:twisniewski]]

I'm actually alright with it being part of this patch. If you don't mind dropping that link into it, it should be more than enough of a trail for folks to follow back, if they really are curious who wrote it (or more importantly why). Is that alright with you?

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Sure, why not.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9fac9eb1c24c
Fix test_input_number_mouse_events.html to not dispatch mouse events from input events. r=masayuki

Comment 30

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/9fac9eb1c24c

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1eace7bd28d9
Rewrite <input type=number> to avoid an anonymous input. r=masayuki,surkov,jwatt,ntim,jfkthame,smaug

Comment 32

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21167 for changes under testing/web-platform/tests

Comment 33

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 981248 - Remove now-unneeded special-case in AutoFillDelegateTest.kt. r=snorp

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a96708cc8b7
Remove now-unneeded special-case in AutoFillDelegateTest.kt. r=snorp

Comment 36

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 37

[Natalia Csoregi [:nataliaCs]]

Backed out for multiple failures.

backout: https://hg.mozilla.org/integration/autoland/rev/89c1fd1db3b14b99560fa69ff2cb5dd9d2152950

1. https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=gv-junit&revision=1eace7bd28d9986cdd43f5ecb6393a4aa8e9bffc
   failure log e.g.: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284860483&repo=autoland&lineNumber=1440

2. https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=wpt3&revision=1eace7bd28d9986cdd43f5ecb6393a4aa8e9bffc&selectedJob=284862847
   failure log e.g.: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284862847&repo=autoland&lineNumber=48859

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/010bd182aff8
Rewrite <input type=number> to avoid an anonymous input. r=masayuki,surkov,jwatt,ntim,jfkthame,smaug

Comment 39

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 40

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/010bd182aff8

Comment 44

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot