Bug 127938 - Web Inspector: Inspect Element sometimes does not select the right DOM Node
Summary: Web Inspector: Inspect Element sometimes does not select the right DOM Node
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.9
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-30 12:56 PST by Recep ASLANTAS
Modified: 2014-05-19 14:20 PDT (History)
5 users (show)

See Also:


Attachments
Sample steps for the issue (7.38 MB, application/zip)
2014-01-30 13:31 PST, Recep ASLANTAS
no flags Details
[PATCH] Proposed Fix: Flag to allow _restoreSelectedNodeAfterUpdate (3.29 KB, patch)
2014-05-15 16:58 PDT, Jonathan Wells
timothy: review-
Details | Formatted Diff | Diff
[PaTCH] Proposed Fix: Flag to allow _restoreSelectedNodeAfterUpdate REV1 (3.57 KB, patch)
2014-05-16 16:36 PDT, Jonathan Wells
joepeck: review-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 2 (3.57 KB, patch)
2014-05-19 13:23 PDT, Jonathan Wells
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 3 (3.43 KB, patch)
2014-05-19 13:38 PDT, Jonathan Wells
joepeck: review-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 4 (3.75 KB, patch)
2014-05-19 13:48 PDT, Jonathan Wells
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Recep ASLANTAS 2014-01-30 12:56:32 PST
Hi,

Safari's web inspector is great tool for web development. But the web inspector of Safari.app does not work properly. 

Issue: 

Web inspector does not focusing on element which I selected when I showing web inspector with select text or with right-click (Inspect Element) on element such as input/span/div... Sometimes it work properly but sometimes does NOT. Sometimes I have to show web inspector and focus manually on HTML-Element with "Inspect Element" or "+ Inspect" button. Web inspector focused previous element that focused by web inspector previously when web inspector has showed. 

I'm still reading WebKit source codes (Xcode) for report significant bugs...

Sincerely...
Comment 1 Radar WebKit Bug Importer 2014-01-30 12:57:11 PST
<rdar://problem/15949845>
Comment 2 Recep ASLANTAS 2014-01-30 13:31:50 PST
Created attachment 222726 [details]
Sample steps for the issue
Comment 3 Jonathan Wells 2014-05-15 16:58:37 PDT
Created attachment 231542 [details]
[PATCH] Proposed Fix: Flag to allow _restoreSelectedNodeAfterUpdate
Comment 4 Timothy Hatcher 2014-05-16 09:37:38 PDT
Comment on attachment 231542 [details]
[PATCH] Proposed Fix: Flag to allow _restoreSelectedNodeAfterUpdate

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

Looks good. Only a minor issue.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:44
> +    this._restoreAllowed = true;

_restoreSelectedNodeAllowed?

> Source/WebInspectorUI/UserInterface/Views/FrameDOMTreeContentView.js:74
> +        if (WebInspector.domTreeManager._restoreAllowed)

This should not access a private property of another class. _restoreSelectedNodeAllowed should be exposed as a public getter or as isRestoreSelectedNodeAllowed().
Comment 5 Jonathan Wells 2014-05-16 14:06:02 PDT
Comment on attachment 231542 [details]
[PATCH] Proposed Fix: Flag to allow _restoreSelectedNodeAfterUpdate

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

>> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:44
>> +    this._restoreAllowed = true;
> 
> _restoreSelectedNodeAllowed?

Would it be:
_isRestoreSelectedNodeAllowed?

