1306054 - Display an indicator on properties with inactive CSS

Status: VERIFIED FIXED

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

Description

[Julian Descottes [:jdescottes]]

The rule-view enhancement proposed here is to add an indicator next to the authored value of a property, if the computed value is different from the authored value.

Moving the mouse over the indicator should display the computed value in a tooltip.

This should only apply to values that are not overridden.
Trivial differences such as "0" and "0px" should be ignored.

Why do this? There can be a big difference between the authored value of a property and its computed value. Some basic examples:
- values such as initial, inherit, auto ... 
- variables
- more complex scenarios (mixing height: 200px and max-height: 10px for instance)

Comment 1

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 3

[Patrick Brosset <:pbro>]

As discussed in bug 1303833, we could go further than showing the computed-value in the tooltip. We can, in some cases, show a warning that explains users why a property isn't applied the way they thought it would.

A common example is trying to set a width or height on an inline element.

I have started to gather a list of such cases here: https://github.com/captainbrosset/useless-css-properties/blob/master/rules.md

If you have a property name, an element, and its computed styles, you could pretty easily cycle through such a list of "rules" and use that to display warnings to users about why that given property may not apply to that current element.

The proposal is therefore the following:
- we would maintain a list of rules in DevTools,
- when getting applied rules from the PageStyleActor (in the rule-view), using the getApplied method, we'd go through this list of rules to see if any match,
- if there are matches, these would be sent back to the client, just like invalid properties are,
- on the client, an icon would be displayed and hovering over it would provide the reason for the property to not apply
- if no rule match but we still found that the computed value was different from the authored value, then a warning would be sent too, with some sort of generic message.

Comment 4

[Sebastian Zartner [:sebo]]

This sounds like another good candidate for linking to MDN for further explanation like it's done for JS errors in the console panel.

Sebastian

Comment 6

[Patrick Brosset <:pbro>]

Attachment: useless-props.diff

This does not yet work, but it's a start. At least the list of rules is encoded in there.
I have not yet found the right way to return the list of errors to the client when a rule is returned.

Just attaching this here since I don't want this to get lost, and I don't have time to work on this right now.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: useless-props.diff

I had a few hours last week so I hacked a bit more on this.
It now mostly works in the general case.

There's one major piece missing: when a rule is modified, then other rules that contain useless properties are not refreshed. They should.
Take this example:

rule1 {
  width: 50px;
  display: block;
}

rule2 {
  display: inline !important;
}

When the rule-view is loaded the first time, it will show `width: 50px` as being useless because the computed style of the current element has an inline display value.
Now, if you click on the checkbox next to `display: inline !important` property to disable it, the `width: 50px` property will still appear as useless even though it is not anymore.

That's because when a property is changed, we only update its parent rule, not other rules.

Comment 8

[Sebastian Zartner [:sebo]]

At the Hawaii All Hands we discussed the integration of a link to MDN[1] providing more detailed explanations.
Should the link to MDN already be part of this bug or should I create a new one depending on it?

Sebastian

[1] https://docs.google.com/document/d/1TteutcGV5WGnDeCliJAunRWNVG7t1yAF-HIuuzYpqyw/edit

Comment 9

[Patrick Brosset <:pbro>]

I would file another follow-up bug for this.
Thanks Sebastian.

Comment 10

[Victoria Wang [:victoria]]

Wanted to mention this related project: Ply detects whether a style will have visual impact on the page by comparing computed visual differences. https://github.com/sarahlim/ply#novel-features

Comment 11

[Victoria Wang [:victoria]]

Attachment: inactive css mockup

This shows an example of how we could portray overridden styles vs. inactive/ineffective styles.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: tooltip-mockup.png

Attaching a second mockup from Victoria for more context on this bug.

Comment 13

[Victoria Wang [:victoria]]

Any updates to this design will be uploaded to this Invision link: https://mozilla.invisionapp.com/share/UQRBDJ8WCTH#/screens

Comment 15

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

We will add the tooltip in bug 1543349

Comment 16

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

Attachment: test.html

When a property changes we only reprocess that rule so we need to mark any inactive CSS rules for all properties each time a property changes.

Here is a simple test case:

1. Open this test case.
2. Disable display: flex.

align-content: space-between should display an inactive CSS icon but it doesn't because we fail to update rule1 when disabling the display: flex property in rule2.

Comment 17

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

Attachment: Bug 1306054 - Display an indicator on properties with inactive CSS r?rcaliman!

Comment 18

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

@gl This works really well but because we are rebuilding properties on each change it breaks most of our rule view tests.

We have to rebuild all properties rather than just the properties in the current rule because properties in one rule can affect whether a property in another rule is valid.

Is there an alternate way to accomplish the same thing?

Comment 19

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

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #18)

@gl This works really well but because we are rebuilding properties on each change it breaks most of our rule view tests.

We have to rebuild all properties rather than just the properties in the current rule because properties in one rule can affect whether a property in another rule is valid.

