This is for linear and linear-radial gradients.
Also, the label should be aligned left and have a trailing colon. I think this is a more standard layout: Angle: [ ] |------o---|
<rdar://problem/29974201>
Created attachment 298650 [details] Patch
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.
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 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
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
Created attachment 298691 [details] Patch
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.
(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.
(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.
Created attachment 298749 [details] Patch
Created attachment 298750 [details] [Image] After Patch is applied
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 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.
Created attachment 299987 [details] Patch
Comment on attachment 299987 [details] Patch Clearing flags on attachment: 299987 Committed r211318: <http://trac.webkit.org/changeset/211318>
All reviewed patches have been landed. Closing bug.