613642 - Web Console is hard to use with polling XMLHttpRequests

Status: VERIFIED FIXED

(DevTools :: General, defect, P1)

Description

[Ian Bicking (:ianbicking)]

I opened the Web Console and went to http://ethercodes.com and opened a new pad.  This site polls via XMLHttpRequest every second or two, and so the requests stream by.  I am unable to scroll to the beginning (where the interesting requests are) because each new request causes the window to scroll to the bottom again.

Comment 1

[David Dahl :ddahl]

Ah, cool. I have wanted to file this bug and keep forgetting. we need to turn off scrollToVisible on new nodes when a user scrolls back up to look at previous entries.

Comment 2

[Kevin Dangoor]

This is a good point. I'm requesting blocking for this, because I agree that this behavior would get maddening.

Comment 3

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 4

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 5

[Rob Campbell [:rc] (:robcee)]

Marking as assigned.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch. This fixes the reported issue, bug 614793, and an unreported scrolling bug.

The reported issue is fixed by not scrolling to the latest appended message node when the user is not scrolled to the bottom (obviously).

Bug 614793 is fixed by scrolling to .jsterm-input-line and .jsterm-output-line messages as well (not just .hud-msg-node ones). It was a one-liner fix.

See the DOMNodeInserted event handler for the above two fixes.

The "unreported scrolling bug" was only a matter of updating the pruneConsoleOutputIfNecessary() function. When the user is scrolled in the middle of output messages, and there's stuff being logged, while old messages are removed, you see how the messages scroll towards the top, breaking your focus. The pruning code now remembers the scroll location and you are no longer bothered by removals.

I have included mochitests for the three cases. Looking forward to your feedback. Thanks!

Comment 7

[Patrick Walton (:pcwalton)]

Comment on attachment 494119 [details] [diff] [review]
proposed patch

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>--- a/toolkit/components/console/hudservice/HUDService.jsm
>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>@@ -1336,32 +1336,40 @@ function pruneConsoleOutputIfNecessary(a
>+  let messageNodes = aConsoleNode.querySelectorAll(
>+                     ".hud-msg-node, .jsterm-input-line, .jsterm-output-line");

This won't be necessary once bug 605621 lands, but this is more likely to land first, so that's fine.

>   for (let i = 0; i < messageNodes.length - logLimit; i++) {
>     let messageNode = messageNodes[i];
>     let groupNode = messageNode.parentNode;
>     if (!groupNode.classList.contains("hud-group")) {
>       throw new Error("pruneConsoleOutputIfNecessary: message node not in a " +
>                       "HUD group");
>     }
> 
>     groupNode.removeChild(messageNode);
> 
>     // If there are no more children, then remove the group itself.
>-    if (!groupNode.querySelector(".hud-msg-node")) {
>+    if (!groupNode.hasChildNodes()) {
>       groupNode.parentNode.removeChild(groupNode);
>     }
>   }
>+
>+  if (!scrolledToBottom && oldScrollHeight != aConsoleNode.scrollHeight) {
>+    aConsoleNode.scrollTop -= oldScrollHeight - aConsoleNode.scrollHeight;
>+  }
> }
> 
> ///////////////////////////////////////////////////////////////////////////
> //// The HUD service
> 
> function HUD_SERVICE()
> {
>   // TODO: provide mixins for FENNEC: bug 568621
>@@ -1741,25 +1749,24 @@ HUD_SERVICE.prototype =
>    * @param function aCallback
>    *        The callback function you want to execute.
>    * @returns void
>    */
>   maintainScrollPosition:
>   function HS_maintainScrollPosition(aOutputNode, aCallback)
>   {
>     let oldScrollTop = aOutputNode.scrollTop;
>-    let scrolledToBottom = oldScrollTop +
>-      aOutputNode.clientHeight == aOutputNode.scrollHeight;
> 
>     aCallback.call(this);
> 
>     // Scroll to the bottom if the scroll was at the bottom.
>-    if (scrolledToBottom) {
>+    if (aOutputNode.lastScrollTop == oldScrollTop) {
>       aOutputNode.scrollTop = aOutputNode.scrollHeight -
>         aOutputNode.clientHeight;
>+      aOutputNode.lastScrollTop = aOutputNode.scrollTop;
>     }
>     else {
>       // Remember the scroll position.
>       aOutputNode.scrollTop = oldScrollTop;
>     }
>   },
> 
>   /**
>@@ -1860,17 +1867,17 @@ HUD_SERVICE.prototype =
>       hidden = true;
>     }
> 
>     // Filter by the message type.
>     let classes = aNewNode.classList;
>     let msgType = null;
>     for (let i = 0; i < classes.length; i++) {
>       let klass = classes.item(i);
>-      if (klass !== "hud-msg-node" && klass.indexOf("hud-") === 0) {
>+      if (klass !== "hud-msg-node" && klass !== "hud-clickable" && klass.indexOf("hud-") === 0) {
>         msgType = klass.substring(4);   // Strip off "hud-".
>         break;
>       }
>     }
>     if (msgType !== null && !this.getFilterState(aHUDId, msgType)) {
>       // The node is filtered by type.
>       aNewNode.classList.add("hud-filtered-by-type");
>       hidden = true;
>@@ -3420,29 +3427,44 @@ HeadsUpDisplay.prototype = {
>+    // Last scrollTop that was programatically updated.

nit: programmatically

>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ *
>+ * Contributor(s):
>+ *  Mihai Èucan <[e-mail]>
>+ *
>+ * ***** END LICENSE BLOCK ***** */

I believe you don't want to use the "BEGIN LICENSE BLOCK" stuff here. Instead use this: http://www.mozilla.org/MPL/boilerplate-1.1/pd-c

This applies to the other tests as well.

Looks great otherwise, f+ with those nits fixed!

Comment 8

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Thanks for your feedback+ Patrick! Updated the patch as requested.

Comment 9

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Removed the fix for bug 614793. Moved those changes into their own bug and patch, as requested by Robert.

Comment 10

[Justin Dolske [:Dolske]]

Previous comments say this interacts with bug 605621, of which Part 1 has landed... Does the patch here need updated, or is it reviewable as-is? (Sorry for the terrible delay getting to this.)

Comment 11

[Mihai Sucan [:msucan]]

Attachment: patch update 4

Rebased the patch. This is now ready for review.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: patch update 4 (fixed)

Slipped a one-liner change that shouldn't have. (during testing I always remove type=content for the network panel iframe, to prevent bug 607325 from happening on my system)

Sorry for the inconvenience.

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 500844 [details] [diff] [review]
patch update 4 (fixed)

r+ assuming pwalton is ok with it still (it's changed a bit since v.1 was feedback+'d).

Comment 14

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch

Thanks for the r+! Rebased the patch. Should apply cleanly now, ready for checkin.

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 503110 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/0bf8fd59c836

Comment 16

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Verified on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10) Gecko/20100101 Firefox/4.0b10 ID:20110121161358

ethercodes.com is gone, but verified by generating a bunch of console messages in gmail, scrolling up to the middle of the mass, then generating a bunch more. Focus stayed on the same message until it was pruned.