Closed Bug 886691 Opened 7 years ago Closed 7 years ago

@font-feature-values tests will fail when Gecko 24 merges to beta

Categories

(Core :: CSS Parsing and Computation, defect)

24 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox22 --- unaffected
firefox23 --- unaffected
firefox24 + fixed
firefox25 --- fixed

People

(Reporter: philor, Assigned: jtd)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

To the extent I'm doing it right (so far, so good), https://tbpl.mozilla.org/?tree=Try&rev=d5d7e66b1e7b is the current mozilla-aurora, 24, pushed to try as though it had been merged to beta with the version number changes and define changes and branding changes that go along with merging to beta.

According to that, if it merged today we'd have permaorange in mochitest-5 (mochitest-8 on Android) from test_bug377947.html and test_font_feature_values_parsing.html and test_specified_value_serialization.html and test_system_font_serialization.html and test_value_storage.html (beware tbpl's restriction of the popup to just the first 20, and of the log to just the 100 failures), probably from keying the feature to disable itself, but not the tests.
Looks like there's two parts to this, a few of the tests need to be sensitive to the font features pref and the font serialization code also needs to be sensitive to the pref.
This was somewhat trickier than I thought.  We have several places in CSS code where we essentially have loops of the form "for all longhands in shorthand".  There's one in Declaration::GetValue that uses the CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES macro, and there's similar logic in Declaration::ToString.  The ToString code is particularly complex and it ends up calling GetValue for the font shorthand once for each subproperty.

I think there may be other places where IsEnabled should probably be checked, within nsTransitionManager for example.
Assignee: nobody → jdaggett
Attachment #767631 - Flags: review?(dbaron)
tryserver runs

Font features disabled:
https://tbpl.mozilla.org/?tree=Try&rev=945f2c9570d0

Font features on by default:
https://tbpl.mozilla.org/?tree=Try&rev=ca378b22badb
Comment on attachment 767631 [details] [diff] [review]
patch, fix up tests and serialization code to work correctly when font features are disabled

test_font_feature_values_parsing.html:

>+  // Gecko-specific check - if pref not set, skip these tests
>+  if (window.SpecialPowers && !SpecialPowers.getBoolPref("layout.css.font-features.enabled")) {
>+    return;
>+  }

It seems odd to use "window." for one of the references to SpecialPowers
but not the other.  I'd either drop it for both, or make the first one
'"SpecialPowers" in window'.


As far as the Declaration.cpp changes:  it seems like it would be
simpler if disabled properties had the same
inherit/initial/-moz-system-font values that the enabled properties do.
If they're disabled, it seems like none of these values should do
anything (with the possible exception of -moz-system-font, which we
might need to do something to disable further).

And, furthermore, it looks to me like the code already does that.

So it's not clear to me why any tests are failing.  Could you explain?

Marking review- to prompt a response, though feel free to re-request
review with an answer if you think that's the right thing to do.
Attachment #767631 - Flags: review?(dbaron) → review-
The logic in Declaration::GetValue has two basic parts for shorthands.  It first checks whether all properties for the shorthand have been set or not.  If only a subset have been set, then empty string is returned.  However, the CSS prop table macros make no distinction between props that are enabled/disabled.  So with font features disabled, there always appear to be disabled properties in the 'font' shorthand and tests of "if all subproperties are set then a value should be returned" fail.  To avoid this, my patch skips disabled props.

The second part of the logic in GetValue assumes that all subproperties of a shorthand have been set.  It then tests to assure that subproperties that are cleared by the shorthand are set to their initial value.  Since my patch skips disabled props, it's no longer assured that all of these have been set, so they should only be checked if they are enabled.

Looking at this again, I should probably change the use of references to pointers, for example:

      const nsCSSValue &fontKerning =
        *data->ValueFor(eCSSProperty_font_kerning);

But that's somewhat cosmetic.

Let me know if you still think another approach would be better.
Flags: needinfo?(dbaron)
(In reply to John Daggett (:jtd) from comment #5)
> enabled/disabled.  So with font features disabled, there always appear to be
> disabled properties in the 'font' shorthand and tests of "if all
> subproperties are set then a value should be returned" fail.  To avoid this,
> my patch skips disabled props.

Why does this happen?  What prevents them from being set when the pref is disabled?  And can it not do that?  (The disabling works by inhibiting the name lookup in the cases when we map a string to a property id, so I don't see how it would lead to that.)
Flags: needinfo?(dbaron)
Sorry if I'm being thick, but I don't quite understand your question.

Consider this simple testcase:

  var f;

  // assume "font" property never set
  style.setProperty("font-family", "blah", "");
  f = style.getPropertyValue("font");

In current trunk code, within Declaration::GetValue the code first
iterates over all subproperties of 'font'.  The first subproperty that
is not 'font-family' will be null so the code at line 189 will bail
without setting the string:

http://mxr.mozilla.org/mozilla-central/source/layout/style/Declaration.cpp#189

    if (!val) {
      // Case (1) above: some subproperties not specified.
      return;
    }

Because of the way the property macros work the font-variant
subproperties will *always* be included in the list of properties
checked, so when font features are explicitly disabled, the case (1)
path above will *always* get hit for the 'font' shorthand, even if
*all* of the non-font feature props have in fact been set.  That's the
situation the patch is trying to work around.

Thinking about this a bit more, I think it would probably be a good
idea to try and figure out a way of having the code easily deal with
disabled props that are included in non-disabled shorthands.  The font
shorthand will always be a little extra complicated because of the
system font functionality.
Flags: needinfo?(dbaron)
Comment on attachment 767631 [details] [diff] [review]
patch, fix up tests and serialization code to work correctly when font features are disabled

sorry about my confusion; I didn't understand quite what the tests were testing (despite having written them)

So, anyway, r=dbaron... and it's also ok to change those references inside the eCSSProperty_font to pointers if you want (but change all of them if you do, and don't null-check them, since there's no need given the other tes you added)
Attachment #767631 - Flags: review- → review+
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #8)
> So, anyway, r=dbaron... and it's also ok to change those references inside
> the eCSSProperty_font to pointers if you want (but change all of them if you
> do, and don't null-check them, since there's no need given the other tes you
> added)

actually, it probably makes sense to do references -> pointers for the whole function, but probably in a separate patch stacked on top of this one
Flags: needinfo?(dbaron)
Since some subproperties may not be available when font features are disabled, use pointers instead of references for font subproperties.
Attachment #776915 - Flags: review?(dbaron)
Comment on attachment 776915 [details] [diff] [review]
patch, switch to using pointers for font subproperty values

I'd prefer to do this for the entire GetValue function; otherwise seems fine, although I didn't do the search-replace verification that I'd do on a final patch for review.
Attachment #776915 - Flags: review?(dbaron) → review-
Switches over to pointers for all values in GetValue
Attachment #776915 - Attachment is obsolete: true
Attachment #776985 - Flags: review?(dbaron)
Comment on attachment 776985 [details] [diff] [review]
patch, switch to using pointers for values in GetValue

r=dbaron... though maybe revert the eCSSProperty_marker chunk at the end?
Attachment #776985 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/1c8d605cce52
https://hg.mozilla.org/mozilla-central/rev/c2efc2cae894
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment on attachment 767631 [details] [diff] [review]
patch, fix up tests and serialization code to work correctly when font features are disabled

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 549861
User impact if declined: mochitests will fail once aurora moves to beta
Testing completed (on m-c, etc.): yes, tested with font features disabled (enabled on trunk/aurora)
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #767631 - Flags: approval-mozilla-aurora?
Comment on attachment 776985 [details] [diff] [review]
patch, switch to using pointers for values in GetValue

[Approval Request Comment]
- same as comment above -
Attachment #776985 - Flags: approval-mozilla-aurora?
Attachment #767631 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #776985 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assuming no QA needed here. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.