Closed Bug 977029 Opened 6 years ago Closed 6 years ago

When changing the value of <input type="color">, its default value is also changed

Categories

(Core :: Layout: Form Controls, defect)

28 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox28 + unaffected
firefox29 + verified
firefox30 + verified

People

(Reporter: claude.pache, Assigned: arnaud.bienner)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140218122424

Steps to reproduce:

1. take a form with an <input type="color"> and a reset button (see testcase attached)
2. change the value of the <input>
3. reset the form


Actual results:

The <input type="color"> does not change its value 


Expected results:

The <input type="color"> value should be reset to its initial value
Further experience with the console: Load the testcase attached with this bug in Firefox 28 beta, open the console, and type in order:

------------
x = document.querySelector('input[type=color]')

// A. changing the value will change the defaultValue
x.value = '#ffff00'
x.defaultValue // '#ffff00' —- changed

// B. changing the defaultValue attribute will change the value attribute 
// if and only if the value attribute was previously changed
x.form.reset()
x.value // '#ffff00' -- ok
x.defaultValue // '#ffff00' -- ok

x.defaultValue = '#00ff00'
x.value // #00ff00 -- changed!

x.value = '#00ff00'
x.defaultValue = '#0000ff'
x.value // '#00ff00' -- not changed (correct)

// C. reset the form: the value attribute is updated, but not the actual color shown
x.form.reset()
x.value // #0000ff, or x.defaultValue: correct
// however, the color displayed on the <input> has not been updated!
------------------

I'm going to fill another bug report for issue (C); just note that, because of it, when testing the resetting the form, you should check the `value` property of the input instead of just looking at the color displayed.
(In reply to Claude Pache from comment #1)
> defaultValue attribute ... value attribute ...
Please read: "defaultValue property" ... "value property" ...
(In reply to Claude Pache from comment #1)
> I'm going to fill [sic] another bug report for issue (C)
Filed: Bug 977038
Yeah, this is just totally broken.  The code added in HTMLInputElement::SetValueInternal for bug 875275 that sets the attribute on any value set is just wrong.  Why was that code needed, exactly?  And how come we have no basic sanity tests for this input type?  :(
Blocks: 875275
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(arnaud.bienner)
Keywords: regression
If all you wanted was to notify the frame, by the way, then you should notify the frame, instead of modifying DOM state that means something just to get some side effects...
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #4)
> And how come we have no basic sanity tests for this input type?  :(

There are (landed as part of bug 875274).
(but not enough, it seems...)

> Why was that code needed, exactly?
> [...]
> If all you wanted was to notify the frame, by the way, then you should
> notify the frame, instead of modifying DOM state that means something just
> to get some side effects...

Indeed this is what I wanted to do. And as it worked, I didn't tried to find another way to achieve this. I honestly didn't thought this might have bad side effects.
How can I notify the frame so the updated value is reflected?
Flags: needinfo?(arnaud.bienner)
> There are (landed as part of bug 875274).

None of those test the defaultValue behavior, afaict.

> How can I notify the frame so the updated value is reflected?

See what the NS_FORM_INPUT_NUMBER case does in SetValueInternal: you get your frame, and if it's the right sort of frame call a method on it directly.
Assignee: nobody → arnaud.bienner
Status: NEW → ASSIGNED
Attached patch WIP: bug977029.patch (obsolete) — Splinter Review
I believe this will fix the problem reported, as well as the one reported in bug 977029.
Still need to add some tests and to review/clean the patch.
OS: Mac OS X → All
Hardware: x86 → All
Attached patch bug977029.patch (obsolete) — Splinter Review
Same patch, but with test cases added (including reftest for the rendering problem reported in comment 1 C and bug 977038).
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=98b1965e70eb

I don't know who can review this. I thought about you, Boris, but looking at your status, it looks like you're busy.
Maybe you can delegate this to someone else? Maybe Daniel, as he already worked with me on input type color?
Daniel, do you feel confident to review this?
Attachment #8382575 - Attachment is obsolete: true
Attachment #8383293 - Flags: review?(dholbert)
Attachment #8383293 - Flags: review?(bzbarsky)
Comment on attachment 8383293 [details] [diff] [review]
bug977029.patch

>+for (i in testData) {
>+  data = testData[i];

  for (var data of testData) {

It would also be good to test that changing defaultValue before the first time the value changed changes the value.  And that changing defaultValue after the first time the value changed does not change the value.

r=me
Attachment #8383293 - Flags: review?(bzbarsky) → review+
Comment on attachment 8383293 [details] [diff] [review]
bug977029.patch

[clearing feedback request, as bz got to this before I did.]
Attachment #8383293 - Flags: review?(dholbert)
Tests cases suggested in comment 10 added.
Marking it as r+ directly as per comment 10, but don't hesitate to have a look if you want :)
Attachment #8383293 - Attachment is obsolete: true
Attachment #8383865 - Flags: review+
For the record, the interdiff.
New patch pushed to try and everything looks OK: https://tbpl.mozilla.org/?tree=Try&rev=3a5a2da145ce
Keywords: checkin-needed
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed443a0daa3

(Before pushing, I added "when color value changes, " to the beginning of the commit message, for a little more context.)

We should request Aurora backport approval for this in a few days, probably (after this has made it to mozilla-central and gotten a bit of Nightly testing.)  Setting needinfo=arnaud to do that.
Flags: needinfo?(arnaud.bienner)
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6ed443a0daa3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Verified as fixed with latest Nightly 30.0a1 on Ubuntu 12.04 x86 and Mac OS X 10.8.5
Status: RESOLVED → VERIFIED
We've disabled input type=color in FF28 so marking unaffected there.
Duplicate of this bug: 977038
Comment on attachment 8383865 [details] [diff] [review]
bug977029-2.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 875275. Introduced the "changing the value of an input type color changes the defaultValue as well".
User impact if declined:
Bug mentioned above + the frame of the input type color isn't refreshed when reseting the corresponding form (while the value is reset, as expected). This isn't a regression as bug 875275 introduced the layout part of this input type.
Testing completed (on m-c, etc.): tested locally on Windows and Linux (manual testing + reftests/mochitest) + pushed to try + in Nightly since a few days now.
Risk to taking this patch (and alternatives if risky): low, small change
String or IDL/UUID changes made by this patch: none
Attachment #8383865 - Flags: approval-mozilla-aurora?
Flags: needinfo?(arnaud.bienner)
Attachment #8383865 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed with latest Aurora 29.0a2 on Ubuntu 13.10 x86, Mac OS X 10.8.5 and Win 7 x64.
You need to log in before you can comment on or make changes to this bug.