Summary: | Web Inspector: Gradient editor: opacity slider is too close to the right edge of the popover | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 205482, 205480 | ||||||||||||||
Attachments: |
|
We should use static layout (e.g. flexbox) instead of positioning everything absolute. The sliders are currently rotated 90 degrees by CSS transform to be vertical. This doesn't work well with flexbox; flexbox doesn't understand that width and height are now flipped. This is the only place where the sliders are used. We should convert WI.Slider to be vertical (and remove CSS transforms from ColorPicker.css). Created attachment 386164 [details]
Patch
Created attachment 386165 [details]
[Image] With patch applied
Comment on attachment 386164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386164&action=review > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:46 > +.color-picker .color-square, > +.color-picker .slider { `.color-picker :matches(.color-square, .slider)` > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:51 > + height: 200px; Can we just make this `height: 100%;`? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:52 > + margin-left: 10px; `-webkit-margin-start` > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:91 > + margin: 0; Why not just drop this then? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:98 > +.color-picker > .color-inputs > div:not([hidden]) + div { I think a more accurate selector would be `.color-picker > .color-inputs > div:nth-child(n + 2 of :not([hidden]))` > Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:99 > + margin-left: 4px; `-webkit-margin-start` > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:78 > + wrapper.className = "wrapper"; Why bother adding a class if it's not used? > Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:81 > + wrapper.append(this._colorSquare.element); > + wrapper.append(this._hueSlider.element); > + wrapper.append(this._opacitySlider.element); Please use `appendChild`. > Source/WebInspectorUI/UserInterface/Views/Slider.css:34 > + left: 2px; Do we need to do anything special for this for RTL? > Source/WebInspectorUI/UserInterface/Views/Slider.js:73 > + this._knob.style.webkitTransform = `translateY(${this._knobY}px) rotate(270deg)`; Rather than have to repeat the `rotate(270deg)`, why not set a `--translate-y` variable and use that in the CSS? ``` this._knob.style.webkitTransform = `--translate-y: ${this._knobY}px`; translate: translateY(var(--translate-y, 0)) rotate(270deg); ``` Comment on attachment 386164 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386164&action=review >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:51 >> + height: 200px; > > Can we just make this `height: 100%;`? This doesn't work, unfortunatelly. >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:52 >> + margin-left: 10px; > > `-webkit-margin-start` Thanks. >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:91 >> + margin: 0; > > Why not just drop this then? Oh, right :) >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:98 >> +.color-picker > .color-inputs > div:not([hidden]) + div { > > I think a more accurate selector would be `.color-picker > .color-inputs > div:nth-child(n + 2 of :not([hidden]))` I find it confusing that you have to write "+ 2" when you want to skip ONE (first) element. P.S. I'm planning to remove this selector as I'm planning to only render visible elements after Bug 203928: Web Inspector: Show RGBA input fields for p3 color picker. P.P.S. https://developer.mozilla.org/en-US/docs/Web/CSS/column-gap should be used for this specific case. In WebKit, it currently only works for CSS grids and doesn't work for flexbox. >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.css:99 >> + margin-left: 4px; > > `-webkit-margin-start` Thanks. >> Source/WebInspectorUI/UserInterface/Views/ColorPicker.js:78 >> + wrapper.className = "wrapper"; > > Why bother adding a class if it's not used? It's used in `.color-picker .wrapper` rule. >> Source/WebInspectorUI/UserInterface/Views/Slider.css:34 >> + left: 2px; > > Do we need to do anything special for this for RTL? I don't think it's particularly matters here. Created attachment 386185 [details]
Patch
Comment on attachment 386185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386185&action=review r=me, with one additional change > Source/WebInspectorUI/UserInterface/Views/Slider.css:-34 > - left: -5px; This doesn't look very good in RTL. The slider thumb is on the wrong side of the slider (it's on the right side in LTR and in RTL, whereas it should be on the left side in RTL). ``` body[dir=ltr] .slider > img { left: 2px; transform: translateY(var(--translate-y, 0px)) rotate(270deg); } body[dir=rtl] .slider > img { left: -6px; transform: translateY(var(--translate-y, 0px)) rotate(90deg); } ``` Created attachment 386282 [details]
Patch
Comment on attachment 386282 [details] Patch Clearing flags on attachment: 386282 Committed r253859: <https://trac.webkit.org/changeset/253859> All reviewed patches have been landed. Closing bug. |
Created attachment 382386 [details] [Image] Bug This is not a recent regression. It's been like this for over a year. The hue and the opacity sliders should move 10px to the left.