Closed Bug 1321791 Opened 7 years ago Closed 7 years ago

[Rule View] Add indicator if parts of compound (shorthand) CSS rule are overridden

Categories

(DevTools :: Inspector: Rules, defect, P2)

52 Branch
defect

Tracking

(firefox54 verified)

VERIFIED FIXED
Firefox 54
Tracking Status
firefox54 --- verified

People

(Reporter: nachtigall, Assigned: rahulc)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 6 obsolete files)

Attached image CSS_rule_collapsed.png
STR:
I was looking for some wrong padding, but could not find it. If only parts of compound css rules like margin, padding, border or background are overridden, then it is not easy to see this with the collapsed rule. See the screenshots.

AR:
Very long searching and not finding the reason...

ER:
1. There should some kind of indication if a value inside a compound / shorthand CSS rule like padding, border, margin is partly overriden. Maybe some kind very soft strike through, or a soft sign or icon next to it
2. Compound / Shorthand CSS rules which are partly overridden should always be expanded initially. Currently they also collapsed which makes it hard to see that they have some part of it not being applied.
Attached image CSS_rule_expanded.png
Hi Jens,

Thanks for the filing this. I agree we should be doing more to indicate an overridden value for shorthand css rules.
Priority: -- → P2
Attached patch Bug1321791.patch (obsolete) — Splinter Review
Proposed patch.
Compound / Shorthand CSS rules which are partly overridden are expanded initially.
Attachment #8833707 - Flags: feedback?(gl)
Assignee: nobody → rahulch95
Status: NEW → ASSIGNED
Comment on attachment 8833707 [details] [diff] [review]
Bug1321791.patch

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

