985517 - [markup-view] pseudo-class lock goes away when clicking on other elements in markup view

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Brian Grinstead [:bgrins]]

It seems that the :hover lock becomes unlocked when navigating around the markup view.  There are multiple ways to trigger this issue, here are 2:

http://jsfiddle.net/bgrins/9QVvw/8/

* Right click on parent node in markup view, select :hover from context menu
* Notice that :hover specific styling is applied to the element.
* Click on a text node in the markup view
* :hover lock goes away


http://jsfiddle.net/bgrins/9QVvw/7/

* Right click on parent node in markup view, select :hover from context menu
* Notice that :hover specific styling is applyed to the element.
* Click on the first child in the node
* Notice that any :hover specific styling is applied to the element.
* Click on the second child in the node
* :hover lock goes away

Comment 1

[Heather Arthur [:harth]]

Yeah, this was by design.

Otherwise, you might leave :hover on something and lose track of that. I guess that's not the end of the world?

It should stay if you click another node in the same hierarchy chain, child or ancestor.

Comment 2

[Heather Arthur [:harth]]

Ah, also, we'd lose track of what elements you have these pseudo-class locks applied to. When you closed the devtools you want these to go away. We could track every lock you've applied I suppose. But that is one reason why we didn't want to do this initially.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Heather Arthur [:harth] from comment #1)
> Yeah, this was by design.
> 
> Otherwise, you might leave :hover on something and lose track of that. I
> guess that's not the end of the world?
> 
> It should stay if you click another node in the same hierarchy chain, child
> or ancestor.

I see that it could be easy to lose track right now since we don't visually indicate which node has a lock applied, but we could maybe show that with an icon or some sort decoration on a node is locked (this could even show up on a parent if it was collapsed).  If the lock was visually shown in the markup view I think we could never reset the lock.  That does bring up the issue of keeping track of multiple locks at once - but that is the behavior I would expect.

At the least, if the user is sticking within the hierarchy chain (especially on children) we should be keeping the :hover applied, since they wouldn't lose track of it in this case.

Comment 5

[Heather Arthur [:harth]]

So right now we do keep it as long as you're navigating to an ancestor or descendant. We clear them if you navigate to sibling or another element off the hierarchy.

We could leave them on, and either track them and delete all the locks when you close the tools, or keep them on. How bad would that be?

You could get rid of them by reloading the page or opening the tools again and re-inspecting the elements.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Heather Arthur [:harth] from comment #5)
> So right now we do keep it as long as you're navigating to an ancestor or
> descendant. We clear them if you navigate to sibling or another element off
> the hierarchy.
> 
> We could leave them on, and either track them and delete all the locks when
> you close the tools, or keep them on. How bad would that be?
> You could get rid of them by reloading the page or opening the tools again
> and re-inspecting the elements.

IMO we should clear any locks when the tools close to restore things to a normal state.  Too much potential for weirdness / thinking that devtools broke your stuff.  I like the idea of tracking the locks then removing them when the toolbox closes if that is doable.

Comment 7

[friedrich]

hy im agree with you, and think its the better solution to keep the hover state as long as you're navigating to an ancestor.

but in addition to your solution:

if you activate the hover state for an element, it could be helpful if you can add some other hover states to its child-elemnts (eg. for a multi-level-navigation)

Comment 8

[friedrich]

if someone fix this bug. 

*****
take a look at these little similar bug.
https://bugzilla.mozilla.org/show_bug.cgi?id=1042523
*****

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Heather Arthur [:harth] from comment #5)
> So right now we do keep it as long as you're navigating to an ancestor or
> descendant. We clear them if you navigate to sibling or another element off
> the hierarchy.

In the other bug the STR is to set :hover on li#extensions - and the wtf moment for me is that both the a and ul elements are descendants of that li, so I would expect clicking on either of those children to preserve :hover, not just the first. Does that make sense?

Comment 10

[friedrich]

Hy, I think you should consider these issue with a more detailed example.

http://jsfiddle.net/xnarwzka/2/embedded/result/

I think the workflow is not right.
* in Inspector, you activate :hove on .level1 > li 
* after that i want to prototype the style für .level2 > p
  * but now the hover state get loose

i have to active :hover-state for every .level2 > li, if i prototype <p> and <a>
I beleve this is not developer friendly.

what do you think, sorry for my bad english

Comment 11

[Patrick Brosset <:pbro>]

Thank you Friedrich for this test case. I agree with you, the workflow feels broken.
Once you set :hover on an element and the menu is shown, it's very easy to make it hidden again. Just selecting a child or sibbling element will do it.

