1492797 - Provide autocompletion when adding a new class to an element

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, enhancement, P3)

Description

[Sebastian Zartner [:sebo]]

When adding a new class via the '.cls' button the classes existing within the page should be autosuggested while typing.

Test case:
1. Open the Inspector on data:text/html,<div class="foo"><div class="bar"></div></div><div></div>
2. Select the last <div>
3. Within the Rules panel click the '.cls' button
4. Focus the input field for adding a new class
5. Type 'f' (without the single quotes)

=> An autocompletion list should be shown suggesting 'foo' and 'bar' as class names.

This request initially came up on Stack Overflow:
https://stackoverflow.com/q/52421685/432681

Sebastian

Comment 1

[Patrick Brosset <:pbro>]

Thanks for filing Sebastian!
This sounds like a useful feature to add and a parity gap with Chrome.

I was wondering what we would do about iframes, but in this case it should be quite easy: the node that is currently selected lives in a document, so we need a way to query all classNames from that document only (no need to walk into iframes).
The other thing is we could get all classNames currently used in the DOM, but we could also get all classNames defined in stylesheets loaded in the document, but not necessarily used yet. I don't know if one is more useful than the other.

Comment 2

[Sebastian Zartner [:sebo]]

(In reply to Patrick Brosset <:pbro> from comment #1)
> Thanks for filing Sebastian!

Thank you for jumping in so quickly, Patrick! :-)

> This sounds like a useful feature to add and a parity gap with Chrome.

Should the parity-chrome flag be set?

> I was wondering what we would do about iframes, but in this case it should
> be quite easy: the node that is currently selected lives in a document, so
> we need a way to query all classNames from that document only (no need to
> walk into iframes).

Yes, iframes can be neglected in this case.

> The other thing is we could get all classNames currently used in the DOM,
> but we could also get all classNames defined in stylesheets loaded in the
> document, but not necessarily used yet. I don't know if one is more useful
> than the other.

A third option would be to merge both, class names from style sheets and the DOM. That's also what Chrome does.

Personally, I think it's most useful to use the class names of the style sheets, because that are the ones that actually style elements when applied, while class names within the DOM may not have any effect when there is no related rule.
On the other hand, one may want to apply a class name first and then add a rule styling the different elements.

When taking the class names from the style sheets, there's also the question whether to only use the ones from simple selectors like ".foo { ... }" or parse all of them, i.e. also the ones within complex selectors, e.g. ".foo > .bar { ... }", (like Chrome does it).
Or the parser might even try to be smart and show the ones first (or filter on those) that may apply to the element. I.e. if you have a rule ".foo > .bar { ... }" and an element within an element with the ".foo" class applied, the first suggestion would be ".bar". But that's probably to hard to implement.

At least for getting all used class names, there's obviously already some code behind the Inspector search field, because you can get them by typing a dot.

Sebastian

Comment 3

[Sebastian Zartner [:sebo]]

Patrick, which path do you think we should take?

1. Only class names defined in the DOM?
2. Only class names defined in the style sheets applying to the document?
3. Both, DOM and style sheets class names?

Should smart suggestions be made? And if so, should that already be integrated initially or should it be deferred?

Also, should the parity-chrome flag be set?

Sebastian

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Sebastian Zartner [:sebo] from comment #3)

Patrick, which path do you think we should take?

1. Only class names defined in the DOM?
2. Only class names defined in the style sheets applying to the document?
3. Both, DOM and style sheets class names?

I would do whatever Chrome does. This isn't a feature that we hope will set us apart from other DevTools out there. Because of this, we might as well just do what others do, so as to facilitate people switching between DevTools.

Should smart suggestions be made? And if so, should that already be integrated initially or should it be deferred?

I wouldn't try to be too smart in this first version. Listing existing class names would already be very useful to people. So we should probably focus on this first.

Also, should the parity-chrome flag be set?

Yes. I've done it.

Comment 5

[Christian]

Just a quick note from someone who uses this a lot in Chromium:

This feature is probably most useful to people who do rapid prototyping/designing in the browser via "Atomic CSS" (aka "Utility-First CSS") classes like those provided by Tachyons, Basscss, Tailwind and even recent versions of Bootstrap – something that's becoming increasingly popular in the webdev community (if you ask me, for all the right reasons!). Therefore I'd consider it rather important to provide all available CSS classes, not just those currently active in the DOM.

Thanks for considering this feature!