Or to make it more of a boolean statement and not a question:
_restoreSelectedNodeIsAllowed?
Comment 6 Jonathan Wells 2014-05-16 16:36:12 PDT
Created attachment 231603 [details]
[PaTCH] Proposed Fix: Flag to allow _restoreSelectedNodeAfterUpdate REV1
Comment 7 Jonathan Wells 2014-05-19 13:08:58 PDT
Bump!
Comment 8 Timothy Hatcher 2014-05-19 13:15:34 PDT
Comment on attachment 231603 [details]
[PaTCH] Proposed Fix: Flag to allow _restoreSelectedNodeAfterUpdate REV1

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

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:267
> +    get isRestoreSelectedNodeAllowed() {

Should be: isRestoreSelectedNodeAllowed: function() or get restoreSelectedNodeIsAllowed()
Comment 9 Joseph Pecoraro 2014-05-19 13:21:20 PDT
Comment on attachment 231603 [details]
[PaTCH] Proposed Fix: Flag to allow _restoreSelectedNodeAfterUpdate REV1

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

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:39
> +    this._isRestoreSelectedNodeAllowed = true;

How about, "_selectedNodeRestorationAllowed"? WebKit style is typically to avoid "is" on booleans.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:269
> +    get isRestoreSelectedNodeAllowed() {
> +        return this._isRestoreSelectedNodeAllowed;
> +    },

Style: "{" on newline.

> Source/WebInspectorUI/UserInterface/Views/FrameDOMTreeContentView.js:75
> +        if (WebInspector.domTreeManager.isRestoreSelectedNodeAllowed)
> +            this._restoreSelectedNodeAfterUpdate(this._domTree.frame.url, rootDOMNode.body || rootDOMNode.documentElement);

This seems like the wrong point to check the boolean state.

    - _restoreSelectedNodeAfterUpdate is called in multiple places. Do we want to respect this state in all places it is called?
    - _restoreSelectedNodeAfterUpdate may be called when this state is true, but run its async operation when this state is false, it should check this state again before actually changing the selected node.

It seems like inside _restoreSelectedNodeAfterUpdate is where we should be checking this "is restoration allowed" state, so we never do a restoration when we are not allowed.
Comment 10 Jonathan Wells 2014-05-19 13:23:55 PDT
Created attachment 231716 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 2
Comment 11 Joseph Pecoraro 2014-05-19 13:30:34 PDT
Comment on attachment 231716 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 2

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

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:267
> +    get restoreSelectedNodeIsAllowed() {

Still has this style issue.

> Source/WebInspectorUI/UserInterface/Views/FrameDOMTreeContentView.js:75
> +        if (WebInspector.domTreeManager.restoreSelectedNodeIsAllowed)
> +            this._restoreSelectedNodeAfterUpdate(this._domTree.frame.url, rootDOMNode.body || rootDOMNode.documentElement);

Still has the issue of async restoration.
Comment 12 Jonathan Wells 2014-05-19 13:38:57 PDT
Created attachment 231718 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 3
Comment 13 Joseph Pecoraro 2014-05-19 13:41:42 PDT
Comment on attachment 231718 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 3

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

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:275
> +        this._restoreSelectedNodeIsAllowed = true;
>          var node = this._idToDOMNode[nodeId];

Style: lets put an empty newline after setting the state.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:285
> +        this._restoreSelectedNodeIsAllowed = false;

Style: lets put an empty newline after setting the state.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:297
> +        if (!WebInspector.domTreeManager.restoreSelectedNodeIsAllowed)
> +            return;

The same check should be done in selectLastSelectedNode below, which is what can be called async, and we don't want to restore the selected node in the async case.
Comment 14 Jonathan Wells 2014-05-19 13:48:00 PDT
Created attachment 231720 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 4
Comment 15 Joseph Pecoraro 2014-05-19 13:49:03 PDT
Comment on attachment 231720 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 4

Now this patch sparkles. r=me
Comment 16 WebKit Commit Bot 2014-05-19 14:20:19 PDT
Comment on attachment 231720 [details]
[PATCH] Proposed Fix: Addition of NavigationSidebar#isElementSelected getter REV 4

Clearing flags on attachment: 231720

Committed r169067: <http://trac.webkit.org/changeset/169067>
Comment 17 WebKit Commit Bot 2014-05-19 14:20:23 PDT
All reviewed patches have been landed.  Closing bug.