It's not an easy problem to solve, because on one hand we want to make sure :hover stays locked so you can use the inspector while the menu is shown to modify any styles/attributes you want, but on the other hand, we want to make sure it's easy to remove the :hover lock.

Chrome doesn't ever remove the lock when you select other nodes, and it doesn't even remove it when you close the devtools, BUT, if you mouse over any node that has a :hover style, that will cause the lock to be removed. I think this workflow is a lot better.
Additionally, they show a little marker next to every node that has the :hover lock in the inspector.

On Firefox, once the pseudo-class is locked, it doesn't get unlocked if you mouse over the node again.

Comment 12

[Patrick Brosset <:pbro>]

We use inDOMUtils::addPseudoClassLock to lock pseudo classes.
To test it:
- open http://jsfiddle.net/xnarwzka/2/embedded/result/
- open scratchpad
- switch it to "browser environment"
- run the following code:

let DOMUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
let node = content.document.querySelector("iframe").contentDocument.querySelectorAll(".level1 li")[1];
DOMUtils.addPseudoClassLock(node, ":hover");

You'll see that the only thing that can get rid of the lock is running:

DOMUtils.removePseudoClassLock(node, ":hover");

Comment 13

[friedrich]

The way how Chrome works is also not nice. 

I think the actual workflow its not intuitive enought for developer.
How can Someone figure out, that only the li:first-child of an :hover activated <ul> is continuous.

I beleve, the :hover-state should keep activated for each Child-element, until some other Element in Inspector was selected or the Devbar was closed.

Comment 14

[Heather Arthur [:harth]]

Here's a problem with just leaving the locks on: IRL, hovering causes the :hover psuedo-class to apply all the way up to the body. So we lock :hover all the way up to the body as well. It's confusing to leave :hover applied to the body and all the big ancestors of the element you just locked. Maybe not though? Just putting that situation out there.

Comment 16

[Sebastian Zartner [:sebo]]

