597460 - Web Console scrolls to top when (at least) a filtered network event occurs

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Patrick Walton (:pcwalton)]

Whenever a network event occurs that is hidden per the user's filter options, the Web Console scrolls to the top. Because this behavior makes the Web Console extremely difficult to use when network events are turned off (which will commonly be the case), I'm nominating this bug for Firefox 4 blocking status.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix and test

Proposed fix and test code.

Please note that the problem you described did not affect only filtered network messages. Problem was that we tried scrolling to nodes that were later filtered by the DOMNodeInserted event handler. I was able to reproduce the same issue with other types of messages that caused scrolling to top when they were filtered. This patch fixes all the cases.

Comment 2

[Patrick Walton (:pcwalton)]

Comment on attachment 478060 [details] [diff] [review]
proposed fix and test

>+
>+    // Scroll to the last unfiltered message.
>+    for (let g = outputNode.children.length-1; g >= 0; g--) {
>+      let group = outputNode.children[g];
>+      for (let i = group.children.length-1; i >= 0; i--) {
>+        let child = group.children[i];
>+        if (!child.classList.contains("hud-filtered-by-type") &&
>+            !child.classList.contains("hud-filtered-by-string")) {
>+          ConsoleUtils.scrollToVisible(child);
>+          return;
>+        }
>+      }
>+    }

Do we want this behavior when searching? It might be annoying if you're scrolled to the top and filtering.

The patch looks great overall, thanks for being so quick on the draw when it comes to fixing my patch's brokenness :) Reminds me that we need to collapse adjacent empty groups when we filter too.

feedback=me

Comment 3

[Mihai Sucan [:msucan]]

