Closed Bug 985517 Opened 10 years ago Closed 8 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox52 fixed)

Status:
VERIFIED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: bgrins, Assigned: ochameau)

References

Details

(Whiteboard: [devtools-ux][polish-backlog][difficulty=hard])

Attachments

(5 files)

pseudo-class-stay-WIP.patch
5.11 KB, patch
Details | Diff | Splinter Review
Screen Shot 2015-09-16 at 10.02.01 AM.png
96.93 KB, image/png
Details
Bug 985517 - Keep pseudo class locked when selecting another node in the markup view.
58 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 985517 - Only allow one node to be pseudo class locked per pseudo.
58 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 985517 - Remove pseudo class locks of children to prevent ending up with broken pseudo class state.
58 bytes, text/x-review-board-request
pbro
: review+
Details 
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
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.
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.
(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.
Priority: -- → P2
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.
(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.
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)
(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?
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
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.
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");
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.
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.
See Also: → 1120406
(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
See Also: → 987365
Summary: [markup-view] :hover pseudo lock goes away when clicking on various other elements in markup view → [markup-view] pseudo-class lock goes away when clicking on other elements in markup view
See Also: → 1120399
Depends on: 1120399
See Also: 1120399 →
Depends on: 1120406
No longer depends on: 1120399
See Also: 1120406 → 1120399
Whiteboard: [devedition-40]
Whiteboard: [devedition-40] → [devedition-40][difficulty=hard]
Whiteboard: [devedition-40][difficulty=hard] → [polish-backlog][difficulty=hard]
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.
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).
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?
Whiteboard: [polish-backlog][difficulty=hard] → [devtools-ux][polish-backlog][difficulty=hard]
(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.
> 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/
(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.
(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
(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.
See Also: → 1205354
Oh nice, finally fixed ;) 
ANd works great and as expected in FF43.0.

THX alo Brian Grinstead
thats not right .... i see the behaviour haven't changed.. sry for spamming :(
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.
Has STR: --- → yes
See Also: → 1185797
It's almost impossible to style mouse-over menus now. Please fix as soon as possible!
Assignee: nobody → poirot.alex
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).
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.
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 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?
Attachment #8806752 - Flags: review?(pbrosset) → review+
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
Attachment #8806753 - Flags: review?(pbrosset) → review+
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
Attachment #8806754 - Flags: review?(pbrosset) → review+
(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.
Pushed by apoirot@mozilla.com:
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
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
Status: RESOLVED → VERIFIED
No longer blocks: top-inspector-bugs
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: