Closed Bug 665655 Opened 10 years ago Closed 9 years ago
Input Data .m Value really used and usable
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.
I forgot the type change case. I've updated the test (see bug 665865).
Comment on attachment 540715 [details] [diff] [review] Patch v2 s/UpdateStates/UpdateState/ Why is getting rid of the BF_PARSER_CREATING checks ok?
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).
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?
(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.
> 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...
(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 on attachment 540715 [details] [diff] [review] Patch v2 Meant to mark this too...
Attachment #540715 - Flags: review?(bzbarsky) → review+
Mounir, can this be landed?
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.
(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.
Ah, thanks, Mounir.
Whiteboard: [needs review]
Target Milestone: --- → mozilla21
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.