Bug 192784 – Web Inspector: Styles: shift-clicking a color-swatch to change formats starts editing the color

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

# 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

[Devin Rousso]

Views/SpreadsheetStyleProperty.js:663:23: CONSOLE ERROR Modified property was unlocked (color)
Models/CSSProperty.js:372:23: CONSOLE ERROR _styleSheetTextRange data is invalid.

[Radar WebKit Bug Importer]

<rdar://problem/46801028>

[Nikita Vasilyev]

Created attachment 357750 [details]
Patch

[Nikita Vasilyev]

Created attachment 357752 [details]
[Animated GIF] With patch applied

[Devin Rousso]

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"?

[Nikita Vasilyev]

(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.

[WebKit Commit Bot]

Comment on attachment 357750 [details]
Patch

Clearing flags on attachment: 357750

Committed r239413: <https://trac.webkit.org/changeset/239413>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.