Closed
Bug 962647
Opened 11 years ago
Closed 11 years ago
inspector search box does not properly show the node in markup view and breadcrumbs
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox29 verified, firefox30 verified)
VERIFIED
FIXED
Firefox 30
People
(Reporter: Optimizer, Assigned: pbro)
References
Details
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
pbro
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
If the node is not already visible in the markup view, and you search for its selector and select the node, both breadcrumbs and markup view behave wierdly. This is solved by telling the markup view to expand to that node. Also, I have cleaned up the constructor of search suggestions as now it does not need a reference to the content document. (now as in after bug 717369)
Attachment #8363743 -
Flags: review?(pbrosset)
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 8363743 [details] [diff] [review] fix Review of attachment 8363743 [details] [diff] [review]: ----------------------------------------------------------------- So the patch is looking good, no problem there, and it appears to work fine! Great to see the search box working again. However, I realized there is another bug. STR: - Search for a node, it expands normally - Select the <body> with your mouse again - Reload the page - Search for the same node again ==> It does not expand it What's strange is that if you reload again the page, the markup view opens that node on startup, so the node is selected in selection.js, but the markup-view fails to expand it after one reload.
Attachment #8363743 -
Flags: review?(pbrosset)
Reporter | ||
Comment 3•11 years ago
|
||
So I debugged a bit with the help of Patrick and found out that the root cause is that the node returned in the .item() call in selector-search.js is not correct. In the sense that its parentNode().hasChildren is false. On further debugging , found that when the item() call is converting the node into form, we make sure that its parent is a form too at [0]. Somewhere inside that call, something goes wrong and the parent front is incorrectly built. I tried figuring out the root cause of that, but was unable to. needinfo?ing Dave . [0] http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#408
Flags: needinfo?(dcamp)
Reporter | ||
Comment 4•11 years ago
|
||
Note: a very good STR is : - open about:home - open inspector using Ctrl Shift I shortcut such that not all nodes are expanded yet. - Typing "#" in inspector search box and select "#defaultSnippet1" using mouse. You will see that the breadcrumbs and markup view is all messed up. The problem starts with the method `_ensureVisible` of markup-view.js in which the line `node.parentNode();` returns `{parent: null, ...}` object. and because of that, line `parentContainer.childrenDirty = true;` throws as parentContainer is null.
Reporter | ||
Comment 5•11 years ago
|
||
I really hate this bug :( Dave, pretty please to take out some time to look into this :)
Reporter | ||
Comment 6•11 years ago
|
||
Unassigning myself as I am unable to figure out the issue.
Assignee: scrapmachines → nobody
Status: ASSIGNED → NEW
Assignee | ||
Comment 7•11 years ago
|
||
The walker normally ensures that nodes are connected to the tree, so if we receive unparented nodes, it might be because we're the NodeListActor's item method isn't attaching the node correctly to the walker's referenced nodes. I've add similar issues when working on the highlighter remoting. I'll take a look at this bug now.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → pbrosset
Assignee | ||
Comment 8•11 years ago
|
||
Ok, this seems to fix the issue, but doesn't incorporate attachment 8363743 [details] [diff] [review] cleanup changes yet. It also misses new tests. I'll upload a new patch soon.
Assignee | ||
Comment 9•11 years ago
|
||
Of course I attached the wrong patch ... :)
Attachment #8376177 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
New patch: - fixes the missing parents in the NodeListActor's item method - adds a new markup-view test to make sure deeply nested nodes can be searched and appear normally thereafter - cleans up a little bit the selector-search class (as per Optimizer's patch) - and also silences the error that appears in the log when searching for non valid selectors (which happens when you start typing, for instance, '#' in the search field) Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=54714cf232e7
Attachment #8363743 -
Attachment is obsolete: true
Attachment #8376222 -
Attachment is obsolete: true
Attachment #8376321 -
Flags: review?(fayearthur)
Comment 11•11 years ago
|
||
Comment on attachment 8376321 [details] [diff] [review] bug962647-broken-selector-search.patch A few tests are failing.
Attachment #8376321 -
Flags: review?(fayearthur)
Assignee | ||
Comment 12•11 years ago
|
||
Yes, sorry there was a change in the last patch that broke some tests. Here's a fix, with ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=a52c5af49831
Attachment #8376321 -
Attachment is obsolete: true
Attachment #8380560 -
Flags: review?(fayearthur)
Assignee | ||
Comment 13•11 years ago
|
||
And here is the right patch file ...
Attachment #8380560 -
Attachment is obsolete: true
Attachment #8380560 -
Flags: review?(fayearthur)
Attachment #8380561 -
Flags: review?(fayearthur)
Comment 15•11 years ago
|
||
Comment on attachment 8380561 [details] [diff] [review] bug962647-broken-selector-search v3.patch Review of attachment 8380561 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/actors/inspector.js @@ +1389,5 @@ > + // This may fail if the selector isn't a valid selector string > + try { > + nodes = baseNode.rawNode.querySelectorAll(selector); > + } catch (e) {} > + return new NodeListActor(this, nodes); Maybe we actually want this to throw an error from the server-side? Our client side could ignore it, but another consumer might want to know if the selector is invalid.
Attachment #8380561 -
Flags: review?(fayearthur) → review+
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Heather Arthur [:harth] from comment #15) > Comment on attachment 8380561 [details] [diff] [review] > bug962647-broken-selector-search v3.patch > > Review of attachment 8380561 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/devtools/server/actors/inspector.js > @@ +1389,5 @@ > > + // This may fail if the selector isn't a valid selector string > > + try { > > + nodes = baseNode.rawNode.querySelectorAll(selector); > > + } catch (e) {} > > + return new NodeListActor(this, nodes); > > Maybe we actually want this to throw an error from the server-side? Our > client side could ignore it, but another consumer might want to know if the > selector is invalid. You're right, it's better not to remove the error altogether as I did. And the selector-search already deals with the error case, so I'm going to simply remove my change to the querySelectorAll method. Thanks for the review.
Assignee | ||
Comment 17•11 years ago
|
||
Removed the try/catch added in the last patch so that the error is thrown and can be used if needed. Also clearing the needinfo.
Attachment #8380561 -
Attachment is obsolete: true
Attachment #8381249 -
Flags: review+
Flags: needinfo?(dcamp)
Assignee | ||
Comment 18•11 years ago
|
||
R+ from Heather. I launched a last try build to test the v4 patch: https://tbpl.mozilla.org/?tree=Try&rev=bd36b9a09809 Green. Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/db3625395d32
Whiteboard: [fixed-in-fx-team]
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/db3625395d32
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 8381249 [details] [diff] [review] bug962647-broken-selector-search v4.patch [Approval Request Comment] Note that bug 968316 needs to be uplifted before this one is. Also, if we want the patch to apply cleanly, both bug 904953 and bug 969247 should be uplifted, but I would rather provide a new patch instead, since these bugs aren't related at all and just happened to have touched some of the same files. Bug caused by (feature/regressing bug #): This bug has been around for a while, and tbh, I don't know what previous change caused it. It's definitely a regression as the inspector search box used to work fine (I've been able to confirm that it fails on 27, 28 and 29 but couldn't test on 26). Multiple changes have been done on the inspector during these 3 releases that could have caused this regression. User impact if declined: If declined, users of the DevTools won't be able to search for nodes in the inspector using the search box. Note that some parts do work (the css rules do update on search for instance), but the main part, i.e. the markup-view, doesn't show the search results at all. Testing completed (on m-c, etc.): The fix, and corresponding browser mochitest tests, have been in m-c for a week. Risk to taking this patch (and alternatives if risky): Most of the code changes are contained in the selector-search.js file which manages the search box. So since that box isn't working as expected already today, there's not much risks accepting these changes. Some minor changes have been made to inspector.js, which is the back-end to all the devtools inspector panels, so a bug in there could have side effects on inspector functionalities other than the search box. However, this bug has been on m-c for a week, and all automated tests pass, so the risk doesn't seem to exist. String or IDL/UUID changes made by this patch: None
Attachment #8381249 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
status-firefox29:
--- → affected
status-firefox30:
--- → fixed
Updated•11 years ago
|
Attachment #8381249 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 22•11 years ago
|
||
Thanks for the approval, just to be really clear, this bug will need bug 968316 to be applied first, and will need some minor merging.
Comment 24•11 years ago
|
||
My minor merging skills weren't good enough :(. Backed out for mochitest-bc failures. https://hg.mozilla.org/releases/mozilla-aurora/rev/74d3aa838c44 https://tbpl.mozilla.org/php/getParsedLog.php?id=35433132&tree=Mozilla-Aurora Bug 904953 adds the addTab stuff. Honestly, I wouldn't argue if we just uplifted it to Aurora since it's a test-only change anyway and it would get those tests re-enabled. Your call :)
Assignee | ||
Comment 25•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #24) > My minor merging skills weren't good enough :(. Backed out for mochitest-bc > failures. > https://hg.mozilla.org/releases/mozilla-aurora/rev/74d3aa838c44 > > https://tbpl.mozilla.org/php/getParsedLog.php?id=35433132&tree=Mozilla-Aurora > > Bug 904953 adds the addTab stuff. Honestly, I wouldn't argue if we just > uplifted it to Aurora since it's a test-only change anyway and it would get > those tests re-enabled. Your call :) You're right, I'll request aurora-uplift for bug 904953.
Updated•11 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•