1213767 - Make it easier to toggle classes on elements in the inspector

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Patrick Brosset <:pbro>]

This idea was filed: https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/10169850-allow-toggling-classes-in-dom-inspector-via-checkb

Also see this twitter conversation: https://twitter.com/simevidas/status/653423558018924544

The use case is: I want to be able to quickly (1 click?) toggle a class on a DOM element displayed in the inspector.

Comment 1

[Grisha Pushkov]

Reference to Chrome's implementation:
https://twitter.com/malyw/status/666963686691577856

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Grisha Pushkov from comment #1)
> Reference to Chrome's implementation:
> https://twitter.com/malyw/status/666963686691577856
Nice, I just saw that too. We should be careful with cases where class names are long and when there are many.

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Filter on CLIMBING SHOES

Comment 4

[Patrick Brosset <:pbro>]

I have started to work on this the other day and have a patch almost ready. Not sure how much I can commit to working on it this week, but I'll try.

Comment 5

[Patrick Brosset <:pbro>]

A few notes about my approach for this:

In terms of design and UX:
- I introduced a new icon next to the pseudo-class icon. This icon toggles a panel similar to that of the pseudo-class. I did not have an icon for this, nor an idea for one, so I ended up using the string '.cls'. This is what Chrome does too, so people should recognize it.
- The panel contains a list of checkboxes, one for each class on the node, as well as a textfield where you can type any string and then press enter, in order to add new classes. In terms of design, it's sort of hard to find something that looks good. There's already a text field above it (to search css), which is why I made the new one look a bit different, so it would be easy to differentiate the 2. Open to suggestions here.
- I couldn't make the new panel appear with an animation easily, like the pseudo-class panel one. The reason is that it's really hard to do such an animation when you don't know the height of the panel. You don't know where you're animating from and to. I've tried a number of approaches, none of them worked. One compromise could be to animate for a fix distance, like 50px or so, even if the panel ends up being taller than that, there are chances you wouldn't notice the difference. This is also what Chrome does. I did not think this was too much of a big deal, but open to comments here.
- I made it so that you could not open the pseudo-class and class panels at the same time. So opening one will close the other.

Here's a screencast: https://dl.dropboxusercontent.com/u/714210/class-panel.gif

In terms of implementation:
- I did not use React for the front-end. The reason being that this piece of UI is a very small part of the rule-view, which does not use React at all. I felt like embedding a small React component inside something we will eventually redo as React wasn't worth the trouble.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: Bug 1213767 - Rule-view class toggle panel;

Review commit: https://reviewboard.mozilla.org/r/117582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117582/

Comment 7

[Patrick Brosset <:pbro>]

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=126841fcb2ba5a60f33f8390976d669d774136b5&group_state=expanded
(I used artifact build, which is maybe why some builds failed, and I need to fix a tiny ESLint error, other than that it looks green).

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8843987 [details]
Bug 1213767 - Rule-view class toggle panel;

https://reviewboard.mozilla.org/r/117582/#review119540

Looks great!

One keyboard navigation issue to address before landing: when the classPanel is hidden and contains checkboxes, they are still part of the keyboard navigation (need to tab through them to go from the ".cls" button to the rules).

Some comments about the behavior, for discussion and potential follow ups:
- it would be nice if the input received the focus when clicking on the ".cls" button
- toggling classes is not part of the undo/redo stack
- when the element has no class defined, maybe display "No class on this element" where the checkboxes will go

::: devtools/client/inspector/rules/views/class-list-previewer.js:12
(Diff revision 1)
> +const EventEmitter = require("devtools/shared/event-emitter");
> +const {LocalizationHelper} = require("devtools/shared/l10n");
> +
> +const L10N = new LocalizationHelper("devtools/client/locales/inspector.properties");
> +
> +const CLASSES = new WeakMap();

Add a comment describing both the keys and content of this cache.

::: devtools/client/inspector/rules/views/class-list-previewer.js:23
(Diff revision 1)
> + * know which classes a given NodeFront has, and which of these are enabled and which are
> + * disabled.
> + * It also reacts to DOM mutations so the list of classes is up to date with what is in
> + * the DOM.
> + * It can also be used to enable/disable a given class, or add classes.
> + * @param {Inspector} inspector The current inspector instance.

I hate to comment on that, but if we want to be consistent with the way other inspector/ files are being reviewed, please make sure there is a blank line before @param in your JSDoc, and the description should go to the next line, aligned with the "{"

::: devtools/client/inspector/rules/views/class-list-previewer.js:56
(Diff revision 1)
> +   * The class states for the current node selection.
> +   */
> +  get currentClasses() {
> +    if (!CLASSES.has(this.currentNode)) {
> +      // Use the proxy node to get a clean list of classes.
> +      this.classListProxyNode.className = this.currentNode.className;

this.currentNode can be null, because the inspector selection can be set to null. We should guard against that.

(for instance, onBeforeNavigate nulls it, which leads to several error logs when navigating from one page to another)

::: devtools/client/inspector/rules/views/class-list-previewer.js:153
(Diff revision 1)
> +                                   target.className === this.lastStateChange.className;
> +
> +      if (!isMutationForOurChange) {
> +        CLASSES.delete(target);
> +        if (target === this.currentNode) {
> +          this.emit("current-node-class-changed", this.currentClasses);

It doesn't hurt to keep it, but for the record, the only consumer of this event is not using the argument emitted here.

::: devtools/client/inspector/rules/views/class-list-previewer.js:166
(Diff revision 1)
> + * This UI widget shows a textfield and a series of checkboxes in the rule-view. It is
> + * used to toggle classes on the current node selection, and add new classes.
> + * @param {Inspector} inspector The current inspector instance.
> + * @param {DomNode} containerEl The element in the rule-view where the widget should go.
> + */
> +function ClassListPreviewer(inspector, containerEl) {

In this constructor, could you group the function bindings together, group the event listeners together and move them after the initial DOM creation/manipulation.

It makes it easier to check for flaws in the removal of event listeners (also it's the structure we use in many classes atm)

::: devtools/client/inspector/rules/views/class-list-previewer.js:197
(Diff revision 1)
> +  this.model.on("current-node-class-changed", this.onCurrentNodeClassChanged);
> +}
> +
> +ClassListPreviewer.prototype = {
> +  destroy() {
> +    this.inspector.selection.off("new-node-front", this.onNewSelection);

Can you group the removed event listeners together ?

::: devtools/client/inspector/rules/views/class-list-previewer.js:257
(Diff revision 1)
> +  onKeyPress(event) {
> +    if (event.key === "Enter" && this.addEl.value !== "") {
> +      this.model.addClassName(this.addEl.value).then(() => {
> +        this.render();
> +        this.addEl.value = "";
> +      }, e => console.error(e));

minor detail, but the call to render() will fail if the instance was destroyed. We usually don't log errors in similar situations in inspector classes.

::: devtools/client/themes/rules.css:55
(Diff revision 1)
>    display: flex;
>  }
>  
> -#pseudo-class-panel {
> +.ruleview-reveal-panel {
>    display: flex;
>    height: 24px;

Maybe move this hardcoded height to a dedicated #pseudo-class-panel:not([hidden]) rule and remove the height: unset from the #ruleview-class-panel selector?

::: devtools/client/themes/rules.css:601
(Diff revision 1)
>    background-image: url("chrome://devtools/skin/images/pseudo-class.svg");
>    background-size: cover;
>  }
>  
> +#class-panel-toggle::before {
> +  content: ".cls";

will be displayed as "cls." in rtl. Force direction: ltr; on this element.

on linux the font-size of this element is computed to 14px, which makes it a bit too big. Maybe set it to 11px.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8843987 [details]
Bug 1213767 - Rule-view class toggle panel;

https://reviewboard.mozilla.org/r/117582/#review119582

Clearing the review request for now, will take a second quick look after the comments are addressed. Thanks!

Comment 10

[Julian Descottes [:jdescottes]]

(flagging qe+ and dev-doc-needed, seems appropriate here)

Comment 11

[Patrick Brosset <:pbro>]

Attachment: class-panel.diff

Thank you Julian for the review. I think I have addressed all your comments. Also, the field gets focused automatically now, and there is a message when no classes exist.
I did not, however, implement undo/redo. If you think that's ok, I can look at that in another bug.

Sorry for using bugzilla for this incremental review, but I'm getting 500 server errors on mozreview.

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8844889 [details] [diff] [review]
class-panel.diff

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

Great work, this will be a neat addition to the rule view!

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to Patrick Brosset <:pbro> from comment #11)
> Created attachment 8844889 [details] [diff] [review]
> class-panel.diff
> 
> Thank you Julian for the review. I think I have addressed all your comments.
> Also, the field gets focused automatically now, and there is a message when
> no classes exist.
> I did not, however, implement undo/redo. If you think that's ok, I can look
> at that in another bug.
> 

Let's just log a follow-up, it doesn't prevent using undo/redo in the meantime.

Comment 14

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a74ba303d4d18fdb32c54cf9d73ff2f4eaec85d&group_state=expanded

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a66819111583
Rule-view class toggle panel. r=jdescottes

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a66819111583

Comment 17

[Ciprian Muresan [:cmuresan], Ecosystem QA]

User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID 	20170326030204

I have tested this issue using the latest Nightly on Windows 10 x64, Mac OS 10.12 and Ubuntu 14.04 x64.

While testing, I've used the Nightly Start page and this page. I've selected different nodes with classes and made sure that the checkboxes for each class show or hide their respective elements.

Patrick or Julien, do I need to go more in depth? If yes, could you please provide some scenarios?

Comment 18

[Patrick Brosset <:pbro>]

Thank you Ciprian for testing this.
You have tested the most important part of it, and sorry about not providing a scenario earlier. I can think of a couple extra test cases that might be good to cover:

- Selecting a node, unchecking a few classes, selecting another node, and then again the original node, and testing that the state of each class is preserved (unchecked classes still appear unchecked).
- Selecting a node that is getting its classes changed by javascript. You can test with this page:
data:text/html,<div class="c1 c2 c3 c4"></div><button>change classes</button><script>document.querySelector("button").addEventListener("click", function(){document.querySelector("div").className = "c5 c6 c7"})</script>
select the <div>, change a few classes, then click the button on the page to run some javascript code that will change the <div>'s classes, test that the class panel now shows the new classes.
- Trying to add new classes using the textfield inside the class panel: pressing enter should add the class, it should be possible to add multiple classes at once, duplicated classes should be de-duped.

Comment 19

[Ciprian Muresan [:cmuresan], Ecosystem QA]

User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID 	20170328030207

I have verified the above scenarios using the latest Nightly on Windows 10 x64, Mac OS 10.12 and Ubuntu 14.04 x64. 

As stated above, I've made sure that:
- the checkboxes show or hide their respective elements
- check/uncheck state is correctly remembered when switching nodes
- adding classes works as expected ( eg. adding "a b c d" will add a,b,c and d classes )
- new classes appear and are usable after they were changed by javascript
- all changes reset themselves after the page is refreshed

Patrick, thank you for providing the additional scenarios.