Comment 6

[robbprescott]

Just agreeing with Christian. This is a super useful feature when using "Atomic CSS" and rapid prototyping. It's one of the only things I switch back to Chromium to develop with.

Comment 8

[Simon Walker]

I would be happy to work on this, but I've never contributed to the firefox suite before. I would appreciate some mentoring, if possible.

Comment 9

[Patrick Brosset <:pbro>]

Thank you for your interest. The first step you should take is reading up on our contribution workflow, and installing the dev environment.
The starting point for this is: https://docs.firefox-dev.tools

Once you've done that, you should be able to build Firefox yourself, make changes, and test those changes.

One important thing that can help you through this is joining our public Slack instance and asking questions about any problems you might encounter there.
Feel free to also ask questions here as comments if you want to, Slack is just a bit better for live conversations.

I think the best way to get started here is, because this is not a small task, to get organized a little bit. Define the steps that are need to achieve the wanted result, and file one bug for each of these, so it's easy to make progress on small, actionable, tasks instead of trying to solve everything at once.
The tasks I see are:

• create an end point that can return the class names available in the document
  • maybe start with returning only the class names in the DOM, because that's easier
  • then extend it to also return classes found in the stylesheets loaded in the document
• add an auto-complete component to the class list field in the UI
• wire the auto-complete with the end point

Do these make sense?

About the end point: DevTools is based on a client-server architecture where the UI is the client, and the server is code that runs in the context of the page being inspected. The objects that are instantiated on the server-side to actually inspect the page (list the DOM nodes, scripts, stylesheets, etc.) are called Actors. So by end point I really mean Actor.
All of the actors are contained in this directory.
In particular here, we'll need to make changes to an actor that can retrieve classes from the DOM and list stylesheets. There are 2 actors that treat with these things today already:

• The WalkerActor knows how to walk the DOM and would be the right place to add a new method that retrieves all of the classes.
• The StyleSheetsActor can list all stylesheets in a document. That might be the right place to add a method that retrieves all classes too.

Thinking ahead, I know there are other places in DevTools that would benefit from such new methods. Bug 1166963, for example, defines a new auto-complete for when a selector is edited or added. And for this case, it makes sense to not only return classes, but also IDs.
So I think it would be good to think of these new methods as a little generic, so are not constrained to only classes, but could also be extended in the future to support (at least) IDs.

About the UI: For this bug, we want to add an autocomplete to the class list field in the Rules panel. The code for this field is in this file. The input is created here.
The good news is, you don't have to create an autocomplete from scratch. We have a component for that, as there are many other places in DevTools that need this too.
The Rules panel already uses it here. I'd have to do a bit more research to find out how this is instantiated and populated, but there should be many examples throughout the codebase.

I hope this is enough to get you started on one of the tasks required here. I'm assigning the bug to you now. Let me know if you have any questions (here or on #general on Slack).

Comment 10

[Simon Walker]

Hi Patrick, thank you for your detailed and valuable insights. I have a copy of the mozilla-central source code ready to go. It will take me a bit to digest what you've said, and to take a look at the code. I plan to do some reading and playing with the source, and then I think you're right: compile a list of immediate tasks in order once I have a greater understanding of the code.

I'm excited to work on this. It sounds like a lot of the groundwork already exists. I'll be on the slack when I'm working on this (Simon Walker).

Thanks again!

Comment 11

[Patrick Brosset <:pbro>]

Hi Simon, how are things going here? Do you need any help getting started? I could provide a simple patch with some code to get going.

Comment 12

[Simon Walker]

Hi Patrick, I'm still interested in working on this, however my time has been short recently. Another bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1413678) looked very similar to this which has made more progress so I can take inspiration from that. I was encouraged that my research into the approach matched the other bug's initial implementation.

Comment 13

[Romain Sickenberg]

Hello, any updates on this? This would be a realy nice feature!

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Simon, I'm taking over if you don't mind as I'll be able to devote some time to implement this.

Comment 15

[Minkul]

Hi, We really want this feature in Firefox devtools. Can we expect this in any near version soon? With class names suggestion, like Chrome; it'd be great to see the class applied for preview when scrolling suggested items with up and down arrow keys.

Comment 16

[Simon Walker]

Hi all, I'm sorry I haven't had much time to work on this. Nicolas: I'm happy for you to take over. I'll stay subscribed to keep visibility.

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1492797 - Turn ClassListPreviewer into a class. r=rcaliman.

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1492797 - Autocomplete for classes panel. r=rcaliman.

Depends on D71159

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

I pushed a WIP patch which is working, but has no tests.

The server function is designed to be able to retrieve different type of attributes, so we can reuse it in the future, for example for autocomplete in the rule view selector input.

This is both retrieving classes used in the DOM, and the one set in stylesheets.
In order to parse the stylesheet, I'm using the CSSLexer on each CSSRule#selectorText of the stylesheets. This is a bit naive, and could be slow (~100ms for 1500 rules https://perfht.ml/2XF1TI0)
We may try to tackle Bug 1410184 to replace our JS implementation of the lexer by a WASM version of our Rust lexer and see if that bring any improvement.
If that's still too slow for our taste, Emilio offered help to write a simple function on InspectorUtil that could be quite efficient.

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Here's a new profile on the bootstrap page: https://perfht.ml/2Y83BSl

We went from ~100ms (see the profile in Comment 19) to ~20ms by:

• checking if selectorText includes the filter string
• checking if selectorText includes . or [class if we're looking for classes, # or [id if we're looking for ids

before parsing the selectorText with the lexer.

For the Tachyons components page (https://tachyons.io/components/), we get similar numbers (~15ms) https://perfht.ml/2Y8mILX
There are more time spent looping from all the matching DOM elements, but it still feels reasonable

Comment 21

[Christian]

This is massively appreciated, thanks a lot! I'll definitely give FF another go as my main browser to develop with once this lands. And I'm very positive this is the last missing piece that kept me from doing so earlier. So thanks again!

Comment 22

[Nikolai Pulkkinen]

Thank you for your work, Nicolas !!!

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1492797 - Allow to pass an input to the autocomplete constructor. r=rcaliman.

This will automatically add all the necessary event listeners
for the autocomplete (navigating, selecting an item, closing).
The input will also be used as a default anchor when showing
the popup.

Depends on D71159

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7f6906f69de
Turn ClassListPreviewer into a class. r=rcaliman.
https://hg.mozilla.org/integration/autoland/rev/3ecf0fc44704
Allow to pass an input to the autocomplete constructor. r=rcaliman.
https://hg.mozilla.org/integration/autoland/rev/3bfad9588f0b
Autocomplete for classes panel. r=rcaliman,jdescottes.

Comment 25

[Razvan Maries]

Backed out for perma failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=06ad38b3eaffab8fe3b4e05194953b138aab65ee&failure_classification_id=2

Logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310529469&repo=autoland&lineNumber=5381
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310529570&repo=autoland&lineNumber=4097
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310529583&repo=autoland&lineNumber=3297

Backout: https://hg.mozilla.org/integration/autoland/rev/ae598dddd28af846e967d0e34578947c9bf2f29a

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

of course I forgot to update the phabricator revision before landing the queue 🤦‍♂️
will push an updated version soon

Comment 27

[Razvan Maries]

Another failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310535247&repo=autoland&lineNumber=2708

Comment 28

[Razvan Caliman [:rcaliman]]

Of course I forgot to trigger a try run after reviewing 🤦‍♂️. Sorry about this!

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

TV looked fine here https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3a1087d26315c2477a2d7ec0ba9a3bd16a61137&selectedTaskRun=AimMU4zDTh6W8zvXghmnLQ.0 , but I triggered a few more run just to make sure everything is okay

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/883d1e3d90b6
Turn ClassListPreviewer into a class. r=rcaliman.
https://hg.mozilla.org/integration/autoland/rev/4e8231a78205
Allow to pass an input to the autocomplete constructor. r=rcaliman.
https://hg.mozilla.org/integration/autoland/rev/2242e9d7e74e
Autocomplete for classes panel. r=rcaliman,jdescottes.

Comment 31

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/883d1e3d90b6
https://hg.mozilla.org/mozilla-central/rev/4e8231a78205
https://hg.mozilla.org/mozilla-central/rev/2242e9d7e74e

Comment 32

[Janet Swisher]

The .cls button wasn't really documented at all. I added a section about it to the "Examine and edit CSS" article, mentioning autocomplete.

Comment 33

[Sebastian Zartner [:sebo]]

A belated thank you for implementing this feature, Nicolas! I've tested it now in DevEdition 80.0b7 and Nightly 81.0a1 (2020-08-13) and it works as expected.

Sebastian