Is there an alternate way to accomplish the same thing?

This sounds very similar to markOverriddenAll which has to be called after every time we toggle on/off a declaration in a rule. If the concept is similar, I imagine there is no way we can get around it, however, you might want to explore that bit of code to see how it is able to accomplish it without breaking our tests.

Comment 20

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2936eda3db10c4ce9cc772f21316f0a4210d4dae

Comment 21

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

Although I have most tests working I am not happy with the fix. I a nutshell this has exposed a race condition inside assertShowPreviewTooltip() and it should be fixed inside that method where possible.

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61915c5bdce394023153b96e9a3eafdbad58eb4b

Comment 23

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

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #21)

Although I have most tests working I am not happy with the fix. I a nutshell this has exposed a race condition inside assertShowPreviewTooltip() and it should be fixed inside that method where possible.

This is now fixed.

Comment 24

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=016f8dc8e05dbaa89bc5a79b822ce23e786d3fc1

Comment 25

[Patrick Brosset <:pbro>]

Attachment: work-in-progress.gif

Attaching this here, just so people can see how this feature is shaping up.

Comment 26

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

Attachment: Bug 1306054 - [inactive CSS] Fix current tests and add new test r?rcaliman!

Comment 27

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5606f0c3c18f4a3a75041c8647d7be9b56b49e35

Comment 28

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bad22682ebfa917a91e9b95dab5345093f68d79

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4369c5635972
Display an indicator on properties with inactive CSS r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/88fb590040c4
[inactive CSS] Fix current tests and add new test r=rcaliman

Comment 30

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4369c5635972
https://hg.mozilla.org/mozilla-central/rev/88fb590040c4

Comment 31

[Razvan Maries]

Backed out for l10n issues.
Backout: https://hg.mozilla.org/integration/autoland/rev/e34d54ab62b4e52aebcbed7456d67d0c5d9f0f9c

Comment 32

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

Attachment: Bug 1306054 - Use custom tooltip for inactive properties r?pbro!,jdescottes!

Changes

Probably the most important change apart from the tooltips is that we now only support one property at a time. This allows us to short circuit at the first invalid property and improve performance. This was previously agreed with Razvan but there were some relics left in the code.

toolbox.xul

• Added tooltips.ftl

devtools/client/inspector/markup/test/helper_events_test_runner.js:

• Had to change to synthesizeMouseAtCenter because CSS changes caused the original to fail.

devtools/client/inspector/rules/rules.js:

• Added VIEW_NODE_INACTIVE_CSS to node types and sorted alphabetically.
• Added new nodeInfo data for Inactive CSS icons.

devtools/client/inspector/rules/test/browser_rules_inactive_css_flexbox.js &
devtools/client/inspector/rules/test/browser_rules_inactive_css_grid.js:

• removed some listeners that are no longer needed

devtools/client/inspector/rules/test/head.js:

• Refactored getPropertiesForRuleIndex() in order to pass along information needed for testing our Fluent strings.
• Refactored checkDeclarationIsInactive() to check tooltip contnts using a new method.
• Added checkInteractiveTooltip() for checking the tooltip contents themselves.
• Simple changes to runInactiveCSSTests().

devtools/client/inspector/rules/views/text-property-editor.js:

• We no longer create the tooltip by adding the title attribute.

devtools/client/inspector/shared/node-types.js:

• Changed the enum to use strings to simplify debugging.
• Added VIEW_NODE_INACTIVE_CSS.
• Sorted alphabetically.

devtools/client/inspector/shared/tooltips-overlay.js:

• Introduced a new tooltip type called interactiveTooltip.

devtools/client/locales/en-US/inspector.properties:

• Removed strings.

devtools/client/locales/en-US/tooltips.ftl:

• Added structured versions of the properties from inspector.properties.

devtools/client/shared/widgets/tooltip/HTMLTooltip.js:

• Made the tooltips obey the "prevent popup autohide" option in the browser debugger.

devtools/client/shared/widgets/tooltip/InactiveCSSTooltipHelper.js:

• Main file for handling InactiveCSS Tooltips.

devtools/client/themes/tooltips.css:

• Made arrow tooltips follow the Proton theme.

devtools/server/actors/utils/inactive-property-helper.js:

• General changes to support Fluent.
• Bail on first inactive property found.

Latest Try (expecting green)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de28939206d444dc4b534a3e5cc7a84b8797bec3

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ecf6843307fa
Display an indicator on properties with inactive CSS r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/85d5010b19ab
[inactive CSS] Fix current tests and add new test r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/362df4629f8f
Use custom tooltip for inactive properties r=jdescottes,flod,rcaliman

Comment 34

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/ecf6843307fa
https://hg.mozilla.org/mozilla-central/rev/85d5010b19ab
https://hg.mozilla.org/mozilla-central/rev/362df4629f8f

Comment 35

[Sebastian Zartner [:sebo]]

Works really great! I'm sure that'll help many people to understand the background behind inactive properties. Thanks for implementing this, Mike!

