1643294 - font-smooth CSS property is reported as an issue even though it is not used

Status: RESOLVED FIXED

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

Description

[Patrick Brosset <:pbro>]

Attachment: Screenshot 2020-06-04 at 13.14.50.png

Steps:

• Go to https://www.mozilla.org/en-US/firefox/79.0a1/whatsnew/all/?oldversion=78.0a1
• Open the Compatibility panel and wait for it to populate.
• You should see a compat issue for font-smooth being reported on the body element.
• Click on the body element to see the applied rules in the Rules panel.

Try to find the corresponding font-smooth property in the Rules panel.
It's not actually used on this page. 2 other properties are used though:

-moz-osx-font-smoothing and -webkit-font-smoothing.

So maybe that explains it, but it's a bit weird that a warning is shown for a property that isn't part of the stylesheets on this page. It makes it hard for authors to do anything about this.

Comment 1

[Razvan Caliman [:rcaliman]]

Daisuke, it seems the Compatibility panel is using an internal alias instead of the property used.
Is there a way we can switch to the property used (also, what do we do with the two prefixed font-smoothing properties)?

Comment 2

[Daisuke Akatsuka (:daisuke)]

Hi Patrick, thank you very much for the report!
And thank you for the ni, Razvan!

For properties with aliases, the support status of each prefixed alias you set up will be combined to show which browsers do not support.
In this instance, we combine the support status of -moz-osx-font-smoothing and -webkit-font-smoothing, display that font-smooth do not cover the following browsers.
Thus, I think we’d better show all aliases in one place, not separate to each item.

On the other hand, as Patrick advised, showing the aliases that user is setting would be helpful actually.
So, we may better display 1. that the property has aliases and 2. the aliases user are setting.

I’d like to hear the opinions of Victoria and Martin as well.
What do you think??

Comment 3

[Victoria Wang [:victoria]]

Ah, this is related to the issue we were talking about in this bug :)

From looking at this example, it seems like it could be more clear to show -moz-osx-font-smoothing and -webkit-font-smoothing as separate issues, since each of them has differing compatibility and it could be useful to know that e.g. -webkit-font-smoothing is supported by several non-Firefox browsers, while the -moz one is only Firefox.

Maybe in the future we could be smarter and combine the two into one issue that shows font-smooth as a parent, but lists the prefixes used underneath.

font-smooth (prefix needed)
   -moz-osx-font-smooth (non-FF icons)
   -webkit-font-smoothing (FF icon)

   elementname

Interested in hearing what Martin thinks.

Comment 4

[Martin Balfanz [:mbalfanz]]

I think we have two issues here:

1. It's confusing to look for the property font-smooth and not find it in the CSS. I do agree with Daisuke that the grouping makes sense, also to reduce the noise. I'd like to avoid showing the vendor-prefixed versions permanently as this might add significantly more information. Maybe we have have this available in the tooltip, or as an expandable element?

2. The bigger problem for me is that the report is a false positive. The combination of the vendor prefixed properties marks all browsers as issues, while the union of -moz-osx-font-smooth and -webkit-font-smoothing is actually supported properly. Only IE and Fx Android should be reported

So, the result I imagine to be something like this:

[>] font-smooth (prefix needed) (IE icon, Fx Android icon)

      elementname

[v] font-smooth (prefix needed) (IE icon, Fx Android icon)
      -moz-osx-font-smooth (non-FF icons, FX Android icon)
      -webkit-font-smoothing (FF icon, IE, FX Android icon)

      elementname

Comment 5

[Victoria Wang [:victoria]]

Martin: your solution looks good to me!

Does your example mean we're planning to support IE? That would be awesome! I couldn't find a bug for it though - does one need to be filed?

Comment 6

[Martin Balfanz [:mbalfanz]]

Oh, I mean if we can, I think we should. Actually, I don't know why it's currently excluded. Maybe a mistake on our end. @Daisuke, was there a reason to not support IE?

Comment 7

[Daisuke Akatsuka (:daisuke)]

Hi Martin! I'm sorry for my delay.
We currently support the Edge but not IE.
I don't remember the reason why we exclude IE, but if need, as it seems that we can support soon, please let me know :)

Comment 8

[Martin Balfanz [:mbalfanz]]

Thanks Daisuke :) I really can't remember why it was excluded, and I don't see anything documented. Let's keep this in mind going forward

Comment 9

[Victoria Wang [:victoria]]

Filed a basic bug for IE :)

Comment 10

[Daisuke Akatsuka (:daisuke)]

Attachment: prototype.mp4

Hi Martin, Victoria!
I tried to make a prototype for this.
Could you check the attachment??

That there is one thing I have not done yet is to show unsupported browsers for each alias that Martin suggested. But, do you think we'd better do that, or is it enough for now?

Comment 11

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1643294: Introduce prefixNeeded flag to the compatibility issue.

Comment 12

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1643294: Update issue fields.

Depends on D83282

Comment 13

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1643294: Show prefixNeeded message.

Depends on D83283

Comment 14

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1643294: Show prefixed properties.

Depends on D83284

Comment 15

[Martin Balfanz [:mbalfanz]]

Daisuke, I think you uploaded the wrong video. Could you please double check? :)

Comment 16

[Daisuke Akatsuka (:daisuke)]

Attachment: prototype.mp4

Oh NO! Thank you for the notice :)

Comment 17

[Martin Balfanz [:mbalfanz]]

Thanks Daisuke, that looks great!

I think this is a good step forward and helps to reduce false positives. I think we can move forward like this, and maybe log the other suggestion as a separate bug/improvement.

Curious to hear what Victoria thinks.

Comment 18

[Victoria Wang [:victoria]]

