Bug 162759 – Web Inspector: Clicking twice on the color swatch square should hide the color picker

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 290256 [details]
[Image] Bug

Steps:
1. Add "background: white" inline style.
2. Click on the color swatch square before "white".
3. Click there again.

Actual:
The color picker hides and shows immediately again, which isn't particularly useful.

Expected:
The color picker hides. This is how it works in Chrome DevTools and Firefox DevTools.

[Radar WebKit Bug Importer]

<rdar://problem/28553534>

[Devin Rousso]

Created attachment 290270 [details]
Patch

[Build Bot]

Comment on attachment 290270 [details]
Patch

Attachment 290270 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2171636

New failing tests:
transitions/default-timing-function.html

[Build Bot]

Created attachment 290275 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 290270 [details]
Patch

Attachment 290270 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2171684

New failing tests:
fast/images/pixel-crack-image-background-webkit-transform-scale.html

[Build Bot]

Created attachment 290280 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Nikita Vasilyev]

Comment on attachment 290270 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:65
> +        this._ignoreNextClick = false;

_isColorPickerActive or _isPopoverVisible.

> Source/WebInspectorUI/UserInterface/Views/Popover.js:167
> +                    this.delegate.beforeDismissPopover(event);

We already have willDismissPopover and didDismissPopover. Why don't use those?

[Devin Rousso]

Comment on attachment 290270 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:65
>> +        this._ignoreNextClick = false;
> 
> _isColorPickerActive or _isPopoverVisible.

Good call.

>> Source/WebInspectorUI/UserInterface/Views/Popover.js:167
>> +                    this.delegate.beforeDismissPopover(event);
> 
> We already have willDismissPopover and didDismissPopover. Why don't use those?

The simplest way I found of fixing this issue was to compare the targets of the `click` event for the popover and the `click` event of the swatch.  I was thinking that didDismissPopover wouldn't work because it doesn't (currently) have access to the event that caused the dismiss to happen (this defaults to `mousedown` and `scroll`).  It would be possible to use willDismissPopover, but it would require adding an `event` parameter to WI.Popover.prototype.dismiss that would let us pass in the event object, and that doesn't really seem like the right approach.

[Nikita Vasilyev]

Comment on attachment 290270 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:138
> +        }

It should go after...

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:142
>          if (this._type === WebInspector.InlineSwatch.Type.Color && event.shiftKey && this._value) {

this if block. Shift-Click should switch color format regardless of popover visibility. r- because of that.

>>> Source/WebInspectorUI/UserInterface/Views/Popover.js:167
>>> +                    this.delegate.beforeDismissPopover(event);
>> 
>> We already have willDismissPopover and didDismissPopover. Why don't use those?
> 
> The simplest way I found of fixing this issue was to compare the targets of the `click` event for the popover and the `click` event of the swatch.  I was thinking that didDismissPopover wouldn't work because it doesn't (currently) have access to the event that caused the dismiss to happen (this defaults to `mousedown` and `scroll`).  It would be possible to use willDismissPopover, but it would require adding an `event` parameter to WI.Popover.prototype.dismiss that would let us pass in the event object, and that doesn't really seem like the right approach.

I see. This seems a bit overly complicated but I don't know how to make it better.

[Matt Baker]

If the popover delegate had a didPresentPover method, InlineSwatch could remove the swatch element event listener when the popover is shown, and add it in didDismissPopover.

[Devin Rousso]

Created attachment 290464 [details]
Patch

[Matt Baker]

Comment on attachment 290464 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/InlineSwatch.js:88
> +    // Delegate functions for Popover (Protected)

This comment should just be "// Popover delegate", to match the other delegate section comments (there are only 24, but they are consistently named).

[Devin Rousso]

Created attachment 290479 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290479 [details]
Patch

Rejecting attachment 290479 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 290479, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebInspectorUI/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2206111

[Devin Rousso]

Created attachment 290480 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290480 [details]
Patch

Clearing flags on attachment: 290480

Committed r206729: <http://trac.webkit.org/changeset/206729>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.