Closed Bug 1323700 Opened 7 years ago Closed 7 years ago

Add a "copy full CSS path" option to the inspector's menu

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox53 verified)

VERIFIED FIXED
Firefox 53
Tracking Status
firefox53 --- verified

People

(Reporter: pbro, Assigned: pbro)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Firebug had a feature that let you copy the full CSS path for any DOM node in the inspector.
Firefox DevTools also has this.

The 2 implements differ though.

DevTools creates a *unique* CSS selector that is useful to locate the node.
> .site-title > a:nth-child(1) > img:nth-child(1)

Firebug creates a full path which might be more useful for writing CSS rules.
> html body.html-ltr.firefox.moz-header-slim.reviews.gutter.is-impala.restyle.windows div#main-wrapper div#page.c div.header-bg div.amo-header-wrapper div.amo-header div#masthead h1.site-title a img

This has been reported multiple times in the past.
I think it makes sense to expose both options in the inspector's contextual menu.
And we can probably easily port Firebug's implementation over.
Here's Firebug implementation: https://github.com/firebug/firebug/blob/master/extension/content/firebug/lib/css.js#L442

Css.getElementCSSPath = function(element)
{
    var paths = [];

    for (; element && element.nodeType == Node.ELEMENT_NODE; element = element.parentNode)
    {
        var selector = Css.getElementCSSSelector(element);
        paths.splice(0, 0, selector);
    }

    return paths.length ? paths.join(" ") : null;
};

Css.getElementCSSSelector = function(element)
{
    if (!element || !element.localName)
        return "null";

    var label = Xml.getLocalName(element);
    if (element.id)
        label += "#" + element.id;

    if (element.classList)
    {
        for (var i=0, len=element.classList.length; i<len; ++i)
            label += "." + element.classList[i];
    }

    return label;
};

Xml.getLocalName = function(node)
{
    var name = node.localName;
    return isElementHTML(node) ? name.toLowerCase() : name;
};

This code should run in an "actor", meaning, on the server-side. 
We already have a getUniqueSelector method on the NodeActor class inside devtools\server\actors\inspector.js
It might make sense to add this code to a new method in the same class, something like getAbsoluteSelector.

If you're looking to work on this bug and are new to DevTools development, make sure you go through the contribution guide first: https://developer.mozilla.org/en-US/docs/Tools/Contributing
I think we'd be better off seeing how every browser does it and try to unify what we all do here. Chrome for example will generate the simplest possible selector to get an element's target for styling purposes.

We should collect data on what is really expected by developers when they use this "Copy CSS Path" functionality and make what we provide the best for that majority case. Having multiple options is just adding more things in that may not be really needed in many cases.

In the specific case of what Firebug provide(s/d) I find that overly-verbose and not entirely useful. I for one would spend more time removing things from that path then it would take me to just write the path I need. But that is just me and why I think collecting data in unison with other vendors and unifying the functionality here for one method would be the best long-term solution.
Severity: normal → enhancement
Priority: -- → P3
I find the existing method to copy the CSS path is too specific. In some cases this is fine, but with the whole path I can see every selector involved and edit it to style precisely what I want. This feature is really the only thing keeping me from removing Firebug.
I am missing the full CSS path facility now that I've switched away from firebug. 

A couple of comments:
1. doing what everyone else is doing, no less/no more, is following the leader, not leading.

2. The specific css path/shortest unique selector etc is not always useful for styling. 
Often one is styling an element which is one of a group. Sometimes one is looking back up the CSS path to find a particular class.  

3. The long css path is often very long, and does require editing to get to the meat. However the verbose path can be enlightening and be a trigger to reducing CSS complexity.

4. The short or unique css path generated is often useless. I have just received "#video_5 > span:nth-child(2) > a:nth-child(3)".  What I really want is "#video_5 .actions a.delete" (probably).  

5. Having two options ('Copy Unique CSS Path', and 'Copy Full CSS path') does not sound difficult or confusing.

