1367266 - Console scroll position no longer follows console tail as new commands are entered

Status: VERIFIED FIXED

(DevTools :: Console, defect, P1)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Open devtools console in Nightly 55.
2. Enter some random characters at the console and press ENTER.
3. Repeat step 2 until there is enough backlog to cause the console scroll bar to appear.

RESULT:
As you continue to enter more console commands, the scroll position does not follow the log tail with the latest commands. The scroll bar sits at the top of the backlog.

I bisected this recent regression to the following pushlog:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f9ca97a334296facd2e0ea5582e7f12d0fe70fe4&tochange=d712c82c59ec5a277047a75d09bec48be4a64b87

@ Honza: is this a regression from console message limit bug 1307884?

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1367266 - Properly calculate autoscroll prop;

Review commit: https://reviewboard.mozilla.org/r/142756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142756/

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Chris: thanks for the report!
And yes, it's regression from bug 1307884

@Nicolas: can you check if this also improves the performance?

Honza

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8871264 [details]
Bug 1367266 - Properly calculate autoscroll prop;

https://reviewboard.mozilla.org/r/142756/#review146782

This seems weird to me, why do we need to take care of this when handling the removed messages ? This shouldn't trigger a new render.
Unless it's because of the batching or something that doesn't let the time to the ConsoleOutput to do the render resulting of MESSAGE_ADD.
It worries me that we have to to this because it means that any additionnal action that we could fire after MESSAGE_ADD in the same "render timespan" should be added here.
I can't find something smart about that, but ideally we could maybe not have this autoscroll property on the store and only add the `scrollToMessage` props to the last message we add in the ConsoleOutput, if we are already at the bottom.
I don't know if this the right thing to do though. How do we handle the autoscroll in Netmonitor ?

::: devtools/client/webconsole/new-console-output/store.js:86
(Diff revision 1)
> +      // If there are no removed messages, bail out immediately.
> +      if (!state.messages.removedMessages.length) {
> +        return state;
> +      }

This doesn't seem to resolve the perf issue, even if it's a nice thing to have.

Since I plan to investigate the performance issue on my upcoming patch, and given it's not really related to this bug, can you remove those lines ? 
I'll add them in my patch.

Thanks

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8871264 [details]
Bug 1367266 - Properly calculate autoscroll prop;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142756/diff/1-2/

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Comment on attachment 8871264 [details]
> Bug 1367266 - Properly calculate autoscroll prop;
> 
> https://reviewboard.mozilla.org/r/142756/#review146782
> 
> This seems weird to me, why do we need to take care of this when handling
> the removed messages ? This shouldn't trigger a new render.
> Unless it's because of the batching or something that doesn't let the time
> to the ConsoleOutput to do the render resulting of MESSAGE_ADD.
> It worries me that we have to to this because it means that any additionnal
> action that we could fire after MESSAGE_ADD in the same "render timespan"
> should be added here.
> I can't find something smart about that, but ideally we could maybe not have
> this autoscroll property on the store 
I don't like the current "scroll to bottom" solution either and I think the UI
reducer should not calculate the 'autoscroll' state. And yes, it should be
removed. Since this requires more changes, I would suggest to first: fix this
regression and remove the 'autoscroll' in a follow up (I'll assign to it).
Does that sound ok?


> This doesn't seem to resolve the perf issue, even if it's a nice thing to
> have.
Removed

Honza

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8871264 [details]
Bug 1367266 - Properly calculate autoscroll prop;

https://reviewboard.mozilla.org/r/142756/#review146918

Okay, let's have a follow up to handle that gracefully.
Thanks Honza !

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks, here is the follow up: bug 1368022

Honza

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2231d313a521
Properly calculate autoscroll prop; r=nchevobbe

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2231d313a521

Comment 10

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Managed to reproduce the issue on 55.0a1 (2017-05-23). I can confirm the initial issue is fixed on 55.0a1 (2017-06-07) using  Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.