Bug 147913 - Web Inspector: DOM Node should have context menu to scroll it into view on the inspected page
Summary: Web Inspector: DOM Node should have context menu to scroll it into view on th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-11 16:40 PDT by Joseph Pecoraro
Modified: 2015-08-12 12:04 PDT (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (5.14 KB, patch)
2015-08-11 16:44 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-08-11 16:40:10 PDT
* SUMMARY
DOM Node should have context menu to scroll it into view on the inspected page

* NOTES
- Chrome and Firefox both have this.
Comment 1 Joseph Pecoraro 2015-08-11 16:44:57 PDT
Created attachment 258780 [details]
[PATCH] Proposed Fix
Comment 2 Devin Rousso 2015-08-11 17:33:19 PDT
Comment on attachment 258780 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1377
> +        var node = this.representedObject;

NIT: Aren't we using let now?

By the way, this doesn't apply to TOT.
Comment 3 Timothy Hatcher 2015-08-11 19:31:52 PDT
Comment on attachment 258780 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:672
> +            contextMenu.appendItem(WebInspector.UIString("Scroll Into View"), this._scrollIntoView.bind(this));

Should we only show this context menu item in the console and object trees? Showing it in the Elements tab seems silly.
Comment 4 Nikita Vasilyev 2015-08-11 20:24:18 PDT
(In reply to comment #3)
> Comment on attachment 258780 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258780&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:672
> > +            contextMenu.appendItem(WebInspector.UIString("Scroll Into View"), this._scrollIntoView.bind(this));
> 
> Should we only show this context menu item in the console and object trees?
> Showing it in the Elements tab seems silly.

I don't think it's silly. When I inspect CodeMirror I see lots of similar looking nodes — it's not always clear if the selected element is invisible or just out of the screen.
Comment 5 Timothy Hatcher 2015-08-11 20:45:56 PDT
Oh, it scrolls the page not the DOM tree. Not silly at all.
Comment 6 Timothy Hatcher 2015-08-11 20:47:58 PDT
Comment on attachment 258780 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:670
> +        var node = this.representedObject;

Let here too.
Comment 7 Joseph Pecoraro 2015-08-12 11:26:31 PDT
(In reply to comment #2)
> Comment on attachment 258780 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258780&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1377
> > +        var node = this.representedObject;
> 
> NIT: Aren't we using let now?
> 
> By the way, this doesn't apply to TOT.

This doesn't apply to ToT because of localizedString changes and svn-apply doesn't deal with binary diffs well.
Comment 8 Joseph Pecoraro 2015-08-12 12:04:03 PDT
http://trac.webkit.org/changeset/188343