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.
<rdar://problem/28553534>
Created attachment 290270 [details] Patch
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
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
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
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
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?
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.
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.
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.
Created attachment 290464 [details] Patch
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).
Created attachment 290479 [details] Patch
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
Created attachment 290480 [details] Patch
Comment on attachment 290480 [details] Patch Clearing flags on attachment: 290480 Committed r206729: <http://trac.webkit.org/changeset/206729>
All reviewed patches have been landed. Closing bug.