Entering command does not scroll console to bottom

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Developer Tools: Console
P1
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: Oriol, Assigned: nchevobbe)

Tracking

unspecified
Firefox 57
Points:
---
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-console-html])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
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.
(Assignee)

Comment 1

2 months ago
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 !
No longer blocks: 1307884
Priority: -- → P2
Whiteboard: [console-html][triage]

Updated

2 months ago
Flags: qe-verify?
Priority: P2 → P3
Whiteboard: [console-html][triage] → [reserve-console-html]
(Assignee)

Updated

2 months ago
Assignee: nobody → nchevobbe
Priority: P3 → P1
Comment hidden (mozreview-request)

Updated

2 months ago
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29

Comment 3

2 months ago
mozreview-review
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`
Attachment #8898846 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 4

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

Comment 6

2 months ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6226fe3629c
Scroll the console to the bottom on Evaluation result. r=bgrins

Comment 7

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e6226fe3629c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Reporter)

Updated

2 months ago
Depends on: 1393815
(Reporter)

Updated

2 months ago
See Also: → bug 1395658
You need to log in before you can comment on or make changes to this bug.