Open Bug 734934 Opened 9 years ago Updated 2 years ago

[rule view] ability to disable a rule

Categories

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

13 Branch
enhancement

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: sys.sgx, Assigned: gl)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(3 files)

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.
Component: Untriaged → Developer Tools: Inspector
OS: Windows Vista → All
Hardware: x86 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Style Inspector - Style Rules panel enable rules at will → ability to disable a rule
So a checkbox next to the rule itself to enable / disable all that rule's properties.
yep. and if there's a more visually-enhanced other way, it'd be better :)
(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.
QA Contact: untriaged → developer.tools.inspector
Summary: ability to disable a rule → [rule view] ability to disable a rule
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
Bug triage. Filter on CLIMBING SHOES
Priority: -- → P3
Assignee: nobody → hodginsl2
Status: NEW → ASSIGNED
Attachment #8923683 - Flags: review?(gl) → review?(pbrosset)
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.
Attachment #8923683 - Flags: review?(pbrosset)
Attached image disable-rule.png
Here's how it looks with the patch. I'm asking around on Slack for suggestions.
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?
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
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.
(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.
(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
(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.)
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.
Severity: normal → enhancement
Attached image 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
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
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"
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 on attachment 8923683 [details]
Bug 734934 - [rule view] ability to disable a rule

Clearing review for now until we get the new design implemented
Attachment #8923683 - Flags: review?(gl)
Product: Firefox → DevTools
Assignee: hodginsl2 → nobody
Blocks: dt-rules-wow
Status: ASSIGNED → NEW
Assignee: nobody → gl
Status: NEW → ASSIGNED
You need to log in before you can comment on or make changes to this bug.