707740 - Ability to lock in a pseudo class in the page inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Kevin Dangoor]

People need a way to lock in pseudo-classes (like "hover") so that they can inspect the styles for the hovered state of an element on the page.

This bug is a companion to this feature page: https://wiki.mozilla.org/DevTools/Features/PseudoClassLock

These four are commonly supported by existing tools:

* active
* focus
* hover
* visited 

There are additional pseudo-classes listed in the feature page that would also be useful to support, if they're easily added.

Comment 1

[Heather Arthur [:harth]]

Attachment: WIP, pseudo-class lock in context menus

Here's a WIP. Consider it a mockup for trying out the behavior.

Exposes the pseudo-class control in the context menu of the element itself after it's highlighted, and on the infobar.

Locking a pseudo-class will apply whatever pseudo-class styles would be applied to the element if it were invoked naturally, but won't make the element go into the actual state associated with the pseudo-class (i.e. applying :focus from this won't actually focus the element). The styles will remain until the pseudo-class is removed through the same mechanism.

Selecting :visited will apply :visited styles and selecting :link will remove :visited styles and make the link appear unvisited.

Opinions about the UI wanted, and also an answer to the question: If we move on to inspect another element after we've applied a pseudo-class to the previously selected element, what should we do?

1. Remove the pseudo-class from the previously inspected element.

2. Remove the pseudo-class from the previously inspected element, and apply it to the next element inspected. Chrome does this, for a reason I'd be interested in. My guess is it makes sense if you realize that the parent is the real element with the pseudo-class style.

3. Keep the pseudo-class on the previously inspected element. This is what the patch does. This is really useful in the case that you need to inspect a "tr:hover > td" kind of rule, when you need to apply hover to an element and keep it there while you inspect another. This would involve keeping track of all of the elements with pseudo-classes applied so we could clean up after closing the inspector.

Comment 2

[Rob Campbell [:rc] (:robcee)]

this is pretty awesome.

I hadn't read c#1 before playing with this, so I had to go looking for it. I found it eventually on the infobar. Surprised to find the pseudo-classes on the element itself (but not on uninspected elements). I think it was a pleasant surprise.

Might want a separator under the list of pseudo-classes Clear all / Set all options. Possibly in a follow-up.

"If we move on to inspect another element after we've applied a pseudo-class to the previously selected element, what should we do?"

I'm not sure. It'd certainly make things somewhat simpler to deal with if we just cleared all settings on uninspect. That would also allow us to clear all pseudo-classes from the element on inspector shutdown. If we leave them applied, we have to go through the whole storage mechanism to remember what's been modified and deal with the tab selection problem which feels like a lot of extra work.

Answer to 2. Propagating pseudo-class selection to next selected element feels a bit weird, though your example use case makes sense. Pseudo-class selection then becomes a feature of the selection in a fairly strong way. We may want to do this.

Number 3 is hard for the reasons you mention and I echo above. Also, a lot of popup menus seem to be done with js and don't listen to the pseudo-classes so we get into cases where we're doing a lot of engineering for a situation that doesn't apply everywhere. Not sure it's worth the effort to do that here and if we do decide to do it, I think it would make more sense to apply that sort of mechanism to the debugger.

Comment 3

[Heather Arthur [:harth]]

(In reply to Rob Campbell [:rc] (robcee) from comment #2)
> I hadn't read c#1 before playing with this, so I had to go looking for it. I
> found it eventually on the infobar.

It's pretty out of the way in context menus, yeah. I think that's okay and if not really discoverable, it's at least remember-able because it's close to where you expect the UI to change. But I can be convinced.
 
> Might want a separator under the list of pseudo-classes Clear all / Set all
> options. Possibly in a follow-up.

Good call. Though with "set all", :visited and :link would be at odds.

> Answer to 2. Propagating pseudo-class selection to next selected element
> feels a bit weird, though your example use case makes sense. Pseudo-class
> selection then becomes a feature of the selection in a fairly strong way. We
> may want to do this.

Starting to think this fits in with the Chrome UI (pseudo-class control is on the global CSS rules panel), but not with this UI if we choose to go with it. Showing it infobar makes it feel element-specific.

> Number 3 is hard for the reasons you mention and I echo above. I think it would make
> more sense to apply that sort of mechanism to the debugger.

Ooh, that's an interesting thought.

Comment 4

[Heather Arthur [:harth]]

Attachment: WIP, pseudo-class lock in context menus on element and breadcrumbs

> "If we move on to inspect another element after we've applied a pseudo-class
> to the previously selected element, what should we do?"

There's also a fourth option that we could consider, which this patch implements:

4) Keep the pseudo-class on the previously inspected element, if it's in the hierarchy chain of the newly inspected element. In other words, remove the pseudo-class from the previously inspected element if that element is removed from the breadcrumbs upon inspecting the next element. So which elements have pseudo-class locks is always represented in the breadcrumbs.

This would clean up after itself nicely, getting rid of that problem with 3. It would also allow inspecting "tr:hover > td" rules. Doesn't allow inspecting "td:hover ~ td" rules though, if that's an important use case to cover.

Comment 5

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 584447 [details] [diff] [review]
WIP, pseudo-class lock in context menus on element and breadcrumbs

I realize this is a WIP, just doing a driveby while testing this out.

Modifications to nsContextMenu.js have some fun testing implications. Something to watch out for when getting this ready to go.

in inspector.jsm:
 
     aParent.appendChild(container);
