885996 - Interaction between nsIColorPicker and content

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch

This is using the patch in bug 875747 to interact between <input type='file'> and the nsIColorPicker widget. It is taking care of the input and change events.

I am going to attach tests later.

Comment 1

[Arnaud Bienner]

(In reply to Mounir Lamouri (:mounir) from comment #0)
> between <input type='file'>
I guess you meant <input type='color'> ;)

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Tests

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 766270 [details] [diff] [review]
Patch

returnOK/Cancel are odd names for consts, but not really about this bug.

Could you make |win| nsCOMPtr. It is used so far a way from declaration that
making a security bug by accident is easy. And this is not perf-critical code.

aTrustedUpdate is odd name. Perhaps call it aFinalUpdate or some such.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 766338 [details] [diff] [review]
Tests


>+      if (element.dataset.type == 'update') {
>+        MockColorPicker.returnColor = '#f00ba4';
>+        update();
>+
>+        is(inputEvent, true, 'input event should have been received');
>+        is(changeEvent, false, 'change event should not have been received');
>+
>+        inputEvent = changeEvent = false;
>+
>+        is(element.value, MockColorPicker.returnColor);
>+
>+        MockColorPicker.returnColor = '#f00ba7';
>+        isnot(element.value, MockColorPicker.returnColor);
>+      } else if (element.dataset.type == 'cancel') {
>+        MockColorPicker.returnColor = '#bababa';
>+        isnot(element.value, MockColorPicker.returnColor);
>+      } else if (element.dataset.type == 'done') {
>+        MockColorPicker.returnColor = '#098766';
>+        isnot(element.value, MockColorPicker.returnColor);
>+      } else if (element.dataset.type == 'noop-done') {
>+        MockColorPicker.returnColor = element.value;
>+        is(element.value, MockColorPicker.returnColor);
>+      }
>+
I don't understand what these .returnColor = foo; is[not] tests are trying to test.
Do they test how MockColorPicker works or what?

>+  <meta charset="utf-8">
>+  <title>Test for Bug 1234567</title>
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+  <script type="application/javascript;version=1.8">
>+
>+  /** Test that update() modifies the element value such as done() when it is
>+   * not called as a concellation.
This whole sentence is hard to parse.
You mean cancellation and 'the same way as done() ...' or  something like that?

Comment 5

[Mounir Lamouri (:mounir)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 766338 [details] [diff] [review]
> Tests
> 
> 
> >+      if (element.dataset.type == 'update') {
> >+        MockColorPicker.returnColor = '#f00ba4';
> >+        update();
> >+
> >+        is(inputEvent, true, 'input event should have been received');
> >+        is(changeEvent, false, 'change event should not have been received');
> >+
> >+        inputEvent = changeEvent = false;
> >+
> >+        is(element.value, MockColorPicker.returnColor);
> >+
> >+        MockColorPicker.returnColor = '#f00ba7';
> >+        isnot(element.value, MockColorPicker.returnColor);
> >+      } else if (element.dataset.type == 'cancel') {
> >+        MockColorPicker.returnColor = '#bababa';
> >+        isnot(element.value, MockColorPicker.returnColor);
> >+      } else if (element.dataset.type == 'done') {
> >+        MockColorPicker.returnColor = '#098766';
> >+        isnot(element.value, MockColorPicker.returnColor);
> >+      } else if (element.dataset.type == 'noop-done') {
> >+        MockColorPicker.returnColor = element.value;
> >+        is(element.value, MockColorPicker.returnColor);
> >+      }
> >+
> I don't understand what these .returnColor = foo; is[not] tests are trying
> to test.
> Do they test how MockColorPicker works or what?

Those tests are indeed naive but the idea is to check that the value did (or did not) change when the color picker current color changed. There is no way to really express that with MockColorPicker so I expressed that with .returnColor which makes those checks a bit stupid. Though, they do not hurt and if we end up having a better way to change the current selected color in MockColorPicker we will have tests for free.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 766338 [details] [diff] [review]
Tests

this is rs'ish r+

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch

Olli, I applied your change requests and some changes requested by roc in the interface review. Could you review this new patch?
(I am going to attach an inter-diff.)

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: inter-diff

Comment 9

[Arnaud Bienner]

The new patch looks good to me.
I've updated the Gtk Widget in bug 875753 accordingly, so you can use it if you want to test this patch "for real".

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: Patch

Some cleaning up.

Comment 11

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/997713d63032
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e3c4e5cba74

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/997713d63032
https://hg.mozilla.org/mozilla-central/rev/3e3c4e5cba74