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

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Claude Pache]

Attachment: An HTML form with an <input type="color"> element and a reset button

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

Comment 1

[Claude Pache]

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.

Comment 2

[Claude Pache]

(In reply to Claude Pache from comment #1)
> defaultValue attribute ... value attribute ...
Please read: "defaultValue property" ... "value property" ...

Comment 3

[Claude Pache]

(In reply to Claude Pache from comment #1)
> I'm going to fill [sic] another bug report for issue (C)
Filed: Bug 977038

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Arnaud Bienner]

(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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Arnaud Bienner]

Attachment: WIP: bug977029.patch

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.

Comment 9

[Arnaud Bienner]

Attachment: bug977029.patch

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?

Comment 10

[Boris Zbarsky [: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

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8383293 [details] [diff] [review]
bug977029.patch

[clearing feedback request, as bz got to this before I did.]

Comment 12

[Arnaud Bienner]

Attachment: bug977029-2.patch

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

Comment 13

[Arnaud Bienner]

Attachment: bug977029.inter.diff

For the record, the interdiff.

Comment 14

[Arnaud Bienner]

New patch pushed to try and everything looks OK: https://tbpl.mozilla.org/?tree=Try&rev=3a5a2da145ce

Comment 15

[Daniel Holbert [:dholbert]]

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.

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6ed443a0daa3

Comment 17

[Manuela Muntean [Away]]

Verified as fixed with latest Nightly 30.0a1 on Ubuntu 12.04 x86 and Mac OS X 10.8.5

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

We've disabled input type=color in FF28 so marking unaffected there.

Comment 20

[Arnaud Bienner]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c0771d13ec26

Comment 22

[Manuela Muntean [Away]]

Verified as fixed with latest Aurora 29.0a2 on Ubuntu 13.10 x86, Mac OS X 10.8.5 and Win 7 x64.