+    
+    nodeInfobar.onclick = (function _onInfobarRightClick(aEvent) {
+      if (aEvent.button == 2) {
+        this.openPseudoClassMenu();
+      }

watch for trailing spaces. There are a few in this patch.

...

+  togglePseudoClass: function IUI_togglePseudoClass(aPseudoClass)
+  {
+    var flag = this.pseudoClasses[aPseudoClass];
+

s/var/let/

Element.h

not sure how the owners of content will feel about defining those functions in the header, but they're pretty simple, so I don't see any harm in it. Will need a peer review, obviously.

inIDOMUtils.idl

you realize touching these files means you get to keep them forever, right? ;)

looking good!

Comment 6

[Rob Campbell [:rc] (:robcee)]

(In reply to Heather Arthur [:harth] from comment #4)
> Created attachment 584447 [details] [diff] [review]
> WIP, pseudo-class lock in context menus on element and breadcrumbs
> 
> > "If we move on to inspect another element after we've applied a pseudo-class
> > to the previously selected element, what should we do?"
> 
> There's also a fourth option that we could consider, which this patch
> implements:
> 
> 4) Keep the pseudo-class on the previously inspected element, if it's in the
> hierarchy chain of the newly inspected element. In other words, remove the
> pseudo-class from the previously inspected element if that element is
> removed from the breadcrumbs upon inspecting the next element. So which
> elements have pseudo-class locks is always represented in the breadcrumbs.
> 
> This would clean up after itself nicely, getting rid of that problem with 3.
> It would also allow inspecting "tr:hover > td" rules. Doesn't allow
> inspecting "td:hover ~ td" rules though, if that's an important use case to
> cover.

This is a nice solution, I think. Feels right.

Comment 7

[Paul Rouget [:paul]]

This feature should be located in the Node Menu. bug 717919

Comment 8

[Kevin Dangoor]

I just tried the try build from Jan 3rd. Looks good and seems to work well for a single element. The behavior got a bit weirder when working with "pure css menus". I tried it out here:

http://www.cssplay.co.uk/menus/dd_valid.html

It was fiddly, but I did actually get it to a point at one time where I had a submenu opened and could see the hovered item in the submenu. That was very cool, and the other tools don't allow that at all, as far as I have seen.

The breadcrumbs seem like a nice way to handle that case (viewing hover state of an element that is only visible when it has a parent element that is also hovered).

Does it make sense to be able to set the hover state in both the breadcrumbs and the node menu as Paul mentions in comment 7?

I think the node menu will be a bit easier to discover and use for the single element case, but the breadcrumbs may be nice in the nested menu case.

Comment 9

[Heather Arthur [:harth]]

(In reply to Kevin Dangoor from comment #8)
> http://www.cssplay.co.uk/menus/dd_valid.html
> 
> It was fiddly, but I did actually get it to a point at one time where I had
> a submenu opened and could see the hovered item in the submenu.

That one in particular seemed fiddly because the :hover rule is set on an <li> with an <a> child that takes up all the space under it. So it's hard to tell that the parent is the real element with the :hover rule.

This occurs sometimes and we could consider setting :hover on all the ancestors when setting :hover on an element. After all, that's what happens essentially with real hover.

> Does it make sense to be able to set the hover state in both the breadcrumbs
> and the node menu as Paul mentions in comment 7?

Yes, I really think it's the way to go. If you're trying to figure out anything with an ancestor/parent pseudo-class rule, then it's easier to play around with and get the hierarchy right if it's on the breadcrumbs too.

Comment 10

[Heather Arthur [:harth]]

Attachment: Pseudo-class lock in infobar context menu

This patch adds a pseudo-class locking mechanism to the highlighter. It allows you to lock :hover, :active, and :focus styles on the currently highlighted element.

If you lock :hover or :active, that pseudo-class lock will also be applied to every ancestor. This mirrors what actually happens when you change these states and allows you to debug the CSS menu case with :hover or :active rules in parents.

The locks will be cleared when you inspect a new node using the highlighter and when you close the inspector, but not if you navigate using the breadcrumbs.

In this patch the locking mechanism is a context menu on the infobar. When bug 717916 is fixed, this will be in the node menu instead.

Comment 11

[Dave Camp (:dcamp)]

Comment on attachment 600529 [details] [diff] [review]
Pseudo-class lock in infobar context menu

Review of attachment 600529 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/highlighter/inspector.jsm
@@ +45,5 @@
>  const Cc = Components.classes;
>  const Cu = Components.utils;
>  const Ci = Components.interfaces;
>  const Cr = Components.results;
> +const Cc = Components.classes;

Cc is re-defined here.

@@ +578,5 @@
>      this.highlighter.lock();
>    },
>  
>    /**
>     * Select an object in the tree view.

If you're updating the patch would you mind replacing "tree view" with "inspector" in this comment please?

@@ +621,5 @@
> +   * Toggle the pseudo-class lock on the currently inspected element.
> +   * 
> +   * @param aPseudo the pseudo-class lock to toggle, e.g. ":hover"
> +   */
> +  togglePseudoClassLock: function IUI_togglePseudoClassLock(aPseudo)

In comment 10 you say "If you lock :hover or :active, that pseudo-class lock will also be applied to every ancestor. This mirrors what actually happens when you change these states and allows you to debug the CSS menu case with :hover or :active rules in parents." - might be nice to include that in this comment.

@@ +628,5 @@
> +      this.breadcrumbs.nodeHierarchy.forEach(function(crumb) {
> +        DOMUtils.removePseudoClassLock(crumb.node, aPseudo);
> +      });
> +    }
> +    else {

This else brace doesn't match file style.

::: browser/devtools/highlighter/test/browser_inspector_pseudoclass_lock.js
@@ +47,5 @@
> +
> +function selectNode()
> +{
> +  Services.obs.removeObserver(selectNode,
> +    InspectorUI.INSPECTOR_NOTIFICATIONS.OPENED, false);

Is the false argument correct here?

Comment 12

[Heather Arthur [:harth]]

http://hg.mozilla.org/integration/fx-team/rev/309a6e9d0127

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/309a6e9d0127

Comment 14

[Joe Walker [:jwalker]]

Comment on attachment 600529 [details] [diff] [review]
Pseudo-class lock in infobar context menu

Review of attachment 600529 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/highlighter/highlighter.jsm
@@ +63,5 @@
>    "title": true,
>  };
>  
> +const PSEUDO_CLASSES = [":hover", ":active", ":focus"];
> +  // add ":visited" and ":link" after bug 713106 is fixed

Do we need to add that there is a similar list in inspector.jsm?