1360136 - Some web pages changes class names on hover

Status: NEW

(DevTools :: Inspector, enhancement, P2)

Description

[kernp25]

When using findCssSelector [2] in handleContentContextMenu [1] the selector will be ".hover > div:nth-child(2)" (using in content script will not work), right-click on element -> Copy -> CSS Selector gives "li.san_navItem:nth-child(1) > a:nth-child(1) > div:nth-child(2)" which will work in content script.
We can fix this by removing that that part with the class name [3] then the selector will be "div#navigationWrp > nav:nth-child(1) > ul:nth-child(1) > li:nth-child(1) > a:nth-child(1) > div:nth-child(2)".

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#92
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/inspector/css-logic.js#365
[3] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/inspector/css-logic.js#390

Comment 1

[Patrick Brosset <:pbro>]

We were trying to triage this bug today, but failed to find what the problem was from comment 0.
Do you mind providing a test page that we can use to reproduce this problem?

Comment 2

[kernp25]

https://www.otto.de/

Use document.querySelector("li.san_navItem:nth-child(1) > a:nth-child(1) > span:nth-child(1)") in console to select the element.

Comment 3

[kernp25]

This page for example changes the class name in an interval.
https://www.gmx.net/

This page also has the bug with the tagname i mentioned.

document.querySelector will not work here because of the tagName bug.

Comment 4

[kernp25]

(In reply to kernp25 from comment #3)
> document.querySelector will not work here because of the tagName bug.

I mean the Copy -> CSS Selector will not work.

If the tagName is escaped then it will work ofc :)

Comment 5

[Patrick Brosset <:pbro>]

Thank you kernp25 for the details. I think I understand the problem, but I'm still not entirely sure what the test case is.

What I understand is: if you have an element in the page that is getting its class (or id) changed dynamically by JavaScript code, then right-clicking on this element to select it might in fact not select the right element.
The problem is once you select the "inspect element" menu item, we copy a unique selector for the element. We then retrieve this string and use it to actually select the element once the inspector is ready. 
In the meantime, the element might actually have changed and therefore can't be selected in the inspector.

Here's a way to reproduce this problem:
1. Load this URL in a new tab:
data:text/html,<style>div {width:500px;height:500px;background:red;}</style><div></div><script>let i = 0; setInterval(() => document.querySelector("div").className = "c" + ++i, 200)</script>
2. Right-click on the red rectangle
3. Select "Inspect element"
4. Wait for the inspector to open
--> No element gets selected.

That's because we do create a selector to target this element, but since the opening of the inspector is async and takes time, the className of the div has the time to change, and we therefore can't target it when the inspector is ready.

(In reply to kernp25 from comment #0)
> We can fix this by removing that that part with the class name [3] then the
> selector will be "div#navigationWrp > nav:nth-child(1) > ul:nth-child(1) >
> li:nth-child(1) > a:nth-child(1) > div:nth-child(2)".
I don't think that would work. If I understand correctly, you're suggesting to drop the classname part only. While it would fix this particular problem, it wouldn't work if the ID was changed by JS instead of the class. Also, JS could be moving the node around in the DOM.

I can't think of a way to fix this off the top of my head. Creating a selector is the only way I know of right now that allows us to uniquely identify this node for later selection. Maybe there are other ways.

> (In reply to kernp25 from comment #3)
> > document.querySelector will not work here because of the tagName bug.
> 
> I mean the Copy -> CSS Selector will not work.
> 
> If the tagName is escaped then it will work ofc :)
Oh I guess this is about bug 1360132 right? I see the gmx.net site has node with tagNames that have a : in them (e.g. <h:header>).

Comment 6

[kernp25]

(In reply to Patrick Brosset <:pbro> from comment #5)
> (In reply to kernp25 from comment #0)
> > We can fix this by removing that that part with the class name [3] then the
> > selector will be "div#navigationWrp > nav:nth-child(1) > ul:nth-child(1) >
> > li:nth-child(1) > a:nth-child(1) > div:nth-child(2)".
> I don't think that would work. If I understand correctly, you're suggesting
> to drop the classname part only. While it would fix this particular problem,
> it wouldn't work if the ID was changed by JS instead of the class.

But those the ID also get changed on hover?
Does it make sense to change id's on hover?

Then we also remove that part with the id and just go with:
body > div:nth-child(1) > nav:nth-child(1) > ul:nth-child(1) > li:nth-child(1) > a:nth-child(1) > div:nth-child(2)

What you think?

> Also, JS could be moving the node around in the DOM.
Look here what rpl said:
https://bugzilla.mozilla.org/show_bug.cgi?id=1325814#c5

https://bugzilla.mozilla.org/show_bug.cgi?id=1325814#c15
> A reference to the DOM Element can be kept around, no need for a selector at this point.

Or, what you think of this idea?
We keep a reference of the element (event.target) from handleContentContextMenu [2] in browser/base/content/content.js
And, if some one needs the selector of the element it sends message to content to create selector and sends the selector back to chrome?

Will this work?

But, this has performance problems i think?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/css-selector.js#61
[2] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#93

Comment 7

[kernp25]

Here is a page with moving elements:
https://chartbeat.ovb24.aws.eraffe-media.de/?host=bg24

Comment 8

[kernp25]

(In reply to kernp25 from comment #6)
> Or, what you think of this idea?
> We keep a reference of the element (event.target) from
> handleContentContextMenu [2] in browser/base/content/content.js
> And, if some one needs the selector of the element it sends message to
> content to create selector and sends the selector back to chrome?
> 
> Will this work?
> 
> But, this has performance problems i think?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/css-selector.
> js#61
> [2]
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.
> js#93

What you think of this idea?
This is the same what you are doing in https://bugzilla.mozilla.org/show_bug.cgi?id=1338898

Then we can leave that part with the classname because there is no hover anymore at this point.

Comment 9

[Shane Caraveo (:mixedpuppy)]

I suppose we could do more than one thing in the late round trip, get the full text, css selector, maybe other items come up later.  Are you certain the selector will be stable if we do it that way?  (I haven't fully thought that through)  Also that would make the selector more webext specific in behavior, how is devtools using it?

Comment 10

[Patrick Brosset <:pbro>]

It came to my attention that we could make use of this module to fix the problem.
It's a nice helper that generates a unique ID for a DOM node that can be passed around across processes (as it's just a string) and be used again later to resolve to the original DOM node.

Today we generate a CSS selector when right-clicking on a node in the page (actually multiple selectors if that node is in a nested iframe).
Instead we could generate the ID for that node (and parent iframe IDs if needed), and then use that to resolve to the actual node in findNodeFront.

Comment 11

[Julian Descottes [:jdescottes]]

Might close as duplicate after Bug 1588773 lands