Closed Bug 950732 Opened 11 years ago Closed 11 years ago

DevTools Inspector "Show all nodes" option is unclickable

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 29

People

(Reporter: mgol, Assigned: pbro)

References

(
URL
)

Details

Attachments

(2 files, 1 obsolete file)

Test case HTML
504 bytes, text/html
Details
bug950732-show-all-nodes-unclickable V2.patch
10.06 KB, patch
pbro
: review+
Details | Diff | Splinter Review
See the attached video. I have a container with a large number of nodes and I don't see them all. At the end of the container there's a "show all 206 nodes" button that is unclickable (clicking on it scrolls the viewport up to the opening container tag). It is reproducable both on nightly & stable.
May be related to Bug 892979
OS: Mac OS X → All
Hardware: x86_64 → All
Reproducible by creating an element with a bunch of child nodes and clicking on the 'Show All N Nodes' button. Simple test case here: http://fiddle.jshell.net/bgrins/XPXjk/1/show/
Works in latest nightly though.
To be more precise, it works in my development build (updated yesterday), with a couple of patches applied. So I'll test with today's nightly release too.
Right, so it indeed fails in nightly, so something in my patch (for bug 916443) must have fixed that. Let me take this bug and test again when my patch lands.
Assignee: nobody → pbrosset
(In reply to Patrick Brosset [:pbrosset] from comment #5) > Right, so it indeed fails in nightly, so something in my patch (for bug > 916443) must have fixed that. > Let me take this bug and test again when my patch lands. Great, I was looking into it a bit and realized it may have some overlap with the highlighter code. You may also check Bug 892979 and see if this is resolved with your patch applied.
Depends on: 916443
Attached file Test case HTML —
Good test case provided in bug 953049. So it's indeed a little bit better now that bug 916443 landed. It works the first time. But after you reload the page, it doesn't work anymore. And it even gets weirder if you reload yet another time: even the nodes that were normally displayed above the "show all" button are not displayed anymore.
The bug whereby <li> nodes are not displayed anymore after reload is related to the MarkupContainer destroy process. That's an easy fix. Now, the bug whereby the "show more nodes" isn't working only occurs if, when the page loads, the <ul> is pre-selected in the markup-view. If instead the <body> is selected, then the <ul> is collapsed, and after expanding it, the button works. However, if the <ul> is selected and then the page is reloaded, then the <ul> becomes the default selected node and it's expanded automatically, then the button won't work.
Here's a simple fix for the 2 problems. First of all, a fix for the destroy mechanism to avoid the markup-view from failing after a page reload. Then, a fix for the show-all-nodes button click handler, which was basically colliding with the line click handler. + 1 new test Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=476c50b7eb42
Attachment #8358390 - Flags: review?(jwalker)
Comment on attachment 8358390 [details] [diff] [review] bug950732-show-all-nodes-unclickable.patch Review of attachment 8358390 [details] [diff] [review]: ----------------------------------------------------------------- These comments are fairly picky, so I won't be cross if you don't update the patch, but I do think they make things simpler. ::: browser/devtools/markupview/test/browser_inspector_markup_950732.js @@ +49,5 @@ > + var target = TargetFactory.forTab(gBrowser.selectedTab); > + let toolbox = gDevTools.showToolbox(target, "inspector").then(function(toolbox) { > + inspector = toolbox.getCurrentPanel(); > + markup = inspector.markup; > + inspector.once("inspector-updated", deferred.resolve); We really should make EventEmitter.once return a promise if there is no callback function, it would come close to halving the length of this test. (Bug 927423) @@ +78,5 @@ > + }); > + content.location.reload(); > + > + return deferred.promise; > + } This is picky, but we can do: function reloadPage() { let reloaded = inspector.once("new-root", () => { doc = content.document; markup = inspector.markup; markup._waitForChildren(); }); content.location.reload(); return reloaded; } @@ +91,5 @@ > + EventUtils.sendMouseEvent({type: "click"}, button, win); > + markup._waitForChildren().then(deferred.resolve); > + > + return deferred.promise; > + } Similar to the above, but more so... function showAllNodes() { let container = getContainerForRawNode(markup, doc.querySelector("ul")); let button = container.elt.querySelector("button"); let win = button.ownerDocument.defaultView; EventUtils.sendMouseEvent({type: "click"}, button, win); return markup._waitForChildren(); }
Attachment #8358390 - Flags: review?(jwalker) → review+
(In reply to Joe Walker [:jwalker] from comment #11) > Comment on attachment 8358390 [details] [diff] [review] > bug950732-show-all-nodes-unclickable.patch > > Review of attachment 8358390 [details] [diff] [review]: > ----------------------------------------------------------------- > > These comments are fairly picky, so I won't be cross if you don't update the > patch, but I do think they make things simpler. > > ::: browser/devtools/markupview/test/browser_inspector_markup_950732.js > @@ +49,5 @@ > > + var target = TargetFactory.forTab(gBrowser.selectedTab); > > + let toolbox = gDevTools.showToolbox(target, "inspector").then(function(toolbox) { > > + inspector = toolbox.getCurrentPanel(); > > + markup = inspector.markup; > > + inspector.once("inspector-updated", deferred.resolve); > > We really should make EventEmitter.once return a promise if there is no > callback function, it would come close to halving the length of this test. > (Bug 927423) Yeah, that would indeed be quite useful! > @@ +78,5 @@ > > + }); > > + content.location.reload(); > > + > > + return deferred.promise; > > + } > > This is picky, but we can do: > > function reloadPage() { > let reloaded = inspector.once("new-root", () => { > doc = content.document; > markup = inspector.markup; > markup._waitForChildren(); > }); > content.location.reload(); > > return reloaded; > } You mean, once we have bug 927423 fixed, right? > @@ +91,5 @@ > > + EventUtils.sendMouseEvent({type: "click"}, button, win); > > + markup._waitForChildren().then(deferred.resolve); > > + > > + return deferred.promise; > > + } > > Similar to the above, but more so... > > function showAllNodes() { > let container = getContainerForRawNode(markup, doc.querySelector("ul")); > let button = container.elt.querySelector("button"); > let win = button.ownerDocument.defaultView; > > EventUtils.sendMouseEvent({type: "click"}, button, win); > return markup._waitForChildren(); > } True, it's a lot simpler. Fixed!
(In reply to Patrick Brosset [:pbrosset] from comment #12) > (In reply to Joe Walker [:jwalker] from comment #11) > > This is picky, but we can do: > > > > function reloadPage() { > > let reloaded = inspector.once("new-root", () => { > > doc = content.document; > > markup = inspector.markup; > > markup._waitForChildren(); > > }); > > content.location.reload(); > > > > return reloaded; > > } > You mean, once we have bug 927423 fixed, right? Yes, sorry that wasn't clear.
Thanks Joe for the review. Here's a V2 for the same patch. The only difference being I fixed the showAllNodes function in the test. New ongoing try: https://tbpl.mozilla.org/?tree=Try&rev=b009867ef33e
Attachment #8358390 - Attachment is obsolete: true
Attachment #8359127 - Flags: review+
Try is happy :)
(In reply to Patrick Brosset [:pbrosset] from comment #15) > Try is happy :) (I'm the guy that reported the duplicate) Seems like I'll be seeing this any time soon on my nightly :) and it'll help me tons on my work. Blazing fast way to solve it! You guys rock!
Fixed in fx-team https://hg.mozilla.org/integration/fx-team/rev/b1616355b439
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b1616355b439
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: