Don't flush frames when setting input type to 'number'

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: deckycoss, Assigned: deckycoss)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
HTMLInputElement::SetType calls FlushFrames() when the new type is new type is "number": https://hg.mozilla.org/mozilla-central/file/a3c769e95c38/dom/html/HTMLInputElement.h#l627

apparently this is only useful for certain tests that simulate key events (example: https://reviewboard.mozilla.org/r/70708/diff/5#4). even then, there doesn't seem to be a reason to treat "number" as a special case. i think the call can be removed safely.

Comment 1

a year ago
It seems like jwatt added this code in the initial implementation of <input type=number>, but it's not really needed since in the case where the events are coming from real users, the frame tree should be updated.  This is only useful for tests that change the type and then synthesizeKey() or some such immediately after without yielding to the event loop.  In such tests we should just flush pending notifications ourselves like we currently do.
Comment hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
Comment on attachment 8780652 [details]
Bug 1294784: remove special case for 'number' type from HTMLInputElement::SetType;

https://reviewboard.mozilla.org/r/71314/#review68838

As far as I know, we don't automatically flush layout when there are keyboard events coming. So wouldn't this lead to some errors if one does something like
<input type=text oninput="this.type = 'number'">  and user types really really fast.
Attachment #8780652 - Flags: review?(bugs) → review-
(Assignee)

Comment 4

a year ago
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8780652 [details]
> Bug 1294784: remove special case for 'number' type from
> HTMLInputElement::SetType;
> 
> https://reviewboard.mozilla.org/r/71314/#review68838
> 
> As far as I know, we don't automatically flush layout when there are
> keyboard events coming. So wouldn't this lead to some errors if one does
> something like
> <input type=text oninput="this.type = 'number'">  and user types really
> really fast.

even if it does, shouldn't that already apply to any type other than number?
(Assignee)

Comment 5

a year ago
olli, please see comment #4
Flags: needinfo?(bugs)

Comment 6

a year ago
Also note that we do flush frames when painting if the DOM has changed, and in the scenario you're describing, there will be a painting in between the keyboard events.  If this wasn't the case, we had the bug you described with this alternative test case:

<input type=number oninput="this.type = 'text'">
(In reply to :Ehsan Akhgari from comment #6)
> Also note that we do flush frames when painting if the DOM has changed, and
> in the scenario you're describing, there will be a painting in between the
> keyboard events. 
There isn't painting between keyboard events. Nothing guarantees that.

But I guess we have similar issue elsewhere, so it should be fixed in more generic way, and removing this code is fine.
Flags: needinfo?(bugs)

Comment 8

a year ago
mozreview-review
Comment on attachment 8780652 [details]
Bug 1294784: remove special case for 'number' type from HTMLInputElement::SetType;

https://reviewboard.mozilla.org/r/71314/#review71422
Attachment #8780652 - Flags: review- → review+
(Assignee)

Comment 9

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5892dfe9e6d2
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 10

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e0ae32fee155
remove special case for 'number' type from HTMLInputElement::SetType; r=smaug
Keywords: checkin-needed

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0ae32fee155
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.