1225236 - CSS-Filter Popup Caps Hue-Rotate deg at 360

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Helen V. Holmes (:helenvholmes) (:✨)]

The CSS filter popup doesn't currently allow values over 360deg, even though MDN says that values over 360deg are valid: "Though there is no maximum value, the effect of values above 360deg wraps around."

My UX two-cents: we should follow spec and accept degree values > 360.

Steps to recreate:
1. Visit http://bennettfeely.com/filters/
2. Inspect hue-rotate
3. Open css-filter pop-up on the <figure> element
4. Try and add a value > 360deg. Oh noes, it doesn't wrap!

Comment 1

[Patrick Brosset <:pbro>]

Good catch, thanks. Marking this as a good first bug because I'm pretty sure it is.
File to modify is: \devtools\client\shared\widgets\FilterWidget.js
Search for "hue-rotate" in this file, there's a range defined there.
Also, this test file should be changed accordingly: \devtools\client\shared\test\browser_filter-editor-02.js I think a new test case should be added to make sure the filter works well with angles > 360.

Comment 2

[Tim Nguyen :ntim]

I honestly don't know why this is needed. We should tell the dev that there are more simple values available (ie we should convert values higher than 360deg, to a 360deg scale).

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Tim Nguyen [:ntim] from comment #2)
> I honestly don't know why this is needed. We should tell the dev that there
> are more simple values available (ie we should convert values higher than
> 360deg, to a 360deg scale).

I guess it currently seems arbitrary to lock it down...  You're going around circle, so it wraps around you'd expect.  Since you can "roll" the value with arrow keys, if you're at say 350 deg, I think it's nice keep going up 350 -> 359 -> 360 -> 370 etc.

Simplifying the value to be within 0 - 360 seems maybe "unnecessarily helpful" to me...?  

If we did want to keep clamping to 360, then I would purpose rolling with arrow keys should wrap 360 back to 0, instead of locking you to 360.

Comment 4

[Patrick Brosset <:pbro>]

I tend to agree with comment 3. I guess one way to reason about this is: no devtools UI should ever get in the way of a user writing valid JS, CSS, HTML. filter:hue-rotate(370deg); is valid CSS, so the CSS filter tooltip should not prevent you from writing it.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

I'm gonna take a look at this as my first bug. 
Can someone with bug-editing privileges assign it to me ?
Thank you

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #5)
> I'm gonna take a look at this as my first bug. 
> Can someone with bug-editing privileges assign it to me ?
> Thank you
Awesome, thanks for wanting to fix this.
Please feel free to ask questions on IRC #devtools if you need, or use the "need more info" field at the bottom of this page with my name.
When you do upload a patch, remember to ask for either feedback or review by setting the corresponding field on the attachment upload page.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: proposed patch for bug resolution

I ran the associated ( and edited ) test with :

./mach test devtools/client/shared/test/browser_filter-editor-02.js 

and I think it passed :

INFO TEST-OK | devtools/client/shared/test/browser_filter-editor-02.js

I tried to follow the guidelines for the commit message but I'm not sure if it's okay.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8696065 [details] [diff] [review]
proposed patch for bug resolution

Review of attachment 8696065 [details] [diff] [review]:
-----------------------------------------------------------------

I've applied this locally and tested it, it works great, thanks!
One comment regarding the commit message: please change r=reviewers with r=pbro
Other than this, I have no comments regarding the code changes.
So I'll R+ this patch. Please just re-upload a new one with the new commit message, mark this one as obsolete, and set the new one as R+

I've pushed the patch to our TRY infra to make sure all tests still pass with it applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff545766acab

If everything turns green, then we'll check this in. Can you please keep an eye on it and ping me if you see errors or when you think everything's done?

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: patch with edited commit message

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Thank you Patrick. I'll keep an eye on the TRY infra tests progress.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/965cfbe44aa5

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

I think the tests are done here https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff545766acab , apart from 3 tests that seems to timeout in Linux Debug. I don't really know if it's caused by the patch.

Does https://bugzilla.mozilla.org/show_bug.cgi?id=1225236#c11 means the patch is commit and thus the bug resolved ?

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #12)
> I think the tests are done here
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff545766acab , apart
> from 3 tests that seems to timeout in Linux Debug. I don't really know if
> it's caused by the patch.
> 
> Does https://bugzilla.mozilla.org/show_bug.cgi?id=1225236#c11 means the
> patch is commit and thus the bug resolved ?
Yes! Your job is done here, thanks a lot for fixing this Nicolas!
So, I checked yesterday the try result and I saw that everything was green apart from the unrelated failures you mentioned in comment 12, so what I did is I added the keyword "checkin-needed".
A bit later, the commit was pushed to the fx-team integration branch.
fx-team is one of our branches were we land new stuff and bug fixes. If all tests pass on that branch too, then it will be merged to the mozilla-central repository. When it does, then the bug will be marked RESOLVED/FIXED.
This means that your fix will be part of Firefox 45 (which will be reaching the dev-edition channel next week I believe).

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/965cfbe44aa5