Closed Bug 1172164 Opened 7 years ago Closed 7 years ago

When highlighting input[type=number], the highlighter descends into div.anonymous-div

Categories

(DevTools :: Inspector, defect)

40 Branch
defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

I found this on http://bgrins.github.io/devtools-demos/inspector/user-agent.html but it can be reproduced on data:text/html,<input type='number' />.

Click to inspect elements and hover the inner part of the input.  Notice that you see 'div.anonymous-div' and that when clicking on that element nothing happens in the markup view.  The highlighter should simply show the normal input in this case.

My guess is this was caused by Bug 1168689.
We should only be using the XBL target in a XUL document.  Or more generically, the Walker should only attach elements that it can know about based on it's current filters.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Hmm, yeah, I was somewhat worried this could happen when working on bug 1168689.

I tried a light amount of testing to check for it, but I suppose I just tried the wrong test cases.
Attached patch input-number-WIP.patch (obsolete) — Splinter Review
no tests
Attached patch input-number.patch (obsolete) — Splinter Review
With a test.  Here's a try push (fingers crossed): https://treeherder.mozilla.org/#/jobs?repo=try&revision=585515e9f86b
Attachment #8616278 - Attachment is obsolete: true
Attachment #8616293 - Flags: review?(pbrosset)
Comment on attachment 8616293 [details] [diff] [review]
input-number.patch

Review of attachment 8616293 [details] [diff] [review]:
-----------------------------------------------------------------

The code changes look good to me. Thanks for spotting this and getting it fixed quickly.
I'm happy to R+ once we've discussed about the potential perf implications of instantiating deeptree walkers.

::: toolkit/devtools/server/actors/inspector.js
@@ +1296,5 @@
>    getDocumentWalker: function(node, whatToShow) {
>      // Allow native anon content (like <video> controls) if preffed on
> +    let nodeFilter = this.showAllAnonymousContent ?
> +                        allAnonymousContentTreeWalkerFilter :
> +                        standardTreeWalkerFilter;

Just a formatting nit, and I might be the only one doing this, but I find this more readable:

let nodeFilter = this.showAllAnonymousContent
                 ? allAnonymousContentTreeWalkerFilter
                 : standardTreeWalkerFilter;

@@ +1420,5 @@
>      let nodeActors = [];
>      let newParents = new Set();
>      for (let node of nodes) {
> +      if (!(node instanceof NodeActor)) {
> +        let rawNode = this.getDocumentWalker(node).currentNode;

Do you know if instantiating documentwalkers is basically free? I see that we do this all over the place, and now it's done here (which is called many times, rapidly, when you move your mouse over the document in pick mode) and it's done one more time just below in ensurePathToRoot.
I'd like to make sure we're not creating a potential perf bottleneck.

I'm sure using the walker here is the only way we can get to the right rawNode target, but perhaps there are ways to reuse walker instances.

@@ +3792,4 @@
>    this.walker.currentNode = node;
> +  while (node &&
> +         this.filter(node) === Ci.nsIDOMNodeFilter.FILTER_SKIP) {
> +    node = this.walker.parentNode();

Ah, this got me very confused for a while, because I didn't know just calling parentNode() would set the walker's currentNode internally correctly, so I was wondering why node wasn't being assigned anywhere.
That's probably because I don't know the walker API well at all, but I could have used a comment here explaining what's happening exactly.
Attachment #8616293 - Flags: review?(pbrosset)
Comment on attachment 8616293 [details] [diff] [review]
input-number.patch

Review of attachment 8616293 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/inspector.js
@@ +1420,5 @@
>      let nodeActors = [];
>      let newParents = new Set();
>      for (let node of nodes) {
> +      if (!(node instanceof NodeActor)) {
> +        let rawNode = this.getDocumentWalker(node).currentNode;

It's not entirely free, but it does seem quick (like 1-2ms).  As discussed, I think we can guard with `!this.showAllAnonymousContent && LayoutHelpers.isAnonymous(node)` to at least make sure we aren't doing this in 99% of the cases.

Also, after investigating a bit, I realize that we are already constructing the documentWalker multiple times for every node change during highlighting mode - in ensurePathToRoot(), children() and getAppliedProps().  There aren't any obvious ways to optimize the construction of the walkers themselves, it's probably just a matter of not constructing one whenever possible.

@@ +3792,4 @@
>    this.walker.currentNode = node;
> +  while (node &&
> +         this.filter(node) === Ci.nsIDOMNodeFilter.FILTER_SKIP) {
> +    node = this.walker.parentNode();

Yeah, the API relies on side effects from the traversal function call.  I've added a comment to clarify
Updated as noted in Comment 6.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09afeff6c0b0
Attachment #8616293 - Attachment is obsolete: true
Attachment #8616725 - Flags: review?(pbrosset)
Attachment #8616725 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/72990290c446
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
See Also: → 1187482
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.