Summary: | Web Inspector: Layout sidebar grid overlay color swatch tooltip shouldn't include "switch format" hint | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick Angle <pangle> | ||||||||
Component: | Web Inspector | Assignee: | 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
Patrick Angle
2021-03-18 00:32:59 PDT
Created attachment 424896 [details]
Patch
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 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. Created attachment 424912 [details]
Patch
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 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 Created attachment 425010 [details]
Patch
Carry over R+; Update method name, fix style issue
Committed r275493: <https://commits.webkit.org/r275493> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425010 [details]. |