Bug 223418

Summary: Web Inspector: Layout sidebar grid overlay color swatch tooltip shouldn't include "switch format" hint
Product: WebKit Reporter: Patrick Angle <pangle>
Component: Web InspectorAssignee: Razvan Caliman <rcaliman>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, ews-watchlist, hi, inspector-bugzilla-changes, rcaliman, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Patrick Angle 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.
Comment 1 Radar WebKit Bug Importer 2021-03-25 00:33:14 PDT
<rdar://problem/75825793>
Comment 2 Razvan Caliman 2021-04-01 08:05:14 PDT
Created attachment 424896 [details]
Patch
Comment 3 Patrick Angle 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?
Comment 4 BJ Burg 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.
Comment 5 Razvan Caliman 2021-04-01 10:59:00 PDT
Created attachment 424912 [details]
Patch
Comment 6 BJ Burg 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.
Comment 7 Patrick Angle 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
Comment 8 Razvan Caliman 2021-04-02 05:25:11 PDT
Created attachment 425010 [details]
Patch

Carry over R+; Update method name, fix style issue
Comment 9 EWS 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].