(In reply to comment #2)
> Comment on attachment 478060 [details] [diff] [review]
> proposed fix and test
> 
> >+
> >+    // Scroll to the last unfiltered message.
> >+    for (let g = outputNode.children.length-1; g >= 0; g--) {
> >+      let group = outputNode.children[g];
> >+      for (let i = group.children.length-1; i >= 0; i--) {
> >+        let child = group.children[i];
> >+        if (!child.classList.contains("hud-filtered-by-type") &&
> >+            !child.classList.contains("hud-filtered-by-string")) {
> >+          ConsoleUtils.scrollToVisible(child);
> >+          return;
> >+        }
> >+      }
> >+    }
> 
> Do we want this behavior when searching? It might be annoying if you're
> scrolled to the top and filtering.

I think we do. The test code was failing because this part of the code scrolled to the top, always. The network entries when they were added ... they caused scroll to the top, but when I disable network entries, it scrolled to the top for the second time, which is troubling as a user and for the test code. The idea is, if we don't do this, we'll scroll to the top. My thought was, well then let's scroll to the bottom - the most-likely place to be. :)

Do you think I should change the behavior somehow? Maybe remember scroll location before filtering, then make a "guess"? A guess would be: if scrollTop > scrollHeight, then scrollTop = maximum (bottom). If scrollTop was somewhere in the middle, it keeps the old location.

Comment 4

[Patrick Walton (:pcwalton)]

(In reply to comment #3)
> Do you think I should change the behavior somehow? Maybe remember scroll
> location before filtering, then make a "guess"? A guess would be: if scrollTop
> > scrollHeight, then scrollTop = maximum (bottom). If scrollTop was somewhere
> in the middle, it keeps the old location.

I think that'd be better, yeah.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch based on your feedback, Patrick. Thanks for the f+! Now it remembers the scroll location and if it was at the very bottom, it will be at the very bottom after filtering as well.

Now asking for review from Gavin.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 478226 [details] [diff] [review]
updated patch

I don't understand the purpose of the code added to adjustVisibilityForMessageType, can you explain? Why does this code not also belong in adjustVisibilityOnSearchStringChange?

adjustVisibilityForNewlyInsertedNode could return the classes that were set on aNewNode to avoid the need to check classList.contains, or even just a boolean indicating whether it was "hidden".

Comment 7

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> Comment on attachment 478226 [details] [diff] [review]
> updated patch
> 
> I don't understand the purpose of the code added to
> adjustVisibilityForMessageType, can you explain? Why does this code not also
> belong in adjustVisibilityOnSearchStringChange?

The code added remembers the scroll location, to scroll back to that location after filtering the messages. If the user was at the very bottom, it does scroll at the very buttom post-updates.

That code addition is needed because the scroll location is always reset when the user toggles message filtering options, from the toolbar. The scroll location is reset because liftNode() removes the entire Web Console output nodes from the UI, during all of the filtering operations.

(I know, it's ugly, but the purpose of this bug is not to refactor the implementation of log filtering :) )

With regards to adding the same code to adjustVisibilityOnSearchStringChange: yes I can do this. Will update the patch.

> adjustVisibilityForNewlyInsertedNode could return the classes that were set on
> aNewNode to avoid the need to check classList.contains, or even just a boolean
> indicating whether it was "hidden".

Good point. Will update the patch accordingly.

Thanks for your feedback!

Comment 8

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Rebased and updated patch based on your comments.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 483425 [details] [diff] [review]
patch update 2

How about a helper:
function maintainScrollPosition(outputNode, someFunc) {
  let oldScrollTop = outputNode.scrollTop;
  let scrolledToBottom = oldScrollTop + outputNode.clientHeight == outputNode.scrollHeight;

  someFunc();

  if (scrolledToBottom) {
    outputNode.scrollTop = outputNode.scrollHeight - outputNode.clientHeight;
  } else {
    outputNode.scrollTop = oldScrollTop;
  }
}

But I wonder whether maintaining the scroll position when filtering is useful, in the non-scrolledToBottom case... If the filter drastically changes the list, then restoring the same scrollTop doesn't seem like it would be that useful, and we might as well just let it jump around. I guess it might be useful if the filter only changes the list of displayed messages slightly, such that the remaining messages stay in roughly the same position, but I'm not sure that's common enough to be worth the trouble... So I think it'd be fine if you only handled the scrolledToBottom case.

r=me with that considered.

Comment 10

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Patch update 3. Added the maintainScrollPosition() helper function as suggested.

I did not remove the else condition for two reasons:

1. As a user it's really not nice to have it jump at the top when you change the filters. It feels "unnatural" / broken.

2. The mochitest breaks if I remove the else condition. Without much investigation: I presume that while running really quick ... the code mistakes a few scrollTops/clientHeights. Perhaps I'd have to introduce artificial/weird delays, to work around the problem.

Thanks for the review+!

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 487405 [details] [diff] [review]
patch update 3

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+      aOutputNode.scrollTop = Math.min(oldScrollTop, aOutputNode.scrollHeight);

The Math.min clamping isn't needed, the scrollTop setter takes care of that on its own.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: patch update 4

Updated the patch. Removed Math.min(). Thanks Gavin!

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/3281b4385e72

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out due to test failures, e.g.

s: talos-r3-fed-033
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_601352_scroll.js | last message is visible

in:

http://hg.mozilla.org/mozilla-central/rev/8e0d3f4d1e71
http://hg.mozilla.org/mozilla-central/rev/a4a6371c90c5

Comment 15

[Mihai Sucan [:msucan]]

Attachment: patch update 5

Fixed the failing test: browser_webconsole_bug_601352_scroll.js. The failure was caused by the combination of this patch and the new Web Console animation. Disabled the animation for this test, and now it works.

Comment 16

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch. Requires the patch from bug 597756 and its tree of dependencies.

Comment 17

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch.

Comment 18

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 19

[Kevin Dangoor]

mass change: filter mail on BLOCKERSETTING

Comment 20

[Mihai Sucan [:msucan]]

Patch checked-in:

http://hg.mozilla.org/mozilla-central/rev/284b8ed69ee3