Bug 60630 - Web Inspector: hovering over linkified nodes in the UI does not highlight elements on the page.
Summary: Web Inspector: hovering over linkified nodes in the UI does not highlight ele...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-11 08:50 PDT by Pavel Feldman
Modified: 2011-06-21 01:43 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.54 KB, patch)
2011-05-11 08:55 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-05-11 08:50:32 PDT
Patch to follow.
Comment 1 Pavel Feldman 2011-05-11 08:55:12 PDT
Created attachment 93125 [details]
Patch
Comment 2 Yury Semikhatsky 2011-05-11 11:56:37 PDT
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?
Comment 3 Alexander Pavlov (apavlov) 2011-05-11 12:24:16 PDT
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.
Comment 4 Yury Semikhatsky 2011-05-11 20:38:49 PDT
(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.
Comment 5 Yury Semikhatsky 2011-05-11 21:47:40 PDT
Comment on attachment 93125 [details]
Patch

r+ since it's a regression. The method should be split in a separate change.
Comment 6 Pavel Feldman 2011-06-18 02:27:31 PDT
I just checken on a canary and it does not seem to work. Reopening. I'll rebase and land.
Comment 7 Pavel Feldman 2011-06-21 01:43:43 PDT
Comment on attachment 93125 [details]
Patch

Clearing flags on attachment: 93125

Committed r89342: <http://trac.webkit.org/changeset/89342>
Comment 8 Pavel Feldman 2011-06-21 01:43:52 PDT
All reviewed patches have been landed.  Closing bug.