Bug 152678 – Web Inspector: Add toggle-able list of classes for each element

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

When testing new functionality that involves setting classes in JavaScript, I often find myself getting annoyed at having to add and remove the desired className to see the functionality.  As such, I propose that we add a small button to the filter bar of the Styles sidebar that, when toggled, will open a list containing all the classes for the selected element, as well as the option to add a new one.  Each of these classes will have a checkbox next to it to allow the user to toggle the class on/off (which will be remembered for that node).

[Radar WebKit Bug Importer]

<rdar://problem/24034354>

[Devin Rousso]

Created attachment 268161 [details]
Patch

[Devin Rousso]

Created attachment 268162 [details]
After patch is applied

[Timothy Hatcher]

It is great you added this! The screenshot puts me off a little because it is so busy. Is that a real world case or a test case you fabricated?

It took me a while to decree what I was looking at in the screenshot. So I am not sure the UI/UX is there yet with this. For starters I don't think a separate scrollable section is needed, it could use the main scroll area of the sidebar. Or was there a reason to have it separate?

I would be tempted to put it with the toggle pseudo state checkboxes with a divider. To add a new class I would have a checkbox size/shape icon with a plus button that added an editor inline to type the class which turns into a checkbox after you type or hit enter. If it is in the main scroll area, I might just have it scrolled off by default and not have a button to show and hide them. I know Crome did it close to the way you have it implemented. I'm mostly concerned about another fixed area that takes away from the scroll region of the sidebar that you need to toggle to get more room.

[Devin Rousso]

(In reply to comment #4)
> It is great you added this! The screenshot puts me off a little because it
> is so busy. Is that a real world case or a test case you fabricated?

No this was me typing a bunch of letters and hitting enter as fast as I could :P Definitely not real world (don't think I've ever seen a site with more than ~8 classes...)

> It took me a while to decree what I was looking at in the screenshot. So I
> am not sure the UI/UX is there yet with this. For starters I don't think a
> separate scrollable section is needed, it could use the main scroll area of
> the sidebar. Or was there a reason to have it separate?

I think that it works better separately because the main use-case that I can imagine is toggling classes to see how they effect the styles in the sidebar.  If the classList was part of the main scrolling area of the sidebar, it is possible that it would prevent me from seeing other styles that are less specific (and therefore further down the scrollable area).  In addition, if I scrolled down and wanted to undo the class-toggle, having to scroll back up and back down is a bit tedious (although I may just be a bit lazy at that point).

> I would be tempted to put it with the toggle pseudo state checkboxes with a
> divider.

Same reasoning as above as to why I didn't.  I also intended it to be a bit more separated from the actual CSS content displayed (kind of like the filter bar).

> To add a new class I would have a checkbox size/shape icon with a
> plus button that added an editor inline to type the class which turns into a
> checkbox after you type or hit enter.

Ooooooh I like this idea.  I'll work on it a bit later today.

> If it is in the main scroll area, I
> might just have it scrolled off by default and not have a button to show and
> hide them. I know Crome did it close to the way you have it implemented. I'm
> mostly concerned about another fixed area that takes away from the scroll
> region of the sidebar that you need to toggle to get more room.

That is a good point.  I was testing it with the sidebar docked to the right so it may be a lot worse if docked at the bottom.

[Devin Rousso]

Created attachment 268676 [details]
After patch is applied (v2)

Instead of having an always visible input for adding new classes, show a smaller checkbox with plus icon that, when clicked, displays the input for new classes.

[Timothy Hatcher]

Looks nice! Can you a plus button look like a checkbox (plus box with a plus glyph)? And only have the plus button? That is what I was getting at in my previous comment.

[Nikita Vasilyev]

I feel ambivalent about this change.

It adds a new UI for editing class="..." attribute but
keeps the old UI still in place. I can still click
on class="..." on the DOM element and edit it as a plain
text. Now we have two different UI to do essentially
the same thing.

Maybe instead we should *replace* existing class="..."
attribute editing UI with a better one? I don't have
any solid suggestions at this point, but I thought it
could be something to explore.

[Timothy Hatcher]

I wondered about that too. What about inline checkboxes in the DOM tree? Would that be funny UX?

I'm not fully against new UI in the sidebar though, I do still have concerns about the space it takes when open.

[Nikita Vasilyev]

(In reply to comment #9)
> I wondered about that too. What about inline checkboxes in the DOM tree?
> Would that be funny UX?

Yes, I think it would create a lot of visual noise.

I was thinking of some sort of tokens:
class="[one-class] [another-one]"
Pressing delete would remove the entire class,
not just one character.

Also, classes can already be toggled in the styles
sidebar by clicking on the "{}" icon on the left to the
CSS selectors. It comments out the entire CSS rule,
which is very similar to class toggling.

> I'm not fully against new UI in the sidebar though, I do still have concerns
> about the space it takes when open.

I feel the same.

[Devin Rousso]

(In reply to comment #10)
> (In reply to comment #9)
> > I wondered about that too. What about inline checkboxes in the DOM tree?
> > Would that be funny UX?
> 
> Yes, I think it would create a lot of visual noise.

I actually think that it isn't a bad idea.  I would make it so that editing the class attribute would remove the checkboxes, but non-editing will show small checkboxes next to each class.  Since it is already pretty simple to add a new class via editing the attribute, I don't think we would need the "+".

> I was thinking of some sort of tokens:
> class="[one-class] [another-one]"
> Pressing delete would remove the entire class,
> not just one character.

I'm not sure how I feel about this...it seems to take a liberty on behalf of the user in that whenever they want to delete something it is the entire class.  What if they accidentally spelled it wrong and just wanted to change a character?

> Also, classes can already be toggled in the styles
> sidebar by clicking on the "{}" icon on the left to the
> CSS selectors. It comments out the entire CSS rule,
> which is very similar to class toggling.

To me, this is actually separate functionality.  Clicking the "{}" icon toggles that *rule*, not the class/selector it represents.  So if a class has both regular styles and a ::before, clicking the "{}" icon just removes the styles associated with the class, not the class itself. (btw I was the one who added this over the summer)

> > I'm not fully against new UI in the sidebar though, I do still have concerns
> > about the space it takes when open.
> 
> I feel the same.

No disagreements here.

[Nikita Vasilyev]

(In reply to comment #11)
> ...
> I'm not sure how I feel about this...it seems to take a liberty on behalf of
> the user in that whenever they want to delete something it is the entire
> class.  What if they accidentally spelled it wrong and just wanted to change
> a character?

This is how it's done in Mail.app:
https://cldup.com/EM8YayBb8S.gif
Double clicking on the token converts it back to the plain text.

> 
> > Also, classes can already be toggled in the styles
> > sidebar by clicking on the "{}" icon on the left to the
> > CSS selectors. It comments out the entire CSS rule,
> > which is very similar to class toggling.
> 
> To me, this is actually separate functionality.  Clicking the "{}" icon
> toggles that *rule*, not the class/selector it represents.  So if a class
> has both regular styles and a ::before, clicking the "{}" icon just removes
> the styles associated with the class, not the class itself. 

Pseudo selectors aren't very common. My guess it's usually <10% of all selectors.

> (btw I was the one who added this over the summer)

I know; I like it :)

[Devin Rousso]

(In reply to comment #12)
> (In reply to comment #11)
> > ...
> > I'm not sure how I feel about this...it seems to take a liberty on behalf of
> > the user in that whenever they want to delete something it is the entire
> > class.  What if they accidentally spelled it wrong and just wanted to change
> > a character?
> 
> This is how it's done in Mail.app:
> https://cldup.com/EM8YayBb8S.gif
> Double clicking on the token converts it back to the plain text.

Oh.  Didn't make that connection.  From what you have shown, it seems like the functionality in Mail.app is a bit different, as that token list doesn't have the "toggled" state for an address (meaning that it was once applied, but has since been removed "temporarily" by the user using a checkbox).  This patch adds the ability for developers to toggle classes, not remove them entirely.  I like the idea of making tokens, but I don't think that it fits well with the intention of this patch.

As an example, how would a token list with checkboxes display a toggled class?  What happens if the user then tries to edit (by double-clicking the token) that toggled class?  Should it edit it and reapply the class or allow the user to change its value without re-adding it?

> > 
> > > Also, classes can already be toggled in the styles
> > > sidebar by clicking on the "{}" icon on the left to the
> > > CSS selectors. It comments out the entire CSS rule,
> > > which is very similar to class toggling.
> > 
> > To me, this is actually separate functionality.  Clicking the "{}" icon
> > toggles that *rule*, not the class/selector it represents.  So if a class
> > has both regular styles and a ::before, clicking the "{}" icon just removes
> > the styles associated with the class, not the class itself. 
> 
> Pseudo selectors aren't very common. My guess it's usually <10% of all
> selectors.

Using ::before was more as an example than a point.  The biggest issue with the "{}" toggling vs what this patch does is that "{}" toggles on a rule basis while class toggling works on an *element* basis, meaning that using "{}" on a rule with a class selector will remove the styles for ALL elements matching that selector while using the class toggle just removes the styles for the SINGLE selected element.

> > (btw I was the one who added this over the summer)
> 
> I know; I like it :)

:) thanks!

[Timothy Hatcher]

Not just pseudo rules, a class name can cause multiple rules to match. So yes, clicking the icon to comment out a rule is different than toggling a class that could apply multiple rules.

[Devin Rousso]

So I'd like to get this merged as I could really use it for some web-design work I'm doing right now, so would it be possible to merge it with the v2 functionality (with the Plus checkbox instead of the visible input) and see how it goes?  Similar to what we did with merging the different Styles sidebar navigation items?

[Devin Rousso]

Created attachment 269113 [details]
Patch

[Nikita Vasilyev]

(In reply to comment #15)
> So I'd like to get this merged as I could really use it for some web-design
> work I'm doing right now, so would it be possible to merge it with the v2
> functionality (with the Plus checkbox instead of the visible input) and see
> how it goes?  Similar to what we did with merging the different Styles
> sidebar navigation items?

I think it's worth merging it in and revisiting later.

[Timothy Hatcher]

Comment on attachment 269113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269113&action=review

One issue then we can land this.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:407
> +                // RegExp to ensure that all instances of className are removed, so long as
> +                // the text is not part of another class name.
> +                let removeClassName = new RegExp(`((\\s+|^)${className}(\\s+|$)|\\s+${className}(\\s)|(\\s)${className}\\s+)`, "g");
> +                newClasses = newClasses.replace(removeClassName, "$2").trim();

Impressive RegExp! The className should be escaped if you use it in the RegExp (use escapeForRegExp). However, I think we should use classList.toggle() to be 100% safe.

To see how to call classList.toggle, see _hideElement in DOMTreeOutline.js.

[Devin Rousso]

Created attachment 269284 [details]
Patch

[Timothy Hatcher]

Comment on attachment 269284 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269284&action=review

One other issue that I didn't think about earlier.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:358
> +        let classes = this.domNode.getAttribute("class");
> +        let classToggledMap = this.domNode[WebInspector.CSSStyleDetailsSidebarPanel.ToggledClassesSymbol];
> +        if (!classToggledMap) {
> +            classToggledMap = new Map;
> +            if (classes && classes.length) {
> +                for (let className of classes.split(/\s+/))
> +                    classToggledMap.set(className, true);
> +            }
> +            this.domNode[WebInspector.CSSStyleDetailsSidebarPanel.ToggledClassesSymbol] = classToggledMap;
> +        }

Nothing keeps this up-to-date if the page change the classes on the DOM element. You would likely want to keep this map on the DOMNode inspector class and add an event listener. Or listener to the existing AttributeModified, AttributeRemoved events.

[Devin Rousso]

Created attachment 269498 [details]
Patch

[Devin Rousso]

(In reply to comment #20)
> Nothing keeps this up-to-date if the page change the classes on the DOM
> element. You would likely want to keep this map on the DOMNode inspector
> class and add an event listener. Or listener to the existing
> AttributeModified, AttributeRemoved events.

Good catch!  Didn't even think of this.  I decided to add the listeners to CSSStyleDetailsSidebarPanel because all of the rest of the class-toggling functionality is there as well.  It didn't make much sense to me to move the logic to DOMNode when it has a very specific use case in a completely separate class.  We already had some listeners being added to the DOMNode (changes to pseudo-classes), so it made sense to follow that pattern (although it was just one listener...).

[Devin Rousso]

Created attachment 269502 [details]
Patch

Just made a small optimization: since the class toggles are now updated whenever the class attribute is changed, we don't need to create a new toggle whenever the user presses enter or blurs the new class input.

[WebKit Commit Bot]

Comment on attachment 269502 [details]
Patch

Clearing flags on attachment: 269502

Committed r195432: <http://trac.webkit.org/changeset/195432>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.