6. Many people don't keep up with what is happening here, so I would guess that people are transferring away from Firebug quite slowly. There are a couple of recent stack overflow question about this issue (that's how I got here) [http://stackoverflow.com/questions/40343362/firefox-developer-edition-css-unique-selector] and [http://stackoverflow.com/questions/41507638/firefox-firebug-update-has-removed-css-path-facility-where-can-i-find-it]
I'm not sure how what I stated is taken as "let's follow the leader", that is absolutely *not* what was said. What I said was, "Lets *work with* other vendors to figure out the use-cases for this and decide which is the best option to provide." Having a good amount of data and input from others allows us all to build better products with less code complexity. It is OK to have an open dialog with other vendors about things so we can all work, where it makes sense, in a cohesive way for developers to have a consistent experience. I'm also not advocating that Chrome, or Edge, or Safari are in any way the best choices to go with. We should have a *discussion with them* about their knowledge of the feature and see together if it makes sense that we unify in some way here, whichever way that is.

What people don't see is there is a slippery slope of "it's just an option". Well, first its once, then its another, and over time you accumulate 50 options that almost never actually get used by the majority of developers. They exist to kinda suite the need of a handful of developers at some point and then get left behind. Meanwhile, that code still needs to exist, be maintained throughout updates to the codebase, be fixed when new things are added to the specifications if needed, etc. All leading to extra work long-term maintaining something. Now, if that something would only be used by say, 5% of users... Is it worth even having from the perspective of the people needing to maintain that codebase? Most likely not, in some cases yes this can be true but generally it isn't.

> 3. The long css path is often very long, and does require editing to get to the meat. However the verbose path can be enlightening and be a trigger to reducing CSS complexity.

I don't see why a developer would want to generate an overly-verbose selector just to then trim it down manually. It seems like a trivial thing to do to yourself when we could just provide the simplest path to start and you can work from there if you want/need something else.

> 2. The specific css path/shortest unique selector etc is not always useful for styling. 
Often one is styling an element which is one of a group. Sometimes one is looking back up the CSS path to find a particular class.  

Splitting this into two...

> The specific css path/shortest unique selector etc is not always useful for styling. 
Often one is styling an element which is one of a group.

Using a full style selector though as presented would lead to feeble code to get to that item. Which could easily change given any number of possible DOM structure changes that could happen through the lifecycle of a page or just in development. Should we be offering a feature that provides such feeble selectors? That's what a good cross-vendor discussion can shed light on against the signals everyone is hearing instead of just the signals of one vendor's user-base.

> Sometimes one is looking back up the CSS path to find a particular class.  

You aren't guaranteed to get a higher-level class from a "full" selector. So that isn't necessarily useful all the time given this use case.
Jonathan makes good point about the cost of maintaining a feature that is used only by a small subset of the user population.
Unfortunately, we do not have data about how much the copy selector feature is used.

Right now, both Chrome and Firefox let you copy a "unique selector". Edge and Safari don't seem to have anything like this. And Firebug used to have the "full path" thing.

It does indeed seem to me that "html body div div div div .class1" is probably not that useful, and someone getting this would probably immediately clean this up to ".class1" only.

So there might indeed be a solution in between those 2 features. Something that no browsers implement yet, and that would be more useful for users.

Unfortunately, this solution is not known yet, and we have received no feature ideas for it. We did receive  many complaints about the full css path not being in Firefox though, and it is something and actionable.

The maintenance cost of something like this would be low. It would be based on a piece of code that has existed in Firebug for a long time, so it's battle-tested. Automated unit tests can also easily be written for it.

So, this is fairly low cost and low risk, and there are people asking for it. So I think we should go for it. And as we do this, add telemetry probes in the code to start counting how many people use these features.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment on attachment 8827432 [details]
Bug 1323700 - Adding a copy full path option to the inspector menu;

https://reviewboard.mozilla.org/r/105120/#review106204

::: devtools/server/actors/inspector.js:651
(Diff revision 1)
>      return findCssSelector(this.rawNode);
>    },
>  
>    /**
> +   * Get the full CSS path for this node.
> +   * @return {String} A CSS selector with a part for the node and each of its ancestors.

Add a new line to separate the JS comment and the @return. Although this file is a bit inconsistent with formatting, I do believe this will be the future direction is to have a new line to seperate the @param/@return

::: devtools/shared/inspector/css-logic.js:413
(Diff revision 1)
>    }
>  
>    return selector;
>  }
>  exports.findCssSelector = findCssSelector;
> +

There is 2 new lines here. Remove 1.
Attachment #8827432 - Flags: review?(gl) → review+
Thanks for the review.
Try is green except for ESLint issues that I have fixed locally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=617cfb923b2183580610e99303e12ca21317bb93&group_state=expanded
I will land this.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76452d510db7
Adding a copy full path option to the inspector menu; r=gl
https://hg.mozilla.org/mozilla-central/rev/76452d510db7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Thank you all. Looking forward to right-clicking my way to CSS.
I have reproduced this bug with Nightly 53.0a1 (2016-12-15) (64-bit) on Widows 7 , 64 Bit !

This bug's fix is verified with latest Developer Edition (Aurora)!

Build   ID 20170124004008
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

[bugday-20170125]
As of comment 16, marking this as verified.

Sebastian
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.