Closed Bug 1303748 Opened 4 years ago Closed 4 years ago

incorrect color conversion with JS replacement functions

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox52 verified)

VERIFIED FIXED
Firefox 52
Iteration:
52.1 - Oct 3
Tracking Status
firefox52 --- verified

People

(Reporter: tromey, Assigned: gregtatum)

References

Details

(Whiteboard: [reserve-html])

Attachments

(1 file)

I found a bug somewhere in one of the color format conversion functions
that we rewrote in JS.
STR:

Make a document that uses the color "seagreen".
Open the rule view.
Shift click on the color swatch to change the color format.
Do this multiple times and notice that it doesn't cycle back
through "seagreen".  One of the conversions computes the wrong
value.
Flags: qe-verify?
Whiteboard: [devtools-html] → [devtools-html] [triage]
Flags: qe-verify? → qe-verify-
Priority: -- → P3
Whiteboard: [devtools-html] [triage] → [reserve-html]
Flags: qe-verify- → qe-verify+
QA Contact: petruta.rasa
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Iteration: --- → 51.3 - Sep 19
Priority: P3 → P1
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
Comment on attachment 8795371 [details]
Bug 1303748 - Fix color cycling in CssColor by adding more HSL precision; r+tromey

https://reviewboard.mozilla.org/r/81434/#review80296

Thank you.  I like this; I just had one question/possible to-do concerning the test.

::: devtools/client/shared/test/browser_css_color.js:56
(Diff revision 2)
> + */
> +function runCycle(value, times) {
> +  let color = new colorUtils.CssColor(value);
> +  for (let i = 0; i < times; i++) {
> +    color.nextColorUnit();
> +    new colorUtils.CssColor(color.toString());

This value isn't used.  Is this line here to verify that the parsing works?

I agree that is worth doing; but I think it's also important to check round-tripping in a way that is insulated from the internals of CssColor.  That is, shouldn't this be checking that the resulting object is equal to "color"?
Attachment #8795371 - Flags: review?(ttromey) → review+
Comment on attachment 8795371 [details]
Bug 1303748 - Fix color cycling in CssColor by adding more HSL precision; r+tromey

https://reviewboard.mozilla.org/r/81434/#review80296

> This value isn't used.  Is this line here to verify that the parsing works?
> 
> I agree that is worth doing; but I think it's also important to check round-tripping in a way that is insulated from the internals of CssColor.  That is, shouldn't this be checking that the resulting object is equal to "color"?

This was a mistake. I refactored some code and lost the `color =` on that line. I'm recreating the CssColor instance each time, then testing the the values match at the end of it.
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b974a64f96bc
Fix color cycling in CssColor by adding more HSL precision; r=tromey
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5083173624d5
Fix color cycling in CssColor by adding more HSL precision; r+tromey r=tromey
https://hg.mozilla.org/mozilla-central/rev/5083173624d5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Flags: needinfo?(gtatum)
I reproduced this bug using Fx 52.0a1, build ID:  20160919065232, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 52.0a1, build ID: 20161004030204 on Windows 10 x64, mac OS X 10.10.5 and Ubuntu 14.04 LTS. Now all the conversions display the correct values.

Cheers!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.