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

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

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

We will add the tooltip in bug 1543349

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.

@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?

(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.

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.

(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.

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

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

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

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

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

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

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!

(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!

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

Sebastian

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!

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

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.

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.

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

Will do.

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)

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.

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

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.

I moved the relnote to the 70 doc.