Display an indicator on properties with inactive CSS

RESOLVED FIXED in Firefox 68

Status

enhancement
P3
normal
RESOLVED FIXED
3 years ago
2 days ago

People

(Reporter: jdescottes, Assigned: miker, NeedInfo)

Tracking

(Blocks 3 bugs, {dev-doc-complete})

Trunk
Firefox 68
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox68 fixed)

Details

Attachments

(7 attachments, 2 obsolete attachments)

Comment hidden (obsolete)
Reporter

Updated

3 years ago
See Also: → 1302752
Inspector bug triage (filter on CLIMBING SHOES).
Severity: normal → enhancement
Priority: -- → P3
Duplicate of this bug: 1303833
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.
Summary: Display an indicator on properties when the computed value differs from the authored value → Display an indicator on properties when the computed value differs from the authored value, and possibly explain the reason
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
Keywords: dev-doc-needed
Duplicate of this bug: 1209075
Posted patch useless-props.diff (obsolete) — Splinter Review
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.
Posted patch useless-props.diff (obsolete) — Splinter Review
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.
Attachment #8876035 - Attachment is obsolete: true
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
Flags: needinfo?(pbrosset)
I would file another follow-up bug for this.
Thanks Sebastian.
Flags: needinfo?(pbrosset)

Updated

Last year
Product: Firefox → DevTools

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

Posted image inactive css mockup

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

Comment hidden (obsolete)
Depends on: 1543216

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

See Also: 1302752
Summary: Display an indicator on properties when the computed value differs from the authored value, and possibly explain the reason → Display an indicator on properties with inactive CSS
Attachment #8882980 - Attachment is obsolete: true
Duplicate of this bug: 1540755

We will add the tooltip in bug 1543349

Posted file 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.

Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Attachment #9057207 - Attachment description: Bug 1306054 - Display an indicator on properties with inactive CSS → Bug 1306054 - Display an indicator on properties with inactive CSS r?rcaliman!

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

Flags: needinfo?(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.

Flags: needinfo?(gl)

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.

Posted image work-in-progress.gif

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

Comment 29

2 months ago
Pushed by mratcliffe@mozilla.com:
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

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

Comment 31

2 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 68 → ---
Flags: needinfo?(mratcliffe)
Regressions: 1548240
Flags: needinfo?(mratcliffe)

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

Last month
Pushed by mratcliffe@mozilla.com:
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

Last month
bugherder
Status: REOPENED → RESOLVED
Closed: 2 months agoLast month
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

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

Flags: needinfo?(mratcliffe)

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!

Flags: needinfo?(mratcliffe)

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

Sebastian

Reporter

Updated

Last month
Regressions: 1552116

Comment 39

Last month

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

Regressions: 1557063
No longer regressions: 1557063

Comment 41

5 days ago

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.

Flags: needinfo?(mratcliffe)
You need to log in before you can comment on or make changes to this bug.