::: devtools/client/inspector/rules/views/rule-editor.js
@@ +340,5 @@
>        if (!prop.editor && !prop.invisible) {
>          let editor = new TextPropertyEditor(this, prop);
>          this.propertyList.appendChild(editor.element);
> +
> +        // auto-expands classes that are overriden

s/auto-expands/Auto-expands
Attachment #8833707 - Flags: feedback?(gl) → feedback+
Attached image CSS Rule Auto Expand (obsolete) —
We (Gabriel [:gl] and I) are currently considering three possibly solutions to the problem of partially overriden CSS shorthands being hard to find.

1 - The existing patch approach to auto-expand

The current patch auto-expands shorthand CSS rules that are partially overriden. This is fairly easy to simple and easy to understand for the user. However this sometimes leads to large auto-expansions like the one in the image and may not be good for UX. 

2 - Auto-expanding only the overwritten properties for a short hand property

This approach may lead to some confusion as to whether the actual property is expanded or not, and is not entirely intuitive why only some of the properties were initially expanded.

3 - Show the expanded version but limit it to padding, margin and border to start

Might cause more confusion while debugging, if developers believe that all overriden properties are auto-expanded.

4 - Show an icon next to the shorthand properties that are partially overriden that explains on mouse over the reason for the icon.
Flags: needinfo?(pbrosset)
Flags: needinfo?(gl)
Attached image CSS Rule Auto Expand
Reuploading PNG
Attachment #8834162 - Attachment is obsolete: true
(In reply to Rahul Chaudhary [:rahulc] from comment #5)
Hm, that's a complex one.
In some situations, I wonder if we even care that much in fact. Your screenshot is essentially this:

html{font:message-box;font-size:100%;}

And, as a user, when I read this, even if 'font' is a short-hand that contains 'font-size', I don't really care that it's partly overridden. Looking at the value: 'message-box', tells me that no font size is being defined, so I'm not going to get confused.

Jens' use case (from comment 0) might be more confusing for people though. Especially if you can't see the other padding-bottom in the rule-view.

So, for this reason, I don't really like the auto-expand of all long-hand properties.
That's a big UI change that, in some cases, may display many more properties than before, for something that isn't always a problem that needs solving. At least that's my gut feeling.

I agree with your comment that solutions 2 and 3 might be confusing too.

So that leaves us with solution 4 which might be worth trying out. Coming up with an icon for this might not be very easy though. And even if we do find one, its meaning might not be easy for people to recognize. So a tooltip would definitely be needed.
Now, other icons in the rule-view have a behavior on click. What should this one do? Just expand the properties, like the expand triangle does already? I guess so.
Flags: needinfo?(pbrosset)
I agree with Patrick in Comment 7.

> So that leaves us with solution 4 which might be worth trying out. Coming up with an icon for this might not be very easy though. And even if we do find one, its meaning might not be easy for people to recognize. So a tooltip would definitely be needed.

I think the "attention icon" as shown for "Invalid property value" is not bad (https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/images/alerticon-warning.png). But it should be weaker, so maybe just grey background instead of yellow (maybe even without the exclamation mark). Tooltip could be "Compound property is partially overridden" or similar. 

The icon should (maybe) only be visible if the CSS is collapsed, and vanish when the user expands.

> Now, other icons in the rule-view have a behavior on click. What should this one do? Just expand the properties, like the expand triangle does already? I guess so.

I think it should not have a click behaviour, because of UX consitency. The expand icon is not far away.
I talked to Patrick some more about this, but we thought doing something such as:
margin: 1px 0;
 L margin-left: 0

margin-left here would be overwritten, and the 'L' would be implying that a property within margin. Expanding behaviour would still behave as expected. Also, perhaps the margin-left should also be greyed out. This is a short version of the having the short hand property expanded. It is still hard to figure out the best UX without prototyping it and trying it out. So, I would suggest trying to implement this and seeing where we should go.
Flags: needinfo?(gl)
Gabriel, this sounds very good to me. Thanks! Much better than my idea because the user would see any overridden value immediately, without having to click on expand...
Is this something along the lines of what we are looking for?
Flags: needinfo?(pbrosset)
Flags: needinfo?(nachtigall)
Flags: needinfo?(gl)
(In reply to Rahul Chaudhary [:rahulc] from comment #11)
> Created attachment 8835899 [details]
> prototype-overriden-shorthand.JPG
> 
> Is this something along the lines of what we are looking for?
Yes this is what Gabriel suggested and discussed with me.

A quick comment about this: the ∟ icon should probably not be black though, but use a more subtle color that blends in the design a little better.
In fact, when you do expand the shorthand property, the longhand properties do not have any ∟ icon. They're just indented. Maybe indentation only would work here too, and be more consistent.

And 2 questions:
- Am I right to assume that when you expand the shorthand property, then the overridden properties disappear, and just get replaced with the normal content of the expanding section, right?
- Also, the overridden properties are not editable, correct?

I agree with comment 9 that we should try this first. Implement it as a patch that people can try, or behind a pref or something. So people can take it for a spin and determine if it's a good idea or not.
Flags: needinfo?(pbrosset)
Attached patch Bug1321791.patch (obsolete) — Splinter Review
I agree with the L being too dark, I changed to it a lighter grey that goes with the color theme. As a reply to questions in comment #12:
Yes, when you expand the shorthand it goes back to the normal content and the overriden properties disappear.
Yes, the overriden properties are uneditable.

This patch contains the code with the L, should I add another patch for the indent?
Attachment #8833707 - Attachment is obsolete: true
Flags: needinfo?(gl)
Attachment #8836721 - Flags: review?(gl)
Comment on attachment 8836721 [details] [diff] [review]
Bug1321791.patch

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

Patch needs to be rebased.
Attachment #8836721 - Flags: review?(gl)
Attached patch Bug1321791.patch (obsolete) — Splinter Review
Small update to patch.
Attachment #8836721 - Attachment is obsolete: true
Attachment #8836756 - Flags: review?(gl)
Comment on attachment 8836756 [details] [diff] [review]
Bug1321791.patch

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

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +197,5 @@
>        class: "ruleview-computedlist",
>      });
>  
> +    // Holds the viewers for the overridden shorthand properties.
> +    // will be populated in |_updateShorthandOverriden|.

s/updateShorthandOverriden/updateShortHandOverridden

@@ +198,5 @@
>      });
>  
> +    // Holds the viewers for the overridden shorthand properties.
> +    // will be populated in |_updateShorthandOverriden|.
> +    this.shorthandOverriden = createChild(this.element, "ul", {

s/this.shorthandOverriden/this.shorthandOverridden

Spelling error of overridden

@@ +199,5 @@
>  
> +    // Holds the viewers for the overridden shorthand properties.
> +    // will be populated in |_updateShorthandOverriden|.
> +    this.shorthandOverriden = createChild(this.element, "ul", {
> +      class: "ruleview-overriden-items",

s/overriden/overridden

@@ +459,5 @@
>        }
>        elToClick.click();
>      }
>  
>      // Populate the computed styles.

Update this comment to also say:
Populate the computed styles and short hand overridden styles.

@@ -500,5 @@
>     * are populated on demand, when they become visible.
>     */
>    _updateComputed: function () {
>      this.computed.innerHTML = "";
> -

We should keep this new line. You typically want to use new lines to separate out logical blocks. In this case, first block is just clearing the innerHTML and second block is styling the expanders.

@@ +572,5 @@
>      }
>    },
>  
>    /**
> +   * Update the indicator for overriden shorthand styles. The computed

s/overriden/overrridden

@@ +573,5 @@
>    },
>  
>    /**
> +   * Update the indicator for overriden shorthand styles. The computed
> +   * styles themselves are populated on demand, when they become visible.

We need to update the second sentence of this JSDoc. It looks you just copied it from _updateComputed.

@@ +575,5 @@
>    /**
> +   * Update the indicator for overriden shorthand styles. The computed
> +   * styles themselves are populated on demand, when they become visible.
> +   */
> +  _updateShorthandOverriden: function () {

s/_updateShorthandOverriden/_updateShorthandOverridden

@@ +576,5 @@
> +   * Update the indicator for overriden shorthand styles. The computed
> +   * styles themselves are populated on demand, when they become visible.
> +   */
> +  _updateShorthandOverriden: function () {
> +    this.shorthandOverriden.innerHTML = "";

s/shorthandOverriden/shorthandOverridden

Add a new line after this.

@@ +584,5 @@
> +
> +  /**
> +   * Populate the list of overriden shorthand styles.
> +   */
> +  _populateShorthandOverriden: function () {

s/_populateShorthandOverriden/_populateShorthandOverridden

@@ +593,5 @@
> +
> +    for (let computed of this.prop.computed) {
> +      // Don't bother to duplicate information already
> +      // shown in the text property.
> +

Remove the extra new line, also, comment that we don't want to show something that is not overridden.

@@ +598,5 @@
> +      if (computed.name === this.prop.name || !computed.overridden) {
> +        continue;
> +      }
> +
> +      let li = createChild(this.shorthandOverriden, "li", {

We should be able to extract out the bit of logic for creating the li element and appending all the necessary text into a helper function since populateComputed and populateShorthandOverridden basically does the same thing. 

Something like _createComputedItem(computed, className)

@@ +631,5 @@
> +      appendText(li, ";");
> +
> +      // Store the computed style element for easy access when highlighting
> +      // styles
> +      computed.element = li;

We don't want to store this, since this is actually overriding what was stored in _populateComputed
Attachment #8836756 - Flags: review?(gl)
Attached patch Bug1321791.patch (obsolete) — Splinter Review
Updated patch based on review from Gabriel in comment #16
Attachment #8836756 - Attachment is obsolete: true
Attachment #8836953 - Flags: review?(gl)
Comment on attachment 8836953 [details] [diff] [review]
Bug1321791.patch

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

Also flagging pbro for feedback on the current ui.

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +537,5 @@
>        let li = createChild(this.computed, "li", {
>          class: "ruleview-computed"
>        });
>  
> +      this._populateComputedLi(li, computed);

I think we should create the li element inside this new function and just rename the function to be 
_createComputedListItem(computed, "ruleview-computed").

Otherwise, we should rename this to be _populateComputedListItem for clarity.

@@ +573,5 @@
> +      if (computed.name === this.prop.name || !computed.overridden) {
> +        continue;
> +      }
> +
> +      let li = createChild(this.shorthandOverridden, "li", {

this._createComputedListItem(computed, "ruleview-overridden-item")

@@ +584,3 @@
>  
> +  /**
> +   * Populates the li item with the computed CSS property.

s/li/list
Attachment #8836953 - Flags: review?(gl) → feedback?(pbrosset)
Attached patch Bug1321791.patch (obsolete) — Splinter Review
Made changes suggested by Gabriel in comment #18 and added mochitests for the feature.
Attachment #8836953 - Attachment is obsolete: true
Attachment #8836953 - Flags: feedback?(pbrosset)
Attachment #8837824 - Flags: review?(gl)
Attachment #8837824 - Attachment is obsolete: true
Attachment #8837824 - Flags: review?(gl)
Comment on attachment 8837873 [details]
Bug 1321791 - [Rule View] Add indicator if parts of compound (shorthand) CSS rule are overridden;

https://reviewboard.mozilla.org/r/112868/#review114408

::: devtools/client/inspector/rules/test/browser.ini:218
(Diff revision 1)
>  [browser_rules_selector-highlighter_02.js]
>  [browser_rules_selector-highlighter_03.js]
>  [browser_rules_selector-highlighter_04.js]
>  [browser_rules_selector-highlighter_05.js]
>  [browser_rules_selector_highlight.js]
> +[browser_rules_shorthand-overridden-lists_01.js]

Since we only have 1 test that is required for this, we can drop the 01 bit.

::: devtools/client/inspector/rules/test/browser_rules_shorthand-overridden-lists_01.js:7
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Tests that the rule view shorthand overridden list work correctly,

s/work/works

::: devtools/client/inspector/rules/test/browser_rules_shorthand-overridden-lists_01.js:35
(Diff revision 1)
> +
> +function* testComputedList(inspector, view) {
> +  let rule = getRuleViewRuleEditor(view, 2).rule;
> +  let propEditor = rule.textProps[0].editor;
> +  let expander = propEditor.expander;
> +  let overriddenItems = propEditor.shorthandOverridden

Instead of querySelectorAll, I think we can simply do children/childNodes.

::: devtools/client/inspector/rules/test/browser_rules_shorthand-overridden-lists_01.js:48
(Diff revision 1)
> +  ok(!propEditor.shorthandOverridden.hasAttribute("hidden"),
> +      "The shorthandOverridden list should be open.");
> +
> +  is(overriddenItems.length, propNames.length,
> +      "There should be 2 overridden shorthand value.");
> +  propNames.forEach((propName, i) => {

We typically favor using for loops instead of forEach. So, I would change this to:
for (let i = 0; i < overriddenItems.length; i++) {}

::: devtools/client/inspector/rules/test/browser_rules_shorthand-overridden-lists_01.js:66
(Diff revision 1)
> +  expander.click();
> +  ok(!expander.hasAttribute("open"), "margin computed list is closed.");
> +  ok(!propEditor.shorthandOverridden.hasAttribute("hidden"),
> +      "The shorthandOverridden list should be open.");
> +
> +  propNames.forEach((propName, i) => {

Same as above.

::: devtools/client/inspector/rules/views/text-property-editor.js:541
(Diff revision 1)
>  
> -      let li = createChild(this.computed, "li", {
> -        class: "ruleview-computed"
> +      let className = "ruleview-computed";
> +
> +      // Store the computed style element for easy access when highlighting
> +      // styles
> +      computed.element = this._createComputedListItem(this.computed, className, computed);

See the next comment.

::: devtools/client/inspector/rules/views/text-property-editor.js:574
(Diff revision 1)
> +      if (computed.name === this.prop.name || !computed.overridden) {
> +        continue;
> +      }
> +
> +      let className = "ruleview-overridden-item";
> +      this._createComputedListItem(this.shorthandOverridden, className, computed);

Let's remove the className variable declaration, and simply just call _createComputedListItem with "ruleview-overridden-item". 

You will need to probably put some of the arguments in a new line and a single indent.

::: devtools/client/inspector/rules/views/text-property-editor.js:581
(Diff revision 1)
> +  },
> +
> +  /**
> +   * Creates and populates a list item with the computed CSS property.
> +   */
> +  _createComputedListItem: function (parentEl, className, computed) {

Let's move className to be the last argument.
Attachment #8837873 - Flags: review?(gl) → review+
Comment on attachment 8837873 [details]
Bug 1321791 - [Rule View] Add indicator if parts of compound (shorthand) CSS rule are overridden;

https://reviewboard.mozilla.org/r/112868/#review115010
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9764f81e1a
[Rule View] Add indicator if parts of compound (shorthand) CSS rule are overridden; r=gl
Flags: qe-verify+
https://hg.mozilla.org/mozilla-central/rev/dc9764f81e1a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Depends on: 1342002
Really, really nice! Thanks all <3 
Clearing my needinfo. But I opened a nit at Bug 1342002
Flags: needinfo?(nachtigall)
Build ID: 20170227030203
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on Firefox Nightly 54.0a1 on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.