Closed Bug 1492797 Opened 6 years ago Closed 4 years ago

Provide autocompletion when adding a new class to an element

Categories

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

enhancement

Tracking

(firefox80 verified, firefox81 verified)

VERIFIED FIXED
Firefox 80
Tracking Status
firefox80 --- verified
firefox81 --- verified

People

(Reporter: sebo, Assigned: nchevobbe)

References

Details

(Keywords: dev-doc-complete, parity-chrome, Whiteboard: [dt-q])

Attachments

(3 files)

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
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.
(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

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

Flags: needinfo?(pbrosset)

(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.

Flags: needinfo?(pbrosset)
Keywords: parity-chrome

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!

Priority: -- → P3
See Also: → 1424485
Whiteboard: [dt-q]

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.

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).

Assignee: nobody → s.r.walker101
Status: NEW → ASSIGNED

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!

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.

Flags: needinfo?(s.r.walker101)

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.

Flags: needinfo?(s.r.walker101)

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

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

Assignee: s.r.walker101 → nchevobbe

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.

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.

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.

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

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!

Thank you for your work, Nicolas !!!

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

Attachment #9140973 - Attachment description: Bug 1492797 - [WIP] Autocomplete for classes panel. r=rcaliman → Bug 1492797 - Autocomplete for classes panel. r=rcaliman
Attachment #9140973 - Attachment description: Bug 1492797 - Autocomplete for classes panel. r=rcaliman → Bug 1492797 - Autocomplete for classes panel. r=rcaliman.
Pushed by nchevobbe@mozilla.com: 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.

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

Flags: needinfo?(nchevobbe)

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

Pushed by nchevobbe@mozilla.com: 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.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80
Blocks: 1654731

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

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

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

Attachment

General

Created:
Updated:
Size: