Closed Bug 981248 Opened 11 years ago Closed 5 years ago

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

Categories

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

Product:
Core
Component:
Layout: Form Controls
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla74
Project Flags:
Webcompat Priority P1
Tracking Flags:
Tracking Status
firefox27 --- unaffected
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- wontfix
firefox74 --- fixed

People

(Reporter: jchen, Assigned: emilio)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: [webcompat:p1], [wptsync upstream])

Attachments

(4 files, 2 obsolete files)

input-number-focus.html
(deleted), text/html
Details
wpt.html
(deleted), text/html
Details
Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. r=masayuki!,jfkthame!,surkov!,jwatt!,ntim!
(deleted), text/x-phabricator-request
Details
Bug 981248 - Fix test_input_number_mouse_events.html to not dispatch mouse events from input events. r=masayuki
(deleted), text/x-phabricator-request
Details
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
Severity: normal → minor
Priority: -- → P4
There are other related issues when changing the type. For example, bug 1057858 comment 3. https://jsfiddle.net/5gc7upLj/
Summary: Input is not focused when switching input type to number on focus → <input> has focus/blur issues when it has focus and its type changes to/from type=number
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.
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.)
Bumping up to P3 because this breaks real sites (not sure anyone uses priority values, but all the same...)
Severity: minor → normal
Priority: P4 → P3
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.
Flags: webcompat+
Whiteboard: [webcompat:p3]
Attached file input-number-focus.html (deleted) —
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
Whiteboard: [webcompat:p3] → [webcompat:p2]
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.
Whiteboard: [webcompat:p2] → [webcompat:p1]
Priority: P3 → P2

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

Webcompat Priority: --- → P1
Attached file wpt.html (deleted) —

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.

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

Flags: needinfo?(emilio)

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

I'll try to rebase it.

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.

Flags: needinfo?(emilio)

(Didn't want to drop the ni?)

Flags: needinfo?(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.

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Depends on: 1565214
Blocks: 1585216
Blocks: 1587014

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.

See Also: → https://webcompat.com/issues/41946
Attachment #9004793 - Attachment is obsolete: true
Attachment #9115915 - Attachment description: Bug 981248 - Rebased wip: Displays something but will probably crash awfully. → Bug 981248 - Better wip: Can edit and such, element has the right baseline.

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.

Attachment #9115915 - Flags: feedback?(mats)
Attachment #9115915 - Flags: feedback?(jwatt)

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

Attachment #9115915 - Flags: feedback?(mats) → feedback+
Blocks: 1430141
Attachment #9115915 - Attachment description: Bug 981248 - Better wip: Can edit and such, element has the right baseline. → Bug 981248 - Rewrite <input type=number> to avoid an anonymous input.

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.

Flags: needinfo?(emilio) → needinfo?(masayuki)
Assignee: nobody → emilio

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.

Attachment #9115915 - Attachment description: Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. → Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. r=masayuki!,mats!,surkov!,jwatt!

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!

Flags: needinfo?(masayuki)
Attachment #9115915 - Flags: feedback?(jwatt)
Attachment #9115915 - Flags: feedback+

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 :)

Flags: needinfo?(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?

Flags: needinfo?(twisniewski)

Sure, why not.

Keywords: leave-open
Pushed by ealvarez@mozilla.com: 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
Attachment #9115915 - Attachment description: Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. r=masayuki!,mats!,surkov!,jwatt! → Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. r=masayuki!,mats!,surkov!,jwatt!,ntim!
Attachment #9115915 - Attachment description: Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. r=masayuki!,mats!,surkov!,jwatt!,ntim! → Bug 981248 - Rewrite <input type=number> to avoid an anonymous input. r=masayuki!,jfkthame!,surkov!,jwatt!,ntim!
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1eace7bd28d9 Rewrite <input type=number> to avoid an anonymous input. r=masayuki,surkov,jwatt,ntim,jfkthame,smaug
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/21167 for changes under testing/web-platform/tests
Whiteboard: [webcompat:p1] → [webcompat:p1], [wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Blocks: 1609167
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7a96708cc8b7 Remove now-unneeded special-case in AutoFillDelegateTest.kt. r=snorp
Upstream PR was closed without merging
Flags: needinfo?(emilio)
Attachment #9120787 - Attachment is obsolete: true
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/010bd182aff8 Rewrite <input type=number> to avoid an anonymous input. r=masayuki,surkov,jwatt,ntim,jfkthame,smaug
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Regressions: 1609394
Regressions: 1609589
Upstream PR merged by moz-wptsync-bot
status-firefox28: affected → wontfix
status-firefox29: affected → wontfix
status-firefox30: affected → wontfix
status-firefox74: --- → fixed
Flags: in-testsuite+
Target Milestone: --- → mozilla74
Regressions: 1611118
Regressions: 1611661
Regressions: 1611701
Blocks: 1611713
Regressions: 1616620
Regressions: 1617342
Blocks: 1473237
Depends on: 1620778
Depends on: 1105920
Regressions: 1621927
Regressions: 1621981
Regressions: 1622221
Blocks: 1555551
Regressions: 1842027
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: