Bug 166937 – Web Inspector: gradient editor should provide horizontal slider for 'angle' value where applicable

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

This is for linear and linear-radial gradients.

[Blaze Burg]

Also, the label should be aligned left and have a trailing colon.


I think this is a more standard layout:

Angle: [  ]  |------o---|

[Radar WebKit Bug Importer]

<rdar://problem/29974201>

[Devin Rousso]

Created attachment 298650 [details]
Patch

[Devin Rousso]

Created attachment 298651 [details]
[Image] After Patch is applied

The <input type="range"> slider track will be fixed once <https://webkit.org/b/166933> lands.

[Devin Rousso]

Currently the units for the angle input are "deg", but this will be fixed by <https://webkit.org/b/166938>.  This will also effect the Gradient model object and the RegExp used to match gradients in CSS.

[Build Bot]

Comment on attachment 298650 [details]
Patch

Attachment 298650 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2877250

New failing tests:
fast/mediastream/MediaStream-video-element-track-stop.html

[Build Bot]

Created attachment 298675 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 298691 [details]
Patch

[Blaze Burg]

Comment on attachment 298691 [details]
Patch

Pretty close. Some issues:
- because of the connective ':', it would make more sense to have the text field adjacent to the label, and have the slider beyond that
- I think we should support an input range of [-180, 180] or [-360, 360] rather than [0, 360]. And equivalent for other units. What do you think? In that case we'd need a '0' label at the midpoint.
- There's no units label, you should add a placeholder 'deg' one outside of the text field. When this gets enhanced to support multiple units, it'd be a drop-down menu like in the visual style sidebar.

[Devin Rousso]

(In reply to comment #9)
> Comment on attachment 298691 [details]
> Patch
> 
> Pretty close. Some issues:
> - because of the connective ':', it would make more sense to have the text
> field adjacent to the label, and have the slider beyond that

Every slider + input combo I've seen so far has been <label> |---o---| [ ].  I think it makes more sense to drop the ":"  and keep the layout considering that that's how the rest of them look.

> - I think we should support an input range of [-180, 180] or [-360, 360]
> rather than [0, 360]. And equivalent for other units. What do you think? In
> that case we'd need a '0' label at the midpoint.

When I tried entering a negative angle it would always recalculate to its positive value.  Regardless, I don't really like the idea of encouraging the use of negative angles.

> - There's no units label, you should add a placeholder 'deg' one outside of
> the text field. When this gets enhanced to support multiple units, it'd be a
> drop-down menu like in the visual style sidebar.

Good point.  If we add this label, I think it makes even more sense to have the text field  be on the right as otherwise the "deg" will be in between the text field and slider.

[Blaze Burg]

(In reply to comment #10)
> (In reply to comment #9)
> > Comment on attachment 298691 [details]
> > Patch
> > 
> > Pretty close. Some issues:
> > - because of the connective ':', it would make more sense to have the text
> > field adjacent to the label, and have the slider beyond that
> 
> Every slider + input combo I've seen so far has been <label> |---o---| [ ]. 
> I think it makes more sense to drop the ":"  and keep the layout considering
> that that's how the rest of them look.

Ok, let's take your suggestion.

> > - I think we should support an input range of [-180, 180] or [-360, 360]
> > rather than [0, 360]. And equivalent for other units. What do you think? In
> > that case we'd need a '0' label at the midpoint.
> 
> When I tried entering a negative angle it would always recalculate to its
> positive value.  Regardless, I don't really like the idea of encouraging the
> use of negative angles.

Oh, okay. This sounds fine as is I guess.

> > - There's no units label, you should add a placeholder 'deg' one outside of
> > the text field. When this gets enhanced to support multiple units, it'd be a
> > drop-down menu like in the visual style sidebar.
> 
> Good point.  If we add this label, I think it makes even more sense to have
> the text field  be on the right as otherwise the "deg" will be in between
> the text field and slider.

There are lots of precedent for either way. Let's stick with it on the right side.

[Devin Rousso]

Created attachment 298749 [details]
Patch

[Devin Rousso]

Created attachment 298750 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

Comment on attachment 298749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298749&action=review

r=me

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:854
> +localizedStrings["deg"] = "deg";

I don't think this should be localized. This is the unit right, like "px" is a unit.

> Source/WebInspectorUI/UserInterface/Views/GradientEditor.js:92
> +        angleContainerElement.append(WebInspector.UIString("deg"));

No UIString.

> Source/WebInspectorUI/UserInterface/Views/GradientEditor.js:203
> +            WebInspector.reportInternalError("Input event fired for disabled color component input");

Bad string. "disabled color component input" => "unexpected angle component input"

[Joseph Pecoraro]

Comment on attachment 298749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298749&action=review

> Source/WebInspectorUI/UserInterface/Views/GradientEditor.js:80
> +        this._angleSliderElement = angleContainerElement.createChild("input");

Nit: I don't really like the createChild approach. The element.appendChild(document.createElement(...)) approach is more straightforward to me.

> Source/WebInspectorUI/UserInterface/Views/GradientEditor.js:84
> +        this._angleSliderElement.addEventListener("input", this._angleChanged.bind(this));

Nit: This bound function happens twice. You could store the bound function in a local and only do it once.

[Devin Rousso]

Created attachment 299987 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 299987 [details]
Patch

Clearing flags on attachment: 299987

Committed r211318: <http://trac.webkit.org/changeset/211318>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.