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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

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.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: input-number-WIP.patch

no tests

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: input-number.patch

With a test.  Here's a try push (fingers crossed): https://treeherder.mozilla.org/#/jobs?repo=try&revision=585515e9f86b

Comment 5

[Patrick Brosset <:pbro>]

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.

Comment 6

[Brian Grinstead [:bgrins]]

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

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: input-number.patch

Updated as noted in Comment 6.  Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09afeff6c0b0

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/72990290c446

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/72990290c446

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/72990290c446