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)

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

Tracking

(firefox29 verified, firefox30 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 30
Tracking Flags:
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: Optimizer, Assigned: pbro)

References

Details

Attachments

(1 file, 6 obsolete files)

bug962647-broken-selector-search v4.patch
(deleted), patch
pbro
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Attached patch fix (obsolete) (deleted) — — 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)
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)
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)
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.
I really hate this bug :(
Dave, pretty please to take out some time to look into this :)
Unassigning myself as I am unable to figure out the issue.
Assignee: scrapmachines → nobody
Status: ASSIGNED → NEW
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: nobody → pbrosset
Attached patch bug962647-search-box-not-expanding-nodes.patch (obsolete) (deleted) — — Splinter Review 
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.
Attached patch bug962647-broken-selector-search.patch (obsolete) (deleted) — — Splinter Review 
Of course I attached the wrong patch ... :)
Attachment #8376177 - Attachment is obsolete: true
Attached patch bug962647-broken-selector-search.patch (obsolete) (deleted) — — Splinter Review 
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 on attachment 8376321 [details] [diff] [review]
bug962647-broken-selector-search.patch

A few tests are failing.
Attachment #8376321 - Flags: review?(fayearthur)
Attached patch bug962647-broken-selector-search v2.patch (obsolete) (deleted) — — Splinter Review 
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)
Attached patch bug962647-broken-selector-search v3.patch (obsolete) (deleted) — — Splinter Review 
And here is the right patch file ...
Attachment #8380560 - Attachment is obsolete: true
Attachment #8380560 - Flags: review?(fayearthur)
Attachment #8380561 - Flags: review?(fayearthur)
Tests on try are green this time.
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+
(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.
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)
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]
Can we have this uplifted to aurora and beta please :)
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
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?
Attachment #8381249 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
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 :)
status-firefox29: fixed → affected
(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.
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
status-firefox30: fixed → verified
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: