Bug 60630 – Web Inspector: hovering over linkified nodes in the UI does not highlight elements on the page.

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

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 93125 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 93125 [details]
Patch

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

> Source/WebCore/inspector/front-end/inspector.js:322
> +        element.addEventListener("mouseover", this.highlightDOMNode.bind(this, nodeId, "all"), false);

How does it differ from the current behavior? Looking at inspector.js:308 I see this:
DOMAgent.highlightNode(nodeId, mode || "all"); which is equivalent to passing "all" explicitely. Should we require mode parameter?

[Alexander Pavlov (apavlov)]

Comment on attachment 93125 [details]
Patch

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

>> Source/WebCore/inspector/front-end/inspector.js:322
>> +        element.addEventListener("mouseover", this.highlightDOMNode.bind(this, nodeId, "all"), false);
> 
> How does it differ from the current behavior? Looking at inspector.js:308 I see this:
> DOMAgent.highlightNode(nodeId, mode || "all"); which is equivalent to passing "all" explicitely. Should we require mode parameter?

In the case of a bound event handler, the mouse event becomes the second parameter which is far from "all". We should doubtfully require the second parameter, as highlightDOMNode(0) results in hiding the highlight (and having the second "mode" parameter is awkward in this case.) Incidentally, I came up with the same solution today while rewriting the StylesSidebarPane.

[Yury Semikhatsky]

(In reply to comment #3)
> (From update of attachment 93125 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93125&action=review
> 
> >> Source/WebCore/inspector/front-end/inspector.js:322
> >> +        element.addEventListener("mouseover", this.highlightDOMNode.bind(this, nodeId, "all"), false);
> > 
> > How does it differ from the current behavior? Looking at inspector.js:308 I see this:
> > DOMAgent.highlightNode(nodeId, mode || "all"); which is equivalent to passing "all" explicitely. Should we require mode parameter?
> 
> In the case of a bound event handler, the mouse event becomes the second parameter which is far from "all". We should doubtfully require the second parameter, as highlightDOMNode(0) results in hiding the highlight (and having the second "mode" parameter is awkward in this case.) 

Sounds like the method needs to be split into highlightXXX and hideHighlightXX.

[Yury Semikhatsky]

Comment on attachment 93125 [details]
Patch

r+ since it's a regression. The method should be split in a separate change.

[Pavel Feldman]

I just checken on a canary and it does not seem to work. Reopening. I'll rebase and land.

[Pavel Feldman]

Comment on attachment 93125 [details]
Patch

Clearing flags on attachment: 93125

Committed r89342: <http://trac.webkit.org/changeset/89342>

[Pavel Feldman]

All reviewed patches have been landed.  Closing bug.