RESOLVED FIXED 223418
Web Inspector: Layout sidebar grid overlay color swatch tooltip shouldn't include "switch format" hint
https://bugs.webkit.org/show_bug.cgi?id=223418
Summary Web Inspector: Layout sidebar grid overlay color swatch tooltip shouldn't inc...
Patrick Angle
Reported 2021-03-18 00:32:59 PDT
Steps to reproduce: 1. Inspect a page with a CSS grid 2. Hover over a grid overlay swatch in the Layout sidebar Results: Tooltip will read: ``` Click to select a color Shift-click to switch color formats ``` Expected result: Tooltip should read: ``` Click to select a color ``` and not reference switching formats, which is not applicable in this context.
Attachments
Patch (5.18 KB, patch)
2021-04-01 08:05 PDT, Razvan Caliman
no flags
Patch (3.01 KB, patch)
2021-04-01 10:59 PDT, Razvan Caliman
no flags
Patch (4.11 KB, patch)
2021-04-02 05:25 PDT, Razvan Caliman
no flags
Radar WebKit Bug Importer
Comment 1 2021-03-25 00:33:14 PDT
Razvan Caliman
Comment 2 2021-04-01 08:05:14 PDT
Patrick Angle
Comment 3 2021-04-01 09:15:30 PDT
Comment on attachment 424896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424896&action=review > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:184 > if (this._allowShiftClickColor()) Just noticing there is a check if shift-clicking should even be allowed, and if it is false we would get the string we wanted. Maybe instead of passing a title into the swatch we should support directly enabling/disabling shift-click, since _swatchElementClicked also has a check for shift-clicking being allowed?
Blaze Burg
Comment 4 2021-04-01 09:54:32 PDT
Comment on attachment 424896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424896&action=review >> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:184 >> if (this._allowShiftClickColor()) > > Just noticing there is a check if shift-clicking should even be allowed, and if it is false we would get the string we wanted. Maybe instead of passing a title into the swatch we should support directly enabling/disabling shift-click, since _swatchElementClicked also has a check for shift-clicking being allowed? +1, it makes more sense to me, to have the swatch object itself be readonly.
Razvan Caliman
Comment 5 2021-04-01 10:59:00 PDT
Blaze Burg
Comment 6 2021-04-01 11:21:05 PDT
Comment on attachment 424912 [details] Patch r=me The logic looks fine to me. I am not fond of the existing _allowShiftClickColor name. Something more like allowsChangingColorFormats would be more about the semantics of what is possible or not, rather than how it is currently exposed.
Patrick Angle
Comment 7 2021-04-01 11:23:22 PDT
Comment on attachment 424912 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424912&action=review > Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:128 > + set shiftClickColorEnabled(value) { NIT: `{` on next line
Razvan Caliman
Comment 8 2021-04-02 05:25:11 PDT
Created attachment 425010 [details] Patch Carry over R+; Update method name, fix style issue
EWS
Comment 9 2021-04-06 00:28:35 PDT
Committed r275493: <https://commits.webkit.org/r275493> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425010 [details].
Note You need to log in before you can comment on or make changes to this bug.