665655 - Make mInputData.mValue really used and usable

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

As said in bug 665612, this change should be implicitly tested by <input type='number'> tests but I will probably open a follow-up to make sure all value modes are tested.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

I forgot the type change case. I've updated the test (see bug 665865).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 540715 [details] [diff] [review]
Patch v2

s/UpdateStates/UpdateState/

Why is getting rid of the BF_PARSER_CREATING checks ok?

Comment 3

[Mounir Lamouri (:mounir)]

IIRC, this check was here to prevent creating too much string during document parsing, thus reducing document loading speed. It could have been there to prevent value sanitizing but ::SetValueInternal is already preventing value sanitizing during parsing. Ideally, we should find a way to still reduce string creation but the problem is a bit harder than it was and except if it happens to be a real issue regarding speed, I think we could try to fix that in a follow-up, ideally after improving our test suite for this code (which isn't bad though).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ok, I see.  You're right that as long as we don't sanitize during parsing this is probably safe, but it does mean that we'll end up doing the SetAttr thing for non-value value modes during parsing, right?  You're right that it's just a performance issue, but I would still prefer we didn't have it.

Is there a reason we actually need to run that code while parsing?

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to comment #4)
> Is there a reason we actually need to run that code while parsing?

It handles these situations:
<input type='foo' value='bar'>
and
<input value='bar' type='foo'>

In both situations, the type changes and the value is set before or after it. Though, we could probably run HandleTypeChange only when the parsing is done and set the value at this moment. I believe no one can call input.value in the middle of the parsing.

If you think this patch shouldn't be pushed, I can have a look at this in this bug. Otherwise, in a follow-up.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> It handles these situations:

Well, I understand that; I just don't see why those situations need a SetAttr call when going to a non-value type while parsing.

I guess let's get this landed and file a followup for optimizations...

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to comment #6)
> > It handles these situations:
> 
> Well, I understand that; I just don't see why those situations need a
> SetAttr call when going to a non-value type while parsing.

Indeed, we don't have to. I could probably keep the condition to |!parsing or mode=value|.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 540715 [details] [diff] [review]
Patch v2

Meant to mark this too...

Comment 9

[(no longer active)]

Mounir, can this be landed?

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: testcase for type=range that fails

Mounir, the patch you uploaded to Try does not appear to work with the type=range patches you had applied. See this testcase for example, where the value alerted should be zero.

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to Jonathan Watt [:jwatt] from comment #10)
> Created attachment 713089 [details]
> testcase for type=range that fails
> 
> Mounir, the patch you uploaded to Try does not appear to work with the
> type=range patches you had applied. See this testcase for example, where the
> value alerted should be zero.

That's actually because of your patch. When you change the value because it is above max, you call SetValue(value) but you should call SetValueInternal(value, false, false), otherwise the value is considered to be set by the user, so, setAttr(max, 10) ends up being like setAttr(max, 10); .value = 10; which is not what you want.

Comment 12

[Jonathan Watt [:jwatt]]

Ah, thanks, Mounir.

Comment 13

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25ae6325380b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25ae6325380b