This looks great to me as well! I agree it's a good solution, and the other part can be landed later.

Comment 19

[Daisuke Akatsuka (:daisuke)]

Thank you very much for your feedback, Martin and Victoria!

Comment 20

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1643294: Add an xpcshell test for prefix needed property.

Depends on D83285

Comment 21

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1643294: Add node tests for prefix needed property.

Depends on D84950

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99799c2141b6
Introduce prefixNeeded flag to the compatibility issue. r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/a61c7172d53e
Update issue fields. r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/a334d8d72e19
Show prefixNeeded message. r=rcaliman,flod,fluent-reviewers
https://hg.mozilla.org/integration/autoland/rev/b8776178d5ad
Show prefixed properties. r=rcaliman,ladybenko
https://hg.mozilla.org/integration/autoland/rev/240347a5071e
Add an xpcshell test for prefix needed property. r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/bf35895585fd
Add node tests for prefix needed property. r=rcaliman

Comment 23

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_compatibility_cssIssues.js

backout: https://hg.mozilla.org/integration/autoland/rev/087cd37b6090794d83f95266b52474e1e425ac9a

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2C18.04%2Cx64%2Copt%2Cmochitests%2Ctest-linux1804-64%2Fopt-mochitest-devtools-chrome-e10s%2Cdt3&revision=bf35895585fd777df5aea9f51595377671e7a77b&selectedTaskRun=Dytoz7sXRXK0NeKd74GhCQ.0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=311471166&repo=autoland&lineNumber=7672

[task 2020-07-30T02:52:35.359Z] 02:52:35 INFO - TEST-PASS | devtools/server/tests/browser/browser_compatibility_cssIssues.js | Expected CSS browser compat data is correct. - [] deepEqual [] -
[task 2020-07-30T02:52:35.360Z] 02:52:35 INFO - Test CSS properties linked with class "class-user-select"
[task 2020-07-30T02:52:35.360Z] 02:52:35 INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/NetUtil.jsm" line: 253}]
[task 2020-07-30T02:52:35.360Z] 02:52:35 INFO - Ensure result is correct
[task 2020-07-30T02:52:35.360Z] 02:52:35 INFO - Buffered messages finished
[task 2020-07-30T02:52:35.365Z] 02:52:35 INFO - TEST-UNEXPECTED-FAIL | devtools/server/tests/browser/browser_compatibility_cssIssues.js | Expected CSS browser compat data is correct. - [{"url":"https://developer.mozilla.org/docs/Web/CSS/user-select","deprecated":false,"experimental":false,"unsupportedBrowsers":[{"id":"chrome","version":"81"},{"id":"chrome_android","version":"81"},{"id":"safari","version":"13"},{"id":"safari_ios","version":"13.4"},{"id":"edge","version":"81"}],"property":"user-select","aliases":["-moz-user-select"],"prefixNeeded":true,"type":"CSS_PROPERTY_ALIASES"}] deepEqual [{"type":"CSS_PROPERTY_ALIASES","property":"user-select","aliases":["-moz-user-select"],"url":"https://developer.mozilla.org/docs/Web/CSS/user-select","deprecated":false,"experimental":false,"unsupportedBrowsers":[{"id":"chrome","version":"81"},{"id":"chrome_android","version":"81"},{"id":"safari","version":"13"},{"id":"safari_ios","version":"13.4"},{"id":"edge","version":"81"}]}] - JS frame :: chrome://mochitests/content/browser/devtools/server/tests/browser/browser_compatibility_cssIssues.js :: testNodeCssIssues :: line 100
[task 2020-07-30T02:52:35.365Z] 02:52:35 INFO - Stack trace:
[task 2020-07-30T02:52:35.366Z] 02:52:35 INFO - chrome://mochitests/content/browser/devtools/server/tests/browser/browser_compatibility_cssIssues.js:testNodeCssIssues:100
[task 2020-07-30T02:52:35.366Z] 02:52:35 INFO - chrome://mochitests/content/browser/devtools/server/tests/browser/browser_compatibility_cssIssues.js:null:115
[task 2020-07-30T02:52:35.367Z] 02:52:35 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1064
[task 2020-07-30T02:52:35.367Z] 02:52:35 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1104
[task 2020-07-30T02:52:35.367Z] 02:52:35 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:927
[task 2020-07-30T02:52:35.368Z] 02:52:35 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:1032
[task 2020-07-30T02:52:35.368Z] 02:52:35 INFO - Test CSS properties linked with multiple classes and id
[task 2020-07-30T02:52:35.369Z] 02:52:35 INFO - Ensure result is correct
[task 2020-07-30T02:52:35.369Z] 02:52:35 INFO - Not taking screenshot here: see the one that was previously logged

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f2f70dba7da
Introduce prefixNeeded flag to the compatibility issue. r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/20275289fbaf
Update issue fields. r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/2c631f8aa7e0
Show prefixNeeded message. r=rcaliman,flod,fluent-reviewers
https://hg.mozilla.org/integration/autoland/rev/64e938951928
Show prefixed properties. r=rcaliman,ladybenko
https://hg.mozilla.org/integration/autoland/rev/8343a0f124e7
Add an xpcshell test for prefix needed property. r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/3e9a6cf5920f
Add node tests for prefix needed property. r=rcaliman

Comment 25

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/0f2f70dba7da
https://hg.mozilla.org/mozilla-central/rev/20275289fbaf
https://hg.mozilla.org/mozilla-central/rev/2c631f8aa7e0
https://hg.mozilla.org/mozilla-central/rev/64e938951928
https://hg.mozilla.org/mozilla-central/rev/8343a0f124e7
https://hg.mozilla.org/mozilla-central/rev/3e9a6cf5920f