Bug 89554

Summary: Web Inspector: Having a "Scroll into view" for nodes through web inspector.
Product: WebKit Reporter: sam <dsam2912>
Component: Web Inspector (Deprecated)Assignee: sam <dsam2912>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, vivekgalatage, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
screenshot illustrating scroll into view option in menu.
none
Patch
none
Patch
none
Patch
none
Patch none

Description sam 2012-06-20 01:55:26 PDT
Web inspector currently does not have an option for getting the node to scroll into view.
Comment 1 sam 2012-06-20 01:58:08 PDT
I am looking into this and will upload the patch soon.
Also attached is the screen shot for illustration purpose.
Plz do let me know if there are any suggestions/comments.
Comment 2 sam 2012-06-20 01:59:07 PDT
Created attachment 148525 [details]
screenshot illustrating scroll into view option in menu.
Comment 3 Yury Semikhatsky 2012-06-20 02:11:18 PDT
This can be achieved by evaluating "$0.scrollIntoViewIfNeeded(true)" in the console. I'm not sure if it's worth a context menu action.
Comment 4 Pavel Feldman 2012-06-20 07:27:36 PDT
I like the context menu part, it does not clutter UI, user does not need to be aware of console API. I would put it lower though, so that it was below all edit / copy operations.
Comment 5 sam 2012-06-22 03:36:15 PDT
Thanks Yury, Pavel for your feedbacks.

I was just wondering if including a new api to execute expressions through console will be ok. This would basically a very tiny one - work similar to executeThroughConsoleUsingTextPrompt but will neither set record  nor print result and will also not show console panel - only direct execution.
I guess it might be used in later use cases also.

Or is there something else also which I might be missing/getting wrong?
Comment 6 Yury Semikhatsky 2012-06-22 04:27:09 PDT
(In reply to comment #5)
> Thanks Yury, Pavel for your feedbacks.
> 
> I was just wondering if including a new api to execute expressions through console will be ok. This would basically a very tiny one - work similar to executeThroughConsoleUsingTextPrompt but will neither set record  nor print result and will also not show console panel - only direct execution.
> I guess it might be used in later use cases also.
> 
> Or is there something else also which I might be missing/getting wrong?

What do you mean by "including a new api"? We already have RuntimeAgent.evaluate(...) method on the front-end that allows you to perform evaluation in the inspected page, current console UI invokes that method, isn't it enough for your needs?
Comment 7 sam 2012-06-22 06:03:31 PDT
Created attachment 149009 [details]
Patch
Comment 8 sam 2012-06-22 06:09:20 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Thanks Yury, Pavel for your feedbacks.
> > 
> > I was just wondering if including a new api to execute expressions through console will be ok. This would basically a very tiny one - work similar to executeThroughConsoleUsingTextPrompt but will neither set record  nor print result and will also not show console panel - only direct execution.
> > I guess it might be used in later use cases also.
> > 
> > Or is there something else also which I might be missing/getting wrong?
> 
> What do you mean by "including a new api"? We already have RuntimeAgent.evaluate(...) method on the front-end that allows you to perform evaluation in the inspected page, current console UI invokes that method, isn't it enough for your needs?

Ya true, i just meant to include a wrapper like executeThroughConsoleUsingTextPrompt, but later realized that eval can be done by direct call.
Thank you Yury.
Comment 9 Yury Semikhatsky 2012-06-22 06:15:15 PDT
Comment on attachment 149009 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1073
> +            var expression = "$0.scrollIntoView(true)";

Note that $0 may not necessarily be a DOM node, e.g. it may be last element selected in a heap profile.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1074
> +            WebInspector.consoleView.evalInInspectedWindow(expression, "console", true, false, false, null);

Instead of going through the consoleView, you should be able to get current selected node and evaluate .scrollIntoView(true) on it using RuntimeAgent.evaluate
Comment 10 Pavel Feldman 2012-06-22 07:06:42 PDT
Comment on attachment 149009 [details]
Patch

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

>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1074
>> +            WebInspector.consoleView.evalInInspectedWindow(expression, "console", true, false, false, null);
> 
> Instead of going through the consoleView, you should be able to get current selected node and evaluate .scrollIntoView(true) on it using RuntimeAgent.evaluate

Here is the hint on how to do what Yury suggests:

WebInspector.RemoteObject.resolveNode(nodeId, "", callback);

function callback(object)
{
    function scrollIntoView()
    {
        this.scrollIntoView(true);
    }

    if (object)
        object.callFunction(scrollIntoView, []);
}
Comment 11 sam 2012-07-04 22:33:34 PDT
Created attachment 150877 [details]
Patch
Comment 12 Pavel Feldman 2012-07-04 23:49:12 PDT
Comment on attachment 150877 [details]
Patch

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

Looks good. Couple of nits and it is ready to land.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1920
> +    scrollIntoView: function()

Should be private (_scrollIntoView)

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1930
> +                object.callFunction(scrollIntoView, undefined);

No need to pass undefined, just declare this parameter as optional.

> Source/WebCore/inspector/front-end/RemoteObject.js:287
> +            if (!callback)

You need to declare callback optional in the jsdoc.
Comment 13 sam 2012-07-05 02:20:37 PDT
Created attachment 150899 [details]
Patch
Comment 14 Pavel Feldman 2012-07-05 02:23:38 PDT
Comment on attachment 150899 [details]
Patch

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

> Source/WebCore/inspector/front-end/RemoteObject.js:275
> +     * @param {(Array.<RuntimeAgent.CallArgument>|undefined)=} args

no need for "|undefined" now that this parameter is optional.
Comment 15 sam 2012-07-05 02:31:29 PDT
Created attachment 150903 [details]
Patch
Comment 16 sam 2012-07-05 02:31:58 PDT
(In reply to comment #14)
> (From update of attachment 150899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150899&action=review
> 
> > Source/WebCore/inspector/front-end/RemoteObject.js:275
> > +     * @param {(Array.<RuntimeAgent.CallArgument>|undefined)=} args
> 
> no need for "|undefined" now that this parameter is optional.

err, will upload it again. Thanks for the review Pavel.
Comment 17 Pavel Feldman 2012-07-05 02:53:03 PDT
Comment on attachment 150903 [details]
Patch

Thanks!
Comment 18 WebKit Review Bot 2012-07-05 03:34:09 PDT
Comment on attachment 150903 [details]
Patch

Clearing flags on attachment: 150903

Committed r121896: <http://trac.webkit.org/changeset/121896>
Comment 19 WebKit Review Bot 2012-07-05 03:34:15 PDT
All reviewed patches have been landed.  Closing bug.