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

VERIFIED FIXED in Firefox 55

Status

defect
P1
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: cpeterson, Assigned: Honza)

Tracking

({regression})

unspecified
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 verified)

Details

(Whiteboard: [console-html])

Attachments

(1 attachment)

Reporter

Description

2 years ago
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?
Flags: needinfo?
Flags: needinfo? → qe-verify?
Priority: -- → P2
Whiteboard: [console-html]
Comment hidden (mozreview-request)
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: qe-verify? → qe-verify+
Priority: P2 → P1
@Chris: thanks for the report!
And yes, it's regression from bug 1307884

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

Honza
Iteration: --- → 55.6 - May 29
QA Contact: iulia.cristescu

Comment 3

2 years ago
mozreview-review
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 hidden (mozreview-request)
(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

2 years ago
mozreview-review
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 !
Attachment #8871264 - Flags: review?(nchevobbe) → review+
Thanks, here is the follow up: bug 1368022

Honza

Comment 8

2 years ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2231d313a521
Properly calculate autoscroll prop; r=nchevobbe

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2231d313a521
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.