Closed
Bug 613642
Opened 14 years ago
Closed 14 years ago
Web Console is hard to use with polling XMLHttpRequests
Categories
(DevTools :: General, defect, P1)
DevTools
General
Tracking
(blocking2.0 final+)
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: ianbicking, Assigned: msucan)
References
Details
(Whiteboard: [patchclean:0112][softblocker])
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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.
Updated•14 years ago
|
Version: unspecified → Trunk
Comment 2•14 years ago
|
||
This is a good point. I'm requesting blocking for this, because I agree that this behavior would get maddening.
Comment 4•14 years ago
|
||
mass change: filter on PRIORITYSETTING
Updated•14 years ago
|
blocking2.0: ? → final+
Updated•14 years ago
|
Severity: normal → blocker
Assignee | ||
Comment 6•14 years ago
|
||
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!
Attachment #494119 -
Flags: feedback?(pwalton)
Comment 7•14 years ago
|
||
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 <mihai.sucan@gmail.com>
>+ *
>+ * ***** 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!
Attachment #494119 -
Flags: feedback?(pwalton) → feedback+
Assignee | ||
Comment 8•14 years ago
|
||
Thanks for your feedback+ Patrick! Updated the patch as requested.
Attachment #494119 -
Attachment is obsolete: true
Attachment #495856 -
Flags: review?(dolske)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:1130] → [patchclean:1207]
Assignee | ||
Comment 9•14 years ago
|
||
Removed the fix for bug 614793. Moved those changes into their own bug and patch, as requested by Robert.
Attachment #495856 -
Attachment is obsolete: true
Attachment #497512 -
Flags: review?(dolske)
Attachment #495856 -
Flags: review?(dolske)
Assignee | ||
Updated•14 years ago
|
Updated•14 years ago
|
Severity: blocker → normal
Comment 10•14 years ago
|
||
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.)
Assignee | ||
Comment 11•14 years ago
|
||
Rebased the patch. This is now ready for review.
Attachment #497512 -
Attachment is obsolete: true
Attachment #500835 -
Flags: review?(dolske)
Attachment #497512 -
Flags: review?(dolske)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:1214] → [patchclean:0103]
Assignee | ||
Comment 12•14 years ago
|
||
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.
Attachment #500835 -
Attachment is obsolete: true
Attachment #500844 -
Flags: review?(dolske)
Attachment #500835 -
Flags: review?(dolske)
Comment 13•14 years ago
|
||
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).
Attachment #500844 -
Flags: review?(dolske) → review+
Updated•14 years ago
|
Whiteboard: [patchclean:0103] → [patchclean:0103][checkin-needed]
Updated•14 years ago
|
Whiteboard: [patchclean:0103][checkin-needed] → [patchclean:0103][checkin-needed][softblocker]
Assignee | ||
Comment 14•14 years ago
|
||
Thanks for the r+! Rebased the patch. Should apply cleanly now, ready for checkin.
Attachment #500844 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:0103][checkin-needed][softblocker] → [patchclean:0112][checkin-needed][softblocker]
Comment 15•14 years ago
|
||
Comment on attachment 503110 [details] [diff] [review]
[checked-in] rebased patch
http://hg.mozilla.org/mozilla-central/rev/0bf8fd59c836
Attachment #503110 -
Attachment description: rebased patch → [checked-in] rebased patch
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:0112][checkin-needed][softblocker] → [patchclean:0112][softblocker]
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.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•