(In reply to Heather Arthur  [:harth] from comment #14)
> Here's a problem with just leaving the locks on: IRL, hovering causes the
> :hover psuedo-class to apply all the way up to the body. So we lock :hover
> all the way up to the body as well.

I believe that's something the DOMUtils API should already handle. Created bug 1120399 for that.

> It's confusing to leave :hover applied
> to the body and all the big ancestors of the element you just locked. Maybe
> not though? Just putting that situation out there.

You mean how it currently works? Then the confusion already comes from the spec defining that behavior.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #11)
> Chrome doesn't ever remove the lock when you select other nodes, and it
> doesn't even remove it when you close the devtools, BUT, if you mouse over
> any node that has a :hover style, that will cause the lock to be removed.

To be more precise on this, it keeps the hover state of the last selected element after the devtools are closed, though removes the hover states of all other elements, which seems to be a bug.

> think this workflow is a lot better.

I agree.

> Additionally, they show a little marker next to every node that has the
> :hover lock in the inspector.

That's a good visual indicator for this. Added bug 1120406 for adding the visualization.

Sebastian

Comment 19

[Brian Grinstead [:bgrins]]

I think now that Bug 1120406 has landed we can stop removing the lock when other nodes are selected in the markup view, since it will be seen in the inspector.  We should still remove the locks when a new lock is set or when the tool is destroyed.

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: pseudo-class-stay-WIP.patch

WIP, needs test changes.  Mostly wondering how this should interact if you enable multiple pseudo class locks on the page (there ends up being dots in the markup view everywhere).  Is there any case where we want pseudo class locks to be removed (short of the page being refreshed, toolbox being closed, etc).

Comment 21

[Brian Grinstead [:bgrins]]

In addition to Comment 20, what happens if we have this DOM:

<html>
  <body>
    <div />
  </body>
</html>

Then set :hover on <div>, then unset :hover on <body>?  Should the lock chain be removed all the way down to the <div> or just from <body>?  What happens with <html> in that case?

Comment 22

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Created attachment 8658910 [details] [diff] [review]
> pseudo-class-stay-WIP.patch
> 
> WIP, needs test changes.  Mostly wondering how this should interact if you
> enable multiple pseudo class locks on the page (there ends up being dots in
> the markup view everywhere).
Shouldn't we allow only one pseudo class lock at a time? Under real conditions, there can only be one hovered element hierarchy, only one active or focused element.
My assumption was that locking a new node would remove the lock on whatever previous node was locked.

(In reply to Brian Grinstead [:bgrins] from comment #21)
> In addition to Comment 20, what happens if we have this DOM:
> 
> <html>
>   <body>
>     <div />
>   </body>
> </html>
> 
> Then set :hover on <div>, then unset :hover on <body>?  Should the lock
> chain be removed all the way down to the <div> or just from <body>?  What
> happens with <html> in that case?
+1 for removing the lock chain all the way down to <div>. Removing the lock just on <body> would remove it on <html> too (at least that's what it does now) and would leave it on <div>. This, I think, looks weird.

Imagine the following test page:

<style> 
div { width:100px;height:100px;padding:10px;border:2px solid red; }
div:hover { background:green; }
</style>
<div>
  <div>
    <div></div>
  </div>
</div>

Now, if you move your mouse over the 3rd div, the 2 parent divs also turn green, which is normal, ancestors also get hovered. In fact it's not possible, using the page, to only make the 3rd div turn green.
That's why I think having something in the inspector that allows you to do this doesn't seem very useful.
Chromedevtools allows you to lock individual elements only which feels wrong to me. With the example above, you'd be able to make the 3rd div only green (this is why they only show the lock icon in the gutter next to the one element that's locked).

Today, with our inspector, you can lock the 3rd div, and then remove the lock on the 2nd div, which ends up doing what chrome does. I don't think we should keep on doing this, but instead maintain a reference to which node has been locked, and when unlocking from anywhere in the hierarchy, then unlock that node instead.

Similarly, when another node is being locked, we should unlock the currently locked node.

Comment 23

[Casper Braschi]

> Shouldn't we allow only one pseudo class lock at a time? Under real
> conditions, there can only be one hovered element hierarchy, only one active
> or focused element.

You can have a focused element - say a input:text - in one place and a "hoverable" element anywhere else.

http://jsfiddle.net/c9zah46h/

Comment 24

[Patrick Brosset <:pbro>]

(In reply to luis.eduardo.braschi from comment #23)
> > Shouldn't we allow only one pseudo class lock at a time? Under real
> > conditions, there can only be one hovered element hierarchy, only one active
> > or focused element.
> 
> You can have a focused element - say a input:text - in one place and a
> "hoverable" element anywhere else.
> 
> http://jsfiddle.net/c9zah46h/
Correct, so we should allow one locked element per state.

Comment 25

[Sebastian Zartner [:sebo]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #22)
> (In reply to Brian Grinstead [:bgrins] from comment #20)
> > Created attachment 8658910 [details] [diff] [review]
> > pseudo-class-stay-WIP.patch
> > 
> > WIP, needs test changes.  Mostly wondering how this should interact if you
> > enable multiple pseudo class locks on the page (there ends up being dots in
> > the markup view everywhere).
> Shouldn't we allow only one pseudo class lock at a time? Under real
> conditions, there can only be one hovered element hierarchy, only one active
> or focused element.
> My assumption was that locking a new node would remove the lock on whatever
> previous node was locked.
> 
> (In reply to Brian Grinstead [:bgrins] from comment #21)
> > In addition to Comment 20, what happens if we have this DOM:
> > 
> > <html>
> >   <body>
> >     <div />
> >   </body>
> > </html>
> > 
> > Then set :hover on <div>, then unset :hover on <body>?  Should the lock
> > chain be removed all the way down to the <div> or just from <body>?  What
> > happens with <html> in that case?
> +1 for removing the lock chain all the way down to <div>. Removing the lock
> just on <body> would remove it on <html> too (at least that's what it does
> now) and would leave it on <div>. This, I think, looks weird.

Note that I created bug 1120399 asking to change the APIs to work like when using the page.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #24)
> (In reply to luis.eduardo.braschi from comment #23)
> > > Shouldn't we allow only one pseudo class lock at a time? Under real
> > > conditions, there can only be one hovered element hierarchy, only one active
> > > or focused element.
> > 
> > You can have a focused element - say a input:text - in one place and a
> > "hoverable" element anywhere else.
> > 
> > http://jsfiddle.net/c9zah46h/
> Correct, so we should allow one locked element per state.

Agree. Though again, I believe this should all be handled by DOMUtils.addPseudoClassLock() and .removePseudoClassLock().

Sebastian

Comment 26

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: Screen Shot 2015-09-16 at 10.02.01 AM.png

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #22)
> (In reply to Brian Grinstead [:bgrins] from comment #21)
> > In addition to Comment 20, what happens if we have this DOM:
> > 
> > <html>
> >   <body>
> >     <div />
> >   </body>
> > </html>
> > 
> > Then set :hover on <div>, then unset :hover on <body>?  Should the lock
> > chain be removed all the way down to the <div> or just from <body>?  What
> > happens with <html> in that case?
> +1 for removing the lock chain all the way down to <div>. Removing the lock
> just on <body> would remove it on <html> too (at least that's what it does
> now) and would leave it on <div>. This, I think, looks weird.
> 
> ...
> 
> Today, with our inspector, you can lock the 3rd div, and then remove the
> lock on the 2nd div, which ends up doing what chrome does. I don't think we
> should keep on doing this, but instead maintain a reference to which node
> has been locked, and when unlocking from anywhere in the hierarchy, then
> unlock that node instead.
> 
> Similarly, when another node is being locked, we should unlock the currently
> locked node.

I agree with Patrick that you shouldn't be able to remove states arbitrarily from different points in the chain without referencing it in any way.

I think that being able to unapply pseudo classes on ancestors adds unnecessary complexity, though. My suggestion would be to see that both <body> and <html> had a hover state if a :hover class was applied to an <h1>, but grayed out in the menu without being interact-able (so, you'd need to find the correct child to unapply the state).

Attached a screenshot of what it currently looks like where I think it makes sense to gray out instead.

Comment 27

[friedrich]

Oh nice, finally fixed ;) 
ANd works great and as expected in FF43.0.

THX alo Brian Grinstead

Comment 28

[friedrich]

thats not right .... i see the behaviour haven't changed.. sry for spamming :(

Comment 29

[myself]

It is impossible to inspect the targetet elements when working with sibling selectors (+, ~) and pseudo-classes.

Take a look at this example: https://jsfiddle.net/mtb9egyp/
There's a selector that shows the Element .LocationConnector when a sibling <a> is hovered.

.LocationsSelection a:hover + .LocationConnector { display:block; }

Since the .LocationConnector is not part of <a>'s tree, I am unable to inspect .LocationConnector after I activated the :hover pseudo class on <a>. Surely, dislay:block isn't something that needs inspecting but I had no other example at hand.

Any chances this will be fixed sometime soon? I switched from Firebug to the native Developer Tools but Firebug is able to keep the selected pseudo-classes when you select a different node.

Comment 32

[Erik Krause]

It's almost impossible to style mouse-over menus now. Please fix as soon as possible!

Comment 33

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52d2b2b362c6

Comment 34

[Alexandre Poirot [:ochameau]]

Attachment: Bug 985517 - Keep pseudo class locked when selecting another node in the markup view.

Review commit: https://reviewboard.mozilla.org/r/90076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90076/

Comment 35

[Alexandre Poirot [:ochameau]]

Attachment: Bug 985517 - Only allow one node to be pseudo class locked per pseudo.

Review commit: https://reviewboard.mozilla.org/r/90078/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90078/

Comment 36

[Alexandre Poirot [:ochameau]]

Attachment: Bug 985517 - Remove pseudo class locks of children to prevent ending up with broken pseudo class state.

Review commit: https://reviewboard.mozilla.org/r/90080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90080/

Comment 37

[Alexandre Poirot [:ochameau]]

I tried to implement something that looks like what was described between comment 22 and comment 24.
But without depending on a behavior change from DOMUtils (bug 1120399).

Comment 38

[Alexandre Poirot [:ochameau]]

When toggling on a new pseudo, we reset all nodes locked for this pseudo.
Same when toggling off, we reset the children. But as we also happen to reset the parents, we reset all of them.

Comment 39

[Patrick Brosset <:pbro>]

I've tested these changes locally. I think they're great. I tried all test cases that have been provided in this bug and they all work fine now. So I think this is the right solution. At least it makes working with pseudo-class locks a lot easier.

I like the fact that now that we have a visual indicator for the lock in the gutter that goes all the way up the ancestors list, it's very easy to unlock the state from anywhere. Also, the locks go away when devtools is closed which is nice.

I noticed one weird edge case though, when iframes are involved:
- go to http://jsfiddle.net/bgrins/9QVvw/7/
- inspect one of the child elements in the page
- lock :hover on it
=> you should see that the pseudo-class lock icon is displayed in the gutter for all of its ancestors, *including* the #document element, iframe element and the ancestors above that

This is incorrect because in practice this doesn't happen. But I think we should keep it, because it makes really easy to find where an element has been locked in the DOM.

However, here is the edge case:
- scroll back up to the top of the inspector
- right-click on the root <html> element and unlock :hover
=> you should see that all descendants of this element are now unlocked but this stops within the iframe.
Within the iframe, the elements keep their pseudo-class lock and the icon in the gutter stays.

If we consider the icon as a visual way to know where there is a pseudo-class lock in the tree rather than a realistic representation of where the pseudo-class is actually locked, then unlocking from any of them should work, even through iframes.

Comment 40

[Patrick Brosset <:pbro>]

Comment on attachment 8806752 [details]
Bug 985517 - Keep pseudo class locked when selecting another node in the markup view.

https://reviewboard.mozilla.org/r/90076/#review90114

::: devtools/client/inspector/inspector.js
(Diff revision 1)
>      this.breadcrumbs = new HTMLBreadcrumbs(this);
>  
>      this.walker.on("new-root", this.onNewRoot);
>  
>      this.selection.on("new-node-front", this.onNewSelection);
> -    this.selection.on("before-new-node-front", this.onBeforeNewSelection);

Turns out the `before-new-node-front` isn't used anywhere anymore. But addons could be using it, so maybe let's leave it in?

Comment 41

[Patrick Brosset <:pbro>]

Comment on attachment 8806753 [details]
Bug 985517 - Only allow one node to be pseudo class locked per pseudo.

https://reviewboard.mozilla.org/r/90078/#review90148

Comment 42

[Patrick Brosset <:pbro>]

Comment on attachment 8806754 [details]
Bug 985517 - Remove pseudo class locks of children to prevent ending up with broken pseudo class state.

https://reviewboard.mozilla.org/r/90080/#review90150

Comment 43

[Alexandre Poirot [:ochameau]]

(In reply to Patrick Brosset <:pbro> from comment #40)
> Comment on attachment 8806752 [details]
> Bug 985517 - Keep pseudo class locked when selecting another node in the
> markup view.
> 
> https://reviewboard.mozilla.org/r/90076/#review90114
> 
> ::: devtools/client/inspector/inspector.js
> (Diff revision 1)
> >      this.breadcrumbs = new HTMLBreadcrumbs(this);
> >  
> >      this.walker.on("new-root", this.onNewRoot);
> >  
> >      this.selection.on("new-node-front", this.onNewSelection);
> > -    this.selection.on("before-new-node-front", this.onBeforeNewSelection);
> 
> Turns out the `before-new-node-front` isn't used anywhere anymore. But
> addons could be using it, so maybe let's leave it in?

I looked at DXR for addons and haven't found any usage. I'll remove it.

Comment 44

[Alexandre Poirot [:ochameau]]

Comment on attachment 8806752 [details]
Bug 985517 - Keep pseudo class locked when selecting another node in the markup view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90076/diff/1-2/

Comment 45

[Alexandre Poirot [:ochameau]]

Comment on attachment 8806753 [details]
Bug 985517 - Only allow one node to be pseudo class locked per pseudo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90078/diff/1-2/

Comment 46

[Alexandre Poirot [:ochameau]]

Comment on attachment 8806754 [details]
Bug 985517 - Remove pseudo class locks of children to prevent ending up with broken pseudo class state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90080/diff/1-2/

Comment 47

[Alexandre Poirot [:ochameau]]

Comment on attachment 8806752 [details]
Bug 985517 - Keep pseudo class locked when selecting another node in the markup view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90076/diff/2-3/

Comment 48

[Alexandre Poirot [:ochameau]]

Comment on attachment 8806753 [details]
Bug 985517 - Only allow one node to be pseudo class locked per pseudo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90078/diff/2-3/

Comment 49

[Alexandre Poirot [:ochameau]]

Comment on attachment 8806754 [details]
Bug 985517 - Remove pseudo class locks of children to prevent ending up with broken pseudo class state.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90080/diff/2-3/

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b708de08fba
Keep pseudo class locked when selecting another node in the markup view. r=pbro
https://hg.mozilla.org/integration/autoland/rev/3fdf9f2b81f2
Only allow one node to be pseudo class locked per pseudo. r=pbro
https://hg.mozilla.org/integration/autoland/rev/0c810988abdc
Remove pseudo class locks of children to prevent ending up with broken pseudo class state. r=pbro

Comment 51

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

https://hg.mozilla.org/mozilla-central/rev/9b708de08fba
https://hg.mozilla.org/mozilla-central/rev/3fdf9f2b81f2
https://hg.mozilla.org/mozilla-central/rev/0c810988abdc

Comment 52

[Sebastian Zartner [:sebo]]

I can confirm that this is working as expected in Nightly 52.0a1 (2016-11-11), while it was not working in Firefox 49.0.2.
Thank you for the fix!

Sebastian