Checking the patch's source, I saw this currently only covers Flexbox and Grid related issues. In comment 3 Patrick linked to a list with many more rules. And I'm sure there are still a lot more of these. Should I file a follow-up bug for that list?

Sebastian

Comment 36

[Patrick Brosset <:pbro>]

Thanks Sebastian. Yes you are correct, we plan on adding many more rules to the engine to cover many cases where CSS doesn't work like people expect it to.
We wanted to focus on a very small subset first to test the engine first.
Filing a bug sounds good, thanks!

Comment 37

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

(In reply to Sebastian Zartner [:sebo] from comment #35)

Works really great! I'm sure that'll help many people to understand the background behind inactive properties. Thanks for implementing this, Mike!

Checking the patch's source, I saw this currently only covers Flexbox and Grid related issues. In comment 3 Patrick linked to a list with many more rules. And I'm sure there are still a lot more of these. Should I file a follow-up bug for that list?

Sebastian

Sorry, I just noticed your comment.

We have a very specific plan on which properties will be added and in which order so it is better if I log the bugs.

I have a particularly difficult-to-please-all-the-people feature where I need to address some review comments first and then I will log a chain of bugs.

If you want to watch the meta bug it is bug 1540753.

I completely agree though, this is one of those really obvious features that should have always been there!

Comment 38

[Sebastian Zartner [:sebo]]

Perfect, Mike, I'll let you file the bugs then and follow the meta-bug. Thank you both for the feedback!

Sebastian

Comment 39

[florianuhlig]

Very nice feature.
Is there a way to get an overview / list of the analyzed CSS or even a NPM package that does a similar job?
Would like to use this feature to analyze entire stylesheets.

Thanks in advance!

Comment 40

[Patrick Brosset <:pbro>]

For now the list of warnings is very small, and we'll keep adding more to it soon.
However this isn't really intended for the use case you have in mind I think. This is a debugging and learning tool which should help people understand when things don't really work the way they thought it would. The workflow is such that you select one element in devtools and then get warnings about some properties that might not do what you thought they did (namely, that they don't apply).

It seems like what you're wanting to do here is more in line with auditing. Perhaps do a static analysis of an entire stylesheet and detecting when things are useless and could be removed.
The tool we built doesn't do that. Certain CSS properties that are marked as inactive now, may very well be useful under other circumstances (when the browser window is resized for example, or after the page was modified by javascript, etc.)

In any case, the list of warnings comes from this file: https://searchfox.org/mozilla-central/source/devtools/server/actors/utils/inactive-property-helper.js

Comment 41

[Irene Smith]

Added the following text to the section of the Inspector documentatation Examine CSS Rules as follows:

A warning icon appears next to unsupported CSS properties or rules that have invalid values. This can help you understand why certain styles are not being applied. In the following example, a spelling error, "background-colour" instead of "background-color" has made the rule invalid.

I also added an image to go with the text.

I also added a comment to the release notes.

Comment 42

[Cristian Fogel [:cfogel]]

Marking as verified with a few notes:

• on 68 builds they're still behind the preff as per bug 1547224;
• per latest nightly builds the styling is updated.

Comment 43

[Irene Smith]

Can you send me a "needinfo" when the UI changes get into the Developer/Beta version so I can make new screenshots?

Comment 44

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

Will do.

Comment 45

[Chris Mills [:cmills]]

I was thinking of adding this to the Firefox 69 release notes.

I've looked over your documentation, and I'm not sure it is correct in terms of what this bug is actually doing. You added the stuff at the bottom fo this section, right:

https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Examine_CSS_rules

?

Whereas the stuff this bug is talking about is more like what you can see here: https://bug1306054.bmoattachments.org/attachment.cgi?id=9056862

I am not sure. In fact I am doubly confused as I can't seem to make this appear using the current Firefox 70.

Also, you added a note to the Firefox 68 release notes about this, when it was still behind a pref at that point. So it should be moved (to 69 or 70, whenever it was enabled, I'm not sure)

Comment 46

[Chris Mills [:cmills]]

I am not sure. In fact I am doubly confused as I can't seem to make this appear using the current Firefox 70.

Sorry for the confusion. I managed to get this to work now. The attached screenshot (inactive-css-indicator.png) shows what you are looking for.

Comment 47

[Chris Mills [:cmills]]

Attachment: inactive-css-indicator.png

Comment 48

[Patrick Brosset <:pbro>]

For info, this will be riding the trains with Firefox 70, not 69. It is still behind a pref in 69.

Comment 49

[Chris Mills [:cmills]]

For info, this will be riding the trains with Firefox 70, not 69. It is still behind a pref in 69.

Ah, good to know — thanks Patrick! So I won't be adding this to the 69 release post, and Irene needs to move the release note to the Fx 70 notes, plus the changes I outlined above.

Comment 50

[Panos Astithas (he/him) [:past] (please ni?)]

I moved the relnote to the 70 doc.