Closed Bug 597460 Opened 10 years ago Closed 9 years ago

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

Categories

(DevTools :: General, defect, P1, blocker)

defect

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
Firefox 4.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: pcwalton, Assigned: msucan)

References

Details

(Whiteboard: [patchclean:1122])

Attachments

(1 file, 7 obsolete files)

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.
blocking2.0: --- → ?
Assignee: nobody → mihai.sucan
Blocks: 594894
blocking2.0: ? → betaN+
Attached patch proposed fix and test (obsolete) — Splinter Review
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.
Attachment #478060 - Flags: feedback?(pwalton)
Blocks: 529086
Status: NEW → ASSIGNED
Whiteboard: [patchclean:0923]
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
Attachment #478060 - Flags: feedback?(pwalton) → feedback+
(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.
(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.
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #478060 - Attachment is obsolete: true
Attachment #478226 - Flags: review?(gavin.sharp)
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".
(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!
Attached patch patch update 2 (obsolete) — Splinter Review
Rebased and updated patch based on your comments.
Attachment #478226 - Attachment is obsolete: true
Attachment #483425 - Flags: review?(gavin.sharp)
Attachment #478226 - Flags: review?(gavin.sharp)
Whiteboard: [patchclean:0923] → [patchclean:1015]
Version: unspecified → Trunk
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.
Attachment #483425 - Flags: review?(gavin.sharp) → review+
Blocks: devtools4b8
Attached patch patch update 3 (obsolete) — Splinter Review
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+!
Attachment #483425 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [patchclean:1015] → [patchclean:1101]
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.
Attached patch patch update 4 (obsolete) — Splinter Review
Updated the patch. Removed Math.min(). Thanks Gavin!
Attachment #487405 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/3281b4385e72
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch update 5 (obsolete) — Splinter Review
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.
Attachment #487549 - Attachment is obsolete: true
Whiteboard: [patchclean:1101] → [patchclean:1115][checkin]
Whiteboard: [patchclean:1115][checkin] → [clean-enough:1115][checking-in]
Attached patch rebased patch (obsolete) — Splinter Review
Rebased patch. Requires the patch from bug 597756 and its tree of dependencies.
Attachment #490640 - Attachment is obsolete: true
Depends on: 597756
Whiteboard: [clean-enough:1115][checking-in] → [patchclean:1119][checkin]
Blocks: 601177
Whiteboard: [patchclean:1119][checkin] → [does not apply:1120]
Attached patch rebased patchSplinter Review
Rebased patch.
Attachment #491895 - Attachment is obsolete: true
Whiteboard: [does not apply:1120] → [patchclean:1122][checkin]
mass change: filter on PRIORITYSETTING
Blocks: devtools4
Priority: -- → P1
mass change: filter mail on BLOCKERSETTING
Severity: normal → blocker
Patch checked-in:

http://hg.mozilla.org/mozilla-central/rev/284b8ed69ee3
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:1122][checkin] → [patchclean:1122]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.