Bug 192784

Summary: Web Inspector: Styles: shift-clicking a color-swatch to change formats starts editing the color
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, nvasilyev, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
URL: https://devinrousso.com/demo/WebKit/test.html
Attachments:
Description Flags
Patch
none
[Animated GIF] With patch applied none

Description Devin Rousso 2018-12-17 16:06:29 PST
# STEPS TO REPRODUCE:
1. inspect any page that uses a CSS color (e.g. via the `color` property)
2. shift-click the color swatch that appears next to the color string
 => format changes and the value enters editing mode
Comment 1 Devin Rousso 2018-12-17 16:07:12 PST
Views/SpreadsheetStyleProperty.js:663:23: CONSOLE ERROR Modified property was unlocked (color)
Models/CSSProperty.js:372:23: CONSOLE ERROR _styleSheetTextRange data is invalid.
Comment 2 Radar WebKit Bug Importer 2018-12-17 21:38:23 PST
<rdar://problem/46801028>
Comment 3 Nikita Vasilyev 2018-12-19 16:32:00 PST
Created attachment 357750 [details]
Patch
Comment 4 Nikita Vasilyev 2018-12-19 16:39:04 PST
Created attachment 357752 [details]
[Animated GIF] With patch applied
Comment 5 Devin Rousso 2018-12-19 16:41:03 PST
Comment on attachment 357750 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357750&action=review

rs=me, thanks for the quick fix!

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:484
> +            if (this._swatchActive || event.shiftKey)

Is there any reason to not prevent editing when clicking on the swatch?  Is this needed to make sure multiple-selection works with ⌘-click/⇧-click?  If that's the case, should this check instead just be that we aren't in "multiple-selection mode" or that "multiple-selection isn't active"?
Comment 6 Nikita Vasilyev 2018-12-19 17:06:05 PST
(In reply to Devin Rousso from comment #5)
> Comment on attachment 357750 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357750&action=review
> 
> rs=me, thanks for the quick fix!
> 
> > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:484
> > +            if (this._swatchActive || event.shiftKey)
> 
> Is there any reason to not prevent editing when clicking on the swatch?  Is
> this needed to make sure multiple-selection works with ⌘-click/⇧-click?  If
> that's the case, should this check instead just be that we aren't in
> "multiple-selection mode" or that "multiple-selection isn't active"?

Ideally, we should always prevent editing when clicking on the swatch.

If I replace it with:

    swatch.element.addEventListener("click", (event) => {
        event.stop();
    });

Inline swatches stop working after the first click!

This happens because _swatchElementClicked removes itself:

    if (this._boundSwatchElementClicked)
        this._swatchElement.removeEventListener("click", this._boundSwatchElementClicked);

and when this event listener gets added back (in didDismissPopover), it ends up *after* the `event.stop()` event listener.

I'd like to rewrite InlineSwatch so I don't need to call `event.stop` in SpreadsheetStyleProperty at all. I'm planning to do after the break, not today or tomorrow.
Comment 7 WebKit Commit Bot 2018-12-19 17:22:46 PST
Comment on attachment 357750 [details]
Patch

Clearing flags on attachment: 357750

Committed r239413: <https://trac.webkit.org/changeset/239413>
Comment 8 WebKit Commit Bot 2018-12-19 17:22:47 PST
All reviewed patches have been landed.  Closing bug.