If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Some web pages changes class names on hover

NEW
Unassigned

Status

()

Firefox
Developer Tools: Inspector
P2
normal
5 months ago
4 months ago

People

(Reporter: kernp25, Unassigned)

Tracking

51 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

5 months ago
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
Component: Developer Tools → Developer Tools: Inspector
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?
Flags: needinfo?(kernp25)
(Reporter)

Comment 2

4 months ago
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.
Flags: needinfo?(kernp25)
(Reporter)

Comment 3

4 months ago
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.
(Reporter)

Comment 4

4 months ago
(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 :)
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>).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
(Reporter)

Comment 6

4 months ago
(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
(Reporter)

Comment 7

4 months ago
Here is a page with moving elements:
https://chartbeat.ovb24.aws.eraffe-media.de/?host=bg24
(Reporter)

Comment 8

4 months ago
(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.
Flags: needinfo?(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?
Flags: needinfo?(mixedpuppy)
You need to log in before you can comment on or make changes to this bug.