734934 - [rule view] ability to disable a rule

Status: ASSIGNED

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

Description

[sys.sgx]

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:13.0) Gecko/20120312 Firefox/13.0a1
Build ID: 20120312031136

Steps to reproduce:

There should be a way to "comment" all rules in a css selector of the Style Rules panel, instead of clicking on each item.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

So a checkbox next to the rule itself to enable / disable all that rule's properties.

Comment 2

[sys.sgx]

yep. and if there's a more visually-enhanced other way, it'd be better :)

Comment 3

[Paul Rouget [:paul]]

(In reply to Michael Ratcliffe from comment #1)
> So a checkbox next to the rule itself to enable / disable all that rule's
> properties.

Let's make sure this feature doesn't make the UI clunky. I would suggest to wait for the new UI (bug 712469) and see how this can fit in it.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Bug triage. Filter on CLIMBING SHOES

Comment 5

[Liam Hodgins]

Attachment: Bug 734934 - [rule view] ability to disable a rule

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

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8923683 [details]
Bug 734934 - [rule view] ability to disable a rule

https://reviewboard.mozilla.org/r/194808/#review201328

I love this feature, very cool.
The code is also pretty simple.

I have 2 comments that I think we should address before shipping this:

- There's no test in this patch. We need an automated integration test to cover this new feature. There are many tests you can use as inspiration to write a new one for this. In particular you can look at the testDisableProperty function in devtools\client\inspector\rules\test\browser_rules_edit-property_02.js. I suggest creating a new test file in the same directory.
- Secondly, I'm a bit hesitant about the visual design for this. Using a checkbox seems like the right thing to do here, like for props, but the place where you've put it messes up with the alignment of things a bit.
Indeed, rules used to be formatted just like css rules in a text editor would. With the start of the selector being vertically aligned with the closing brace. Now the selector is indented a bit, and because the checkbox isn't always visible, that looks a bit weird to me.
In safari, they have this feature too, they use an icon button for this, at the same location. But it looks ok because it's always visible, but also because they don't show the curly braces.
I don't know what the right solution here is, but I do think we need to iterate a bit on this.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: disable-rule.png

Here's how it looks with the patch. I'm asking around on Slack for suggestions.

Comment 8

[Patrick Brosset <:pbro>]

Several options:

- Indent everything so that the selector and closing brace are still aligned, and properties are still indented compared to the selector. Cons: more wasted space to the left of each rule.

- Move the checkbox next to the highlighter icon (right of the selector). Cons: not consistent with property checkboxes. Also, would we always show it, or only on hover?

Comment 9

[Patrick Brosset <:pbro>]

Some feedback on slack about this option:
> - Move the checkbox next to the highlighter icon (right of the selector).
> Cons: not consistent with property checkboxes. Also, would we always show
> it, or only on hover?
Pros: 
- the checkbox feels different for an entire rule than it does for a single property b/c it’s controlling the state of more than just the line it’s on
- we already have space for the action.  so if we put a control to both the left and the right of the selector it’s spreading out the number of places you can click around in the ui
Cons:
- it goes against the flow where you would usually play around toggling on/off properties that is usually on the left side
- the checkbox location to the right of the selector would also not be consistent because of the selector length

Comment 10

[Tom Tromey :tromey]

With this approach, the state of a property will be lost when disabling and re-enabling a rule.
That is: (1) disable a property, (2) disable the whole rule, (3) enable the rule -> the property
is now enabled.

Maybe this is ok, I don't know; but I wanted to be sure it was a conscious decision.

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Tom Tromey :tromey from comment #10)
> Maybe this is ok, I don't know; but I wanted to be sure it was a conscious
> decision.
Yeah I thought about that and decided it was OK because the rule checkbox acts like a selectall kind of thing, like a master switch if you will.

I realized another thing though: we loop through each and every property when toggling them, instead of setting all of them at once. If you disable a rule that has many properties, then it looks like several things happen over time. We might want to make this better. I'll take a closer look at this next time I review this.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Tom Tromey :tromey from comment #10)
> With this approach, the state of a property will be lost when disabling and
> re-enabling a rule.
> That is: (1) disable a property, (2) disable the whole rule, (3) enable the
> rule -> the property
> is now enabled.
> 
> Maybe this is ok, I don't know; but I wanted to be sure it was a conscious
> decision.

How difficult would it be to change the behavior so that the property stays disabled after re-enabling the rule? That would be better behavior IMO

Comment 13

[Liam Hodgins]

Comment on attachment 8923683 [details]
Bug 734934 - [rule view] ability to disable a rule

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194808/diff/1-2/

Comment 14

[Liam Hodgins]

Comment on attachment 8923683 [details]
Bug 734934 - [rule view] ability to disable a rule

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194808/diff/2-3/

Comment 15

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #12)

> How difficult would it be to change the behavior so that the property stays
> disabled after re-enabling the rule? That would be better behavior IMO

It's medium difficulty.  I think it would need extra code in the rewriter, in particular maybe near
here:
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/devtools/shared/css/parsing-utils.js#222-225

The reason for this would be to preserve the state even when selecting some
other element and then returning, or when closing and reopening the toolbox.
(These both work when disabling a single property.)

Comment 16

[Patrick Brosset <:pbro>]

Not entirely sure this is worth the effort, to be quite honest. I like the master switch approach where the rule checkbox just sets the state of all properties. I also find this more consistent with how select/unselect-all checkboxes tend to work in other UIs, like gmail's email list.

Comment 17

[Victoria Wang [:victoria]]

Attachment: deactivate rule mockup

Here's my suggestion (attached) - Safari-style icons next to the rules, possibly color-coded for inline style, classes/ids on this element, and inherited rules. The icons work like select/deselect-all checkboxes. Remove the brackets and line up the text, move the disclosure arrow to be directly in front of the values.

I know the icons are a little hard to parse here, so if we go with this UI I'd try to improve them

Comment 18

[Victoria Wang [:victoria]]

Here's another idea that uses single letter icons instead. These can be made in HTML/CSS. Disabled version of icon can be .6 opacity. The exact types of rules we want to differentiate using these letters may need more discussion.

https://mozilla.invisionapp.com/share/2XEEY0RYA#/screens/266514538_Disable_Rule_with_letter_icons

Link with Invision Inspect (may need sign in)
https://mozilla.invisionapp.com/d/main#/console/12557775/266514538/inspect

Comment 19

[Victoria Wang [:victoria]]

Oops, sorry, that was a bad link - use this one instead to see the letter icons:

https://mozilla.invisionapp.com/share/2XEEY0RYA#/266707836_Inspector_-_Disable_Rule
https://mozilla.invisionapp.com/d/main#/console/12557775/266707836/inspect

The UX critique group liked the letters for the subtle hint - they suggested that if a tooltip could appear upon hover that says ID, Class, Element, Pseudoclass, etc, that would be a nice reminder for those new to CSS. Maybe the full tooltip could say: "Class - Click to disable"

Comment 20

[Patrick Brosset <:pbro>]

Looks nice. I really like the design here. I'm however concerned with the letters. What would we show in more complex cases like:
#some-id .a-class:not(.another-class) > element

Comment 21

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

Comment on attachment 8923683 [details]
Bug 734934 - [rule view] ability to disable a rule

Clearing review for now until we get the new design implemented