904263 - Some font CSS properties can't be removed on mozilla beta 24

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Florian Quèze [:florian]]

Attachment: test-case-firefox-beta.html

Some CSS properties (font-kerning, font-synthesis, font-variant-alternates, ...) can't be removed on Mozilla Beta (24).

This is the cause of bug 904126 (Thunderbird test failure only on comm-beta since the latest uplift).

If layout.css.font-features.enabled is set to true, the test passes.

I'm attaching a reduced testcase.

I suspect this was caused by either bug 549861 or bug 886691.

Comment 1

[John Daggett (:jtd)]

The problem here is that setting the font shorthand makes the font features appear on the style struct when it's enumerated, regardless of whether the pref is enabled or not.

I'm also seeing some really odd behavior for our style OM, style["font-family"] is undefined but style.fontFamily is a null string.  Enumerating style properties via getComputedStyle, a small subset of the appropriate properties are defined on style:

http://people.mozilla.org/~jdaggett/tests/styleom-problems.html

e.g.
  style["color"] is defined
  style["line-height"] is undefined
  style["float"] is undefined (?!?)

It looks like anything with a hyphen in the prop name is undefined.  That's probably a separate bug. :P

Comment 2

[John Daggett (:jtd)]

(In reply to John Daggett (:jtd) from comment #1)

> I'm also seeing some really odd behavior for our style OM,
> style["font-family"] is undefined but style.fontFamily is a null string. 
> Enumerating style properties via getComputedStyle, a small subset of the
> appropriate properties are defined on style:

Talking with dbaron on IRC, this appears just to be JS syntactic sugar.  So not a bug per se...

Comment 3

[John Daggett (:jtd)]

Attachment: patch, add remove all property check when testing values

Test to assure all style properties can be removed via removeProperty.  I put the test into 'test_value_storage.html', but maybe some other place is more appropriate?

Note: this will fail when font features are disabled.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So this is a test that's currently failing, right?

Comment 5

[John Daggett (:jtd)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #4)
> So this is a test that's currently failing, right?

With the pref disabled it fails, yes.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 789388 [details] [diff] [review]
patch, add remove all property check when testing values

>+    // sanity check shorthands to make sure disabled props aren't exposed
>+    if (info.type == CSS_TYPE_TRUE_SHORTHAND) {

This should be a != check against CSS_TYPE_LONGHAND; we want this check for both of the other two.

>+      gDeclaration.setProperty(property, value, "");
>+      ok(test_remove_all_properties(), "unremovalable properties after setting " + property);

unremovable, not unremovalable

We should also have the same test for initial and inherit, though.  That could be done by putting roughly the same thing in test_initial_storage and test_inherit_storage (perhaps better, since I should perhaps condense the three anyway), or it could be done by putting all the checks in a separate test file.

r=dbaron with that

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Though, actually, I think it would also be more useful to have the diagnostic inside test_remove_all_properties, since then you could both test that length is 0 and test that the declaration serializes to the empty string; the latter would give a useful error message in case of failure.

Comment 8

[John Daggett (:jtd)]

Attachment: patch, don't set disabled props within font shorthand parsing

When parsing the 'font' shorthand, avoid setting disabled font feature subproperties.  Modify the CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES macro to only iterate over *enabled* subprops.

Comment 9

[John Daggett (:jtd)]

Attachment: patch v2, add remove all property check when testing prop storage

Updated tests with modifications based on review comments.

Carrying forward r=dbaron

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 790013 [details] [diff] [review]
patch, don't set disabled props within font shorthand parsing

r=dbaron

Comment 11

[John Daggett (:jtd)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f17435611c5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c978cb81f206

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f17435611c5a
https://hg.mozilla.org/mozilla-central/rev/c978cb81f206

Comment 13

[Mark Banner (:standard8)]

This fixes the problem we were having in Thunderbird (bug 904126), can this be backported through to mozilla-beta (gecko 24) please?

Comment 14

[John Daggett (:jtd)]

Comment on attachment 790013 [details] [diff] [review]
patch, don't set disabled props within font shorthand parsing

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 549861
User impact if declined: scripts may experience odd behavior
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: no

Comment 15

[John Daggett (:jtd)]

Comment on attachment 790014 [details] [diff] [review]
patch v2, add remove all property check when testing prop storage

[Approval Request Comment]
- same as above -

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/510c52b4ec97
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4851d7c6393

https://hg.mozilla.org/releases/mozilla-beta/rev/048a44870feb
https://hg.mozilla.org/releases/mozilla-beta/rev/fa0546b58c42

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 790013 [details] [diff] [review]
patch, don't set disabled props within font shorthand parsing

>+  bool featuresEnabled =
>+    mozilla::Preferences::GetBool("layout.css.font-features.enabled");

Sorry for not catching this sooner, but I realized that this is the slow way of checking the pref; calling into the preferences code is substantially slower than needed.  And it turns out we already have this cached; you can just call nsCSSProps::IsEnabled(eCSSProperty_font_variant_alternates) and comment that you're assuming that the whole chunk of properties all use the "layout.css.font-features.enabled" pref.

Based on past experience with the prefs system, I think we need to fix this.  Could you?

Comment 18

[John Daggett (:jtd)]

Attachment: patch, use cached pref for font feature property

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 791958 [details] [diff] [review]
patch, use cached pref for font feature property

Typo repeated in the comments: altnernates -> alternates.

r=dbaron; thanks for fixing.

Comment 20

[John Daggett (:jtd)]

Pushed follow-on patch to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e1ae1f29036

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e1ae1f29036

Comment 22

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0

Verified as fixed on Firefox 24 beta 4 (buildID: 20130819170952) and latest Nightly (buildID: 20130820030206).

Comment 23

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0

Verified as fixed on Firefox 25 beta 2 (buildID: 20130923194050) and latest Aurora (buildID: 20130924004001).