1389803 - Entering command does not scroll console to bottom

Status: VERIFIED FIXED

(DevTools :: Console, defect, P1)

Description

[Oriol Brufau [:Oriol]]

1. Open web console
2. Keep pressing 1 and enter until the console is filled and a scrollbar appears
3. Scroll up a bit
4. Enter something again.

Expected: the console scrolls to the bottom so that you can see the result of your command.

Result: the console does not scroll.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Hello Oriol,

We're doing that on purpose: if the output is not scrolled to the bottom, we keep the user at the same place, even if new messages are appended in the console.
For console.* calls, it seems logical: we don't want to send the user to the bottom if they are looking for a specific message, or if they are inspecting an object.

But, I do think it would make sense for evaluation result: the user probably wants to see the result of what they entered.

And this is what was done in the old frontend, so we should do the same in the new one.

Thanks !

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1389803 - Scroll the console to the bottom on Evaluation result.

This ensures that we scroll to the bottom when the user evaluate
something in the console.
A test is added to make sure this works as expected.

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

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8898846 [details]
Bug 1389803 - Scroll the console to the bottom on Evaluation result.

https://reviewboard.mozilla.org/r/170218/#review175548

Thanks, this looks good!

::: devtools/client/webconsole/new-console-output/components/console-output.js:79
(Diff revision 1)
> +    //   and we are already scrolled to the bottom
> +    // - the number of messages in the store changed
> +    //   and the new message is an evaluation result.
> +    this.shouldScrollBottom =
> +      (visibleMessagesDelta > 0 && isScrolledToBottom(lastChild, outputNode)) ||
> +      (messagesDelta > 0 && nextProps.messages.last().type === MESSAGE_TYPE.RESULT);

Any reason to not invert the order of these checks to prevent doing the isScrolledToBottom check when we get an evaluation result?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_scroll.js:11
(Diff revision 1)
> +"use strict";
> +
> +const TEST_URI = "data:text/html;charset=utf-8,<p>Web Console test for " +
> +                 "scroll behavior";
> +add_task(async function () {
> +  let toolbox = await openNewTabAndToolbox(TEST_URI, "webconsole");

Nit: instead of these two lines you can do:

```
const hud = await openNewTabAndConsole(TEST_URI);
```

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_scroll.js:15
(Diff revision 1)
> +add_task(async function () {
> +  let toolbox = await openNewTabAndToolbox(TEST_URI, "webconsole");
> +  let hud = toolbox.getCurrentPanel().hud;
> +  let {ui} = hud;
> +
> +  ok(ui.jsterm, "jsterm exists");

Nit: no need to have these two assertions. Could throw an info() in to indicate that we are waiting for messages

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_scroll.js:21
(Diff revision 1)
> +  ok(ui.newConsoleOutput, "newConsoleOutput exists");
> +
> +  let receievedMessages = waitForMessages({hud, messages: [{
> +    text: "init-99"
> +  }]});
> +  await ContentTask.spawn(gBrowser.selectedBrowser, {}, function () {

Nit: we shouldn't need to `await` here - `receivedMessages` should handle that

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_scroll.js:39
(Diff revision 1)
> +
> +  info("Add a message to check that the scroll isn't impacted");
> +  receievedMessages = waitForMessages({hud, messages: [{
> +    text: "stay"
> +  }]});
> +  await ContentTask.spawn(gBrowser.selectedBrowser, {}, function () {

ditto about not needing to `await`

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_scroll.js:57
(Diff revision 1)
> +
> +  info("Add a message to check that the console do scroll since we're at the bottom");
> +  receievedMessages = waitForMessages({hud, messages: [{
> +    text: "scroll"
> +  }]});
> +  await ContentTask.spawn(gBrowser.selectedBrowser, {}, function () {

Ditto about not needing to `await`

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8898846 [details]
Bug 1389803 - Scroll the console to the bottom on Evaluation result.

https://reviewboard.mozilla.org/r/170218/#review175548

> Any reason to not invert the order of these checks to prevent doing the isScrolledToBottom check when we get an evaluation result?

Good idea

> Nit: instead of these two lines you can do:
> 
> ```
> const hud = await openNewTabAndConsole(TEST_URI);
> ```

Right, I always forget this function

> Nit: we shouldn't need to `await` here - `receivedMessages` should handle that

good catch, thanks

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8898846 [details]
Bug 1389803 - Scroll the console to the bottom on Evaluation result.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6226fe3629c
Scroll the console to the bottom on Evaluation result. r=bgrins

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e6226fe3629c

Comment 8

[Sajedul Islam]

I have reproduced this bug with Nightly 57.0a1 (2017-08-12) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest beta !

Build   ID    20171102181127
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20171101]

Comment 9

[Oriol Brufau [:Oriol]]

Verifying per comment 8.