Bug 67610 – Web Inspector: disable popover when a mouse button is pressed

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

We should not display popover when a mouse button is pressed, as this conflicts with text selection.
We used to have use cases where popover was shown when a button is pressed -- this is now irrelevant for at least some of them, as the timeout for a popover is really small (100ms for network panel), others (timeline?) probably need the time reduced, having them display popover on mousedown would be inconsistent UX.

This also disables popover in the edit mode of SourceFrame, as the expression highlight conflicts with the TextView implementation of editor.

[Andrey Kosyakov]

Created attachment 106347 [details]
patch

[Pavel Feldman]

Comment on attachment 106347 [details]
patch

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

> Source/WebCore/inspector/front-end/Popover.js:205
> +        this._mouseButtonsState &= ~(1 << event.button);

I am still parsing it... What is happening?

> Source/WebCore/inspector/front-end/Popover.js:-227
> -    _handleMouseAction: function(event, isMouseDown)

I'd say we should keep Timeline's logic of immediately showing popover on click and not reduce its timeout.

[Andrey Kosyakov]

Created attachment 106409 [details]
patch

- use event.which
- retain popup by click by default

[Pavel Feldman]

Comment on attachment 106409 [details]
patch

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

Looks good with a couple of nits.

> Source/WebCore/inspector/front-end/SourceFrame.js:724
> +        if (!this._textViewer.readOnly) {

Do you need this? Will this method be called in case editing starts?

> Source/WebCore/inspector/front-end/SourceFrame.js:827
> +        if (this._popoverHelper)

This should probably go into _setReadOnly.

[Andrey Kosyakov]

(In reply to comment #4)
> (From update of attachment 106409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106409&action=review
> 
> Looks good with a couple of nits.
> 
> > Source/WebCore/inspector/front-end/SourceFrame.js:724
> > +        if (!this._textViewer.readOnly) {
> 
> Do you need this? Will this method be called in case editing starts?

Yes, this is actually the only place where we check that we're in edit mode (the one in doubleClick()/_setReadOnly() is only meant to protect from a race condition while the experssion is being evaluated). We could have made a check in shouldShowPopover(), but this would still leave a potential race condition if we introduce other methods to switch to the edit mode (keyboard shortcut?). 
> 
> > Source/WebCore/inspector/front-end/SourceFrame.js:827
> > +        if (this._popoverHelper)
> 
> This should probably go into _setReadOnly.

Done.

[Andrey Kosyakov]

Manually committed r94665: http://trac.webkit.org/changeset/94665