Bug 67483 - Web Inspector: Console is always scrolled to the top
Summary: Web Inspector: Console is always scrolled to the top
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: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-02 06:33 PDT by Yury Semikhatsky
Modified: 2011-09-22 06:42 PDT (History)
11 users (show)

See Also:


Attachments
Patch (2.98 KB, patch)
2011-09-02 06:40 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (9.81 KB, patch)
2011-09-02 11:37 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (7.60 KB, patch)
2011-09-20 11:31 PDT, Vsevolod Vlasov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2011-09-02 06:33:41 PDT
Ustreaming Chromium bug: http://code.google.com/p/chromium/issues/detail?id=86832

What steps will reproduce the problem?
1. Open JS console.
2. Scroll to the bottom.
3. Go to the script tab.
4. Go back to the console.

What is the expected result?

The console should by default be scrolled to the bottom. That's where the newest information is and where new statements can be entered.


What happens instead?

Console always scrolls back to the top.
Comment 1 Yury Semikhatsky 2011-09-02 06:40:26 PDT
Created attachment 106126 [details]
Patch
Comment 2 Vsevolod Vlasov 2011-09-02 11:16:18 PDT
This does not look nice. Scroll changes when switching between console panel and console drawer are not smooth at all.

I suggest that we have the following behavior of scroll in console:

1) If we are not at the bottom of console, then store/restore scrollTop for possible each action.
2) If we are at the bottom of console, then keep this state until user scrolls explicitly.

I have a patch implementing this behavior ready and will upload it soon.
Comment 3 Vsevolod Vlasov 2011-09-02 11:37:26 PDT
Created attachment 106166 [details]
Patch
Comment 4 Pavel Feldman 2011-09-04 22:52:46 PDT
Comment on attachment 106166 [details]
Patch

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

> Source/WebCore/inspector/front-end/ConsoleView.js:227
> +        WebInspector.View.prototype.show.call(this);

I remember caseq implementing View hierarchy that was moving the logic from show and hide to wasShown and willHide. Is this change in line with what he was doing?

> Source/WebCore/inspector/front-end/ConsoleView.js:254
> +        if (this._scrolledToBottom)

You might want to just override the original logic here so that you don't use this hybrid approach.

> Source/WebCore/inspector/front-end/ConsoleView.js:262
> +        this.restoreScrollPositions();

this._scrolledToBottom value might be out of date by this moment (user scrolled manually).
Comment 5 Yury Semikhatsky 2011-09-05 01:46:40 PDT
Comment on attachment 106166 [details]
Patch

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

> Source/WebCore/inspector/front-end/ConsoleView.js:255
> +            this._immediatelyScrollIntoView();

The console may have been cleared after the position was stored, in this case it doesn't make sense to restore scroller position if it's not bottom/top.
Comment 6 Vsevolod Vlasov 2011-09-20 11:31:59 PDT
Created attachment 108033 [details]
Patch
Comment 7 Vsevolod Vlasov 2011-09-20 11:47:27 PDT
> I remember caseq implementing View hierarchy that was moving the logic from show and hide to wasShown and willHide. Is this change in line with what he was doing?
New logic is added to wasShown now.
> 
> > Source/WebCore/inspector/front-end/ConsoleView.js:254
> > +        if (this._scrolledToBottom)
> 
> You might want to just override the original logic here so that you don't use this hybrid approach.
I don't understand your idea. Essentially console reuses typical store/restore scroll approach with an exception of scrolled-to-bottom case.
> 
> > Source/WebCore/inspector/front-end/ConsoleView.js:262
> > +        this.restoreScrollPositions();
> 
> this._scrolledToBottom value might be out of date by this moment (user scrolled manually).
The idea is that this._scrolledToBottom is updated before hide/resize starts, see _startStatusBarDragging() in Drawer.

(In reply to comment #5)
> (From update of attachment 106166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106166&action=review
> 
> > Source/WebCore/inspector/front-end/ConsoleView.js:255
> > +            this._immediatelyScrollIntoView();
> 
> The console may have been cleared after the position was stored, in this case it doesn't make sense to restore scroller position if it's not bottom/top.
Added call to storeScrollPositions in ConsoleCleared event handler.
Comment 8 Pavel Feldman 2011-09-22 04:47:24 PDT
Comment on attachment 108033 [details]
Patch

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

> Source/WebCore/inspector/front-end/ConsoleView.js:229
> +        WebInspector.View.prototype.wasShown.call(this);

This is a View's flaw. wasShown should be abstract.

> Source/WebCore/inspector/front-end/ConsoleView.js:284
> +        if (!this._scrollIntoViewTimer)

if (!this._isScrollIntoViewScheduled())
Comment 9 Vsevolod Vlasov 2011-09-22 06:42:36 PDT
Committed r95716: <http://trac.webkit.org/changeset/95716>