Bug 97120 – Web Inspector: yellow on-hover pop-up won't go if another pane asynchronously opens

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

[Peter Rybin]

General steps:
- in Sources tab provoke yellow inspect pop-up window by hovering over some variable in text
- in previous step learn the timing for hover to trigger evaluation
- manage to provoke inspect evaluation, but open ||> drawer (sources list) before the pop-up actually appears
- yellow pop-up appears above ||> drawer

Expected:
pointing away from pop-up makes it go

Actual:
the pop-up remains fixed as long as you move mouse inside the opened ||> drawer. User have to learn that not only he has to point away from the pop-up, but also point to the original half-visible pane of source text. This is pretty much unintuitive.

Hints for reproducing
Enable pause on exceptions. Open http://design.ru Execution will stop on widgets.js with TypeError. A "window" variable is references in left-top corner of the text, which is handy for starting heavy evaluate and making it to open ||>

[Mirela]

I will try looking into this bug.

[Mirela]

Created attachment 165388 [details]
Add mouseout event listener on popover element.

[Alexander Pavlov (apavlov)]

Comment on attachment 165388 [details]
Add mouseout event listener on popover element.

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

> Source/WebCore/ChangeLog:3
> +        Fix for bug 97210

Please remove this auto-added line

> Source/WebCore/inspector/front-end/Popover.js:242
> +        if (event.target === this._hoverElement || 

WebKit has no limit for the line length, so the entire condition can be placed on a single line. And we definitely do not observe closing parentheses on a separate line.

> Source/WebCore/inspector/front-end/Popover.js:243
> +            (this.isPopoverVisible() && !event.toElement.isSelfOrDescendant(this._popover._contentDiv))

IIRC, event.toElement can be null/undefined if you are moving the pointer outside the browser window quickly (which is confirmed by http://trac.webkit.org/browser/trunk/Source/WebCore/dom/MouseEvent.cpp#L137). Also, "toElement" is an MSIE extension. The spec'ed way is "event.relatedTarget".

[Mirela]

Created attachment 165556 [details]
Update the patch based on the review.

[Alexander Pavlov (apavlov)]

Comment on attachment 165556 [details]
Update the patch based on the review.

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

> Source/WebCore/ChangeLog:3
> +        Fix for bug 97210

Hmm, please remove this auto-generated line...

> Source/WebCore/inspector/front-end/Popover.js:242
> +        if (event.target === this._hoverElement || (this.isPopoverVisible() && !event.relatedTarget.isSelfOrDescendant(this._popover._contentDiv)))

Please check event.relatedTarget for null-ity to avoid throwing a JS exception.

[Mirela]

Created attachment 165619 [details]
New patch update

[Alexander Pavlov (apavlov)]

Comment on attachment 165619 [details]
New patch update

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

Two small nits to fix, and the patch will be ready to land

> Source/WebCore/inspector/front-end/Popover.js:244
> +            && event.relatedTarget !== null

WebKit coding style guidelines require that null/0/undefined/whatever checks be performed by casting to a bool (e.g. ... && event.relatedTarget && ...) unless we need to disambiguate the actual value (e.g. null vs. undefined vs. "" vs. 0) - see http://www.webkit.org/coding/coding-style.html#zero-comparison

> Source/WebCore/inspector/front-end/Popover.js:246
> +        if (event.target === this._hoverElement || isPopoverMouseOut )

Extraneous whitespace before ')'

[Mirela]

Created attachment 165621 [details]
Address review comments

[Alexander Pavlov (apavlov)]

Comment on attachment 165621 [details]
Address review comments

r=me

[WebKit Review Bot]

Comment on attachment 165621 [details]
Address review comments

Clearing flags on attachment: 165621

Committed r129516: <http://trac.webkit.org/changeset/129516>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.