Closed Bug 1367266 Opened 5 years ago Closed 5 years ago

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

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

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

VERIFIED FIXED
Firefox 55
Iteration:
55.6 - May 29
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- verified

People

(Reporter: cpeterson, Assigned: Honza)

References

Details

(Keywords: regression, Whiteboard: [console-html])

Attachments

(1 file)

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]
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 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
(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 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
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2231d313a521
Properly calculate autoscroll prop; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/2231d313a521
Status: ASSIGNED → RESOLVED
Closed: 5 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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.