incorrect color conversion with JS replacement functions

VERIFIED FIXED in Firefox 52

Status

enhancement
P1
normal
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: tromey, Assigned: gregtatum)

Tracking

unspecified
Firefox 52
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

(Whiteboard: [reserve-html])

Attachments

(1 attachment)

Reporter

Description

3 years ago
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

Updated

3 years ago
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Iteration: --- → 51.3 - Sep 19
Priority: P3 → P1
Iteration: 51.3 - Sep 19 → 52.1 - Oct 3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 5

3 years ago
mozreview-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

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 hidden (mozreview-request)
Assignee

Comment 7

3 years ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)

Comment 10

3 years ago
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
Comment hidden (mozreview-request)

Comment 13

3 years ago
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

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5083173624d5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Assignee

Updated

3 years ago
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+

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.