Bug 166937 - Web Inspector: gradient editor should provide horizontal slider for 'angle' value where applicable
Summary: Web Inspector: gradient editor should provide horizontal slider for 'angle' v...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 166933
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-11 12:02 PST by Brian Burg
Modified: 2017-01-27 18:29 PST (History)
7 users (show)

See Also:


Attachments
Patch (9.03 KB, patch)
2017-01-11 19:41 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (49.49 KB, image/png)
2017-01-11 19:42 PST, Devin Rousso
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.15 MB, application/zip)
2017-01-12 06:11 PST, Build Bot
no flags Details
Patch (8.96 KB, patch)
2017-01-12 09:36 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (9.40 KB, patch)
2017-01-12 20:53 PST, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
[Image] After Patch is applied (39.95 KB, image/png)
2017-01-12 20:53 PST, Devin Rousso
no flags Details
Patch (8.60 KB, patch)
2017-01-27 17:51 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2017-01-11 12:02:35 PST
This is for linear and linear-radial gradients.
Comment 1 Brian Burg 2017-01-11 12:03:53 PST
Also, the label should be aligned left and have a trailing colon.


I think this is a more standard layout:

Angle: [  ]  |------o---|
Comment 2 Radar WebKit Bug Importer 2017-01-11 12:09:16 PST
<rdar://problem/29974201>
Comment 3 Devin Rousso 2017-01-11 19:41:16 PST
Created attachment 298650 [details]
Patch
Comment 4 Devin Rousso 2017-01-11 19:42:07 PST
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.
Comment 5 Devin Rousso 2017-01-11 19:45:30 PST
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.
Comment 6 Build Bot 2017-01-12 06:11:25 PST
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
Comment 7 Build Bot 2017-01-12 06:11:28 PST
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
Comment 8 Devin Rousso 2017-01-12 09:36:41 PST
Created attachment 298691 [details]
Patch
Comment 9 Brian Burg 2017-01-12 10:03:54 PST
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.
Comment 10 Devin Rousso 2017-01-12 12:07:43 PST
(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.
Comment 11 Brian Burg 2017-01-12 13:17:44 PST
(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.
Comment 12 Devin Rousso 2017-01-12 20:53:04 PST
Created attachment 298749 [details]
Patch
Comment 13 Devin Rousso 2017-01-12 20:53:25 PST
Created attachment 298750 [details]
[Image] After Patch is applied
Comment 14 Joseph Pecoraro 2017-01-27 16:32:47 PST
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"
Comment 15 Joseph Pecoraro 2017-01-27 16:34:05 PST
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.
Comment 16 Devin Rousso 2017-01-27 17:51:39 PST
Created attachment 299987 [details]
Patch
Comment 17 WebKit Commit Bot 2017-01-27 18:29:16 PST
Comment on attachment 299987 [details]
Patch

Clearing flags on attachment: 299987

Committed r211318: <http://trac.webkit.org/changeset/211318>
Comment 18 WebKit Commit Bot 2017-01-27 18:29:22 PST
All reviewed patches have been landed.  Closing bug.