Closed Bug 1301638 Opened 8 years ago Closed 8 years ago

Using runtime pref to enable/ disable CSS mask properties

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: bmo, Assigned: bmo, Mentored)

References

Details

(Keywords: dev-doc-complete, Whiteboard: tlt-wip)

Attachments

(3 files)

Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.
58 bytes, text/x-review-board-request
xidorn
: review+
dbaron
: review-
Details
Bug 1301638 : Part 2 - Control CSS masking reftest by preference.
58 bytes, text/x-review-board-request
Details
Bug 1301638 : Part 3 - Add invalid values for mask longhand.
58 bytes, text/x-review-board-request
xidorn
: review+
Details 
This bug is cloned from bug 1243734.

Instead of using #ifdef compile flag for static control, we'd like to come up with a runtime mechanism to runtime switch on/off this feature.

With which we are able to ensure all codes are built and run in current firefox versions and tests.
Assignee: nobody → aschen
Whiteboard: tlt-wip
Status: NEW → ASSIGNED
Blocks: mask-ship
This may be rather hard, and I'm not sure it's actually needed to ship the new mask properties.
(In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from comment #1)
> This may be rather hard, and I'm not sure it's actually needed to ship the
> new mask properties.

It doesn't seem to me this is that hard. Can we simply remove all the compile time conditions, add a runtime pref, and then add a special parsing function for mask, which sets mask-image when the runtime pref is disabled?

It seems to me one challenge would be the webkit aliases, which are currently controlled by the pref for webkit prefix, and we cannot have one property be controlled by two prefs. Probably we can just make them controlled by the new mask runtime pref.

I remembered that you (or roc, or you two together) taught me that we in general want a runtime pref to control new features, so that we can switch it off in case we find any serious issue near or even after release, and the extended mask property looks like something risky to release without a runtime pref in that sense, so I think it is needed.
Blocks: 1304684
Depends on: 1243734
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

https://reviewboard.mozilla.org/r/86836/#review85840

::: modules/libpref/init/all.js:2466
(Diff revision 1)
>  // Is support for CSS initial-letter property enabled?
>  pref("layout.css.initial-letter.enabled", false);
>  
> +// Is support for CSS positioned mask enabled?
> +// Enabled by default for nightly and aurora channels
> +#ifdef RELEASE_BUILD

After Bug 1304829, you might want to use RELEASE_OR_BETA instead of RELEASE_BUILD here.
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

https://reviewboard.mozilla.org/r/86836/#review85840

> After Bug 1304829, you might want to use RELEASE_OR_BETA instead of RELEASE_BUILD here.

Thanks for pointing it out. Fixed.
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

Xidorn, part 1 patch was originally created by CJ on bug 1243734. We are thinking of a better solution to support either mask longhand and mask shorthand by using preference control. Your feedback would be helpful to moving forward.
Thanks.
Attachment #8802442 - Flags: feedback?(xidorn+moz)
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

Looks fine to me.
Attachment #8802442 - Flags: feedback?(xidorn+moz) → feedback+
Attachment #8802442 - Flags: review?(dbaron)
Attachment #8802443 - Flags: review?(dbaron)
Attachment #8802442 - Flags: review?(dbaron) → review?(xidorn+moz)
Attachment #8802443 - Flags: review?(dbaron) → review?(xidorn+moz)
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

https://reviewboard.mozilla.org/r/86836/#review89508

Please address the following issues.

In addition to those issues, please add some invalid values (which is valid when it is used as shorthand) for longhand mask to property_database to ensure our test covers that. This could be in a separate patch. Also when you do try push, you may want to do another push with mark-as-shorthand disabled by default to ensure the behavior is correct in that case.

::: layout/style/Declaration.cpp:763
(Diff revision 3)
>        GetImageLayerValue(data, aValue, aSerialization,
>                           nsStyleImageLayers::kMaskLayerTable);

You probably want a separate branch for longhand mask here. It seems to me the code of GetImageLayerValue would return nothing when mask-as-shorthand is not enabled.

::: layout/style/nsCSSDataBlock.cpp:707
(Diff revision 3)
>    CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aPropID, aEnabledState) {
>      changed |= DoTransferFromBlock(aFromBlock, *p,
>                                     aIsImportant, aOverrideImportant,
>                                     aMustCallValueAppended, aDeclaration,
>                                     aSheetDocument);
> +  } else if (*p == eCSSProperty_mask_image &&

This is too hacky. Please add a separate if-block to check when aPropID is mask and mask-as-shorthand is not enabled, probably before the general handling of shorthands.

::: layout/style/nsCSSParser.cpp:11819
(Diff revision 3)
> +    } else {
> +      return ParseMaskAsLonghand();
> +    }

We do not use "else" after a "return". Just remove the else block and put return there directly.

::: layout/style/nsCSSParser.cpp:12114
(Diff revision 3)
> +             (mToken.mType == eCSSToken_Function &&
> +             IsFunctionTokenValidForImageLayerImage(mToken))) {

It seems the longhand mask only supports none and url(). We probably shouldn't add function support to it.

::: layout/style/nsCSSParser.cpp:12116
(Diff revision 3)
> +    if (ParseSingleValueProperty(imageList->mValue, eCSSProperty_mask_image) !=
> +          CSSParseResult::Ok) {
> +      return false;
> +    }

This section seems to be duplicate the branch above. How about merging the two checks, then just early return when the token is neither none nor a url, and after that, call ParseSingleValueProperty.

::: layout/style/nsCSSParser.cpp:17387
(Diff revision 3)
> +  if (!nsCSSProps::IsEnabled(eCSSProperty_mask_image)) {
> +    AppendValue(eCSSProperty_mask_image, value);
> +  }

Please add a comment to explain why this is necessary. The block itself is confusing when there is no context.
Attachment #8802442 - Flags: review?(xidorn+moz)
Comment on attachment 8802443 [details]
Bug 1301638 : Part 2 - Control CSS masking reftest by preference.

https://reviewboard.mozilla.org/r/86838/#review89522

Not sure whether it is useful enough to have this kind of settings. Maybe we can just rely on the style system tests.

Alternatively, it may be easier to add a new constant to the condition sandbox in reftest.jsm (probably called markIsShorthand), then use fails-if(!markIsShorthand) for those tests.

I'd like to hear from dbaron what does he think about this patch.
Attachment #8802443 - Flags: review?(xidorn+moz)
dbaron, ni? you for comment 14.
Flags: needinfo?(dbaron)
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

https://reviewboard.mozilla.org/r/86836/#review89508

> You probably want a separate branch for longhand mask here. It seems to me the code of GetImageLayerValue would return nothing when mask-as-shorthand is not enabled.

In nsCSSParser::ParseMaskAsLonghand(), mask longhand would be parsed as a single value mask-image. With which in Declaration::GetValue() we will be able to correctly get mask longhand value as is a shorthand mask prop with single mask-image value.

> This is too hacky. Please add a separate if-block to check when aPropID is mask and mask-as-shorthand is not enabled, probably before the general handling of shorthands.

Will update in following patch.

> We do not use "else" after a "return". Just remove the else block and put return there directly.

Thanks for pointing it out. Will fix in following patch.

> It seems the longhand mask only supports none and url(). We probably shouldn't add function support to it.

Yes, will remove it and update in following patch.

> This section seems to be duplicate the branch above. How about merging the two checks, then just early return when the token is neither none nor a url, and after that, call ParseSingleValueProperty.

Will update in following patch.

> Please add a comment to explain why this is necessary. The block itself is confusing when there is no context.

Thanks. Will do.
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

https://reviewboard.mozilla.org/r/86836/#review89508

> In nsCSSParser::ParseMaskAsLonghand(), mask longhand would be parsed as a single value mask-image. With which in Declaration::GetValue() we will be able to correctly get mask longhand value as is a shorthand mask prop with single mask-image value.

Ok, it seems I was wrong, so the first image would be serialized and then we will break from the loop without clearing the result text.
Attachment #8807445 - Flags: review?(xidorn+moz)
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

https://reviewboard.mozilla.org/r/86836/#review90648

r=me with the comments below addressed.

::: layout/style/nsCSSDataBlock.cpp:702
(Diff revision 4)
> +  if (!nsCSSProps::IsEnabled(eCSSProperty_mask_image) &&
> +      aPropID == eCSSProperty_mask) {
> +    changed |= DoTransferFromBlock(aFromBlock, eCSSProperty_mask_image,
> +                                   aIsImportant, aOverrideImportant,
> +                                   aMustCallValueAppended, aDeclaration,
> +                                   aSheetDocument);

Suggestion: you can probably do this before the nsCSSProps::IsShorthand, and just change eCSSProperty_mask to eCSSProperty_mask_image when shorthand mask is not enabled, so you can reuse majority of the code.

::: layout/style/nsCSSParser.cpp:12104
(Diff revision 4)
> +  nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  if (keyword != eCSSKeyword_none && mToken.mType != eCSSToken_URL) {

You should check whether the type is eCSSToken_Ident before querying the keyword, otherwise something like "none(", "#none", etc may pass this test, although they would eventually be blocked by ParseSingleValueProperty.

::: layout/style/nsCSSParser.cpp:17382
(Diff revision 4)
>    }
> +
> +  // When pref layout.css.positioned-mask.enabled is false, mask-image property
> +  // is used to keep the value of mask longhand property for backward
> +  // compatibility. In such case, mask-image is in disabled state so that we
> +  // need to take care of it particularly. 

Please removing the tailing whitespace.
Attachment #8802442 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8802443 [details]
Bug 1301638 : Part 2 - Control CSS masking reftest by preference.

https://reviewboard.mozilla.org/r/86838/#review90660

There is nothing changed to this commit. I'm still not convinced that it is a good idea to do so. They should be using fails-if, or we should just test the enabled case, and relying on style system tests to catch issues.

Also if you really think this would be useful, you should at least also duplicate the non-failing tests instead of moving them to the top of the list. (But I'm not a fan of duplicating things.)
Attachment #8802443 - Flags: review?(xidorn+moz)
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

https://reviewboard.mozilla.org/r/86836/#review90666

::: modules/libpref/init/all.js:2465
(Diff revision 4)
>  pref("layout.css.initial-letter.enabled", false);
>  
> +// Is support for CSS positioned mask enabled?
> +// Enabled by default for nightly and aurora channels
> +#ifdef RELEASE_OR_BETA
> +pref("layout.css.positioned-mask.enabled", false);

Why is it called positioned-mask? Should we just call it mask-shorthand.enabled?
Comment on attachment 8807445 [details]
Bug 1301638 : Part 3 - Add invalid values for mask longhand.

https://reviewboard.mozilla.org/r/90578/#review90668
Attachment #8807445 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

https://reviewboard.mozilla.org/r/86836/#review91162

::: layout/style/nsCSSParser.cpp:12094
(Diff revision 4)
> +CSSParserImpl::ParseMaskAsLonghand()
> +{
> +  nsCSSValue image;
> +
> +  // Check first for inherit/initial/unset.
> +  if (ParseSingleTokenVariant(image, VARIANT_INHERIT, nullptr)) {
> +    AppendValue(eCSSProperty_mask_image, image);
> +    return true;
> +  }

I think the approach you're taking here is not actually going to work -- i.e., you'll have a substantial number of test failures with the preference off.

In particular, the shorthand serialization code in Declaration::GetPropertyValueInternal is going to count the number of subproperties of a shorthand that are set to initial or inherit in order to decide whether it can serialize an initial or inherit value for 'mask'.  So this will break the ability to reserialize 'mask' values.

I think you need to run the style system mochitests both with the preference on and with the preference off, and make sure they pass both ways.  Otherwise there's not much point having a preference, if both values of the preference don't actually work correctly.

It might be somewhat straightforward to fix this, although it might not be.  If it's not, it's not clear to me why we need to convert this to a preference before shipping it.  I think it's OK to use compile flags when we need to.

::: layout/style/nsCSSParser.cpp:12104
(Diff revision 4)
> +  nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  if (keyword != eCSSKeyword_none && mToken.mType != eCSSToken_URL) {
> +    return false;
> +  }
> +
> +  nsCSSValueList* imageList = image.SetListValue();
> +  if (ParseSingleValueProperty(imageList->mValue, eCSSProperty_mask_image) !=
> +        CSSParseResult::Ok) {
> +    NS_NOTREACHED("should be able to parse");
> +    return false;
> +  }

So there are a number of ways this is different from what the old code did, for example, by accepting multiple items.

It seems better to just use ParseVariant(VARIANT_HUO, etc.) and then manually append a mask-image value.
Attachment #8802442 - Flags: review-
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> https://reviewboard.mozilla.org/r/86838/#review89522
> 
> Not sure whether it is useful enough to have this kind of settings. Maybe we
> can just rely on the style system tests.
> 
> Alternatively, it may be easier to add a new constant to the condition
> sandbox in reftest.jsm (probably called markIsShorthand), then use
> fails-if(!markIsShorthand) for those tests.
> 
> I'd like to hear from dbaron what does he think about this patch.

I don't think it's necessary to run the reftests for new masking features with the preference off.  We should just run them with the pref on.  (But if some of the tests cover existing masking features, it probably is worth moving them up to before the default-preferences line, if they're actually tests for the new features of the mask shorthand rather than just tests that happen to pass with the feature off.)
Flags: needinfo?(dbaron)
Comment on attachment 8802442 [details]
Bug 1301638 : Part 1 - Add pref control for CSS positioned mask props.

https://reviewboard.mozilla.org/r/86836/#review91162

> I think the approach you're taking here is not actually going to work -- i.e., you'll have a substantial number of test failures with the preference off.
> 
> In particular, the shorthand serialization code in Declaration::GetPropertyValueInternal is going to count the number of subproperties of a shorthand that are set to initial or inherit in order to decide whether it can serialize an initial or inherit value for 'mask'.  So this will break the ability to reserialize 'mask' values.
> 
> I think you need to run the style system mochitests both with the preference on and with the preference off, and make sure they pass both ways.  Otherwise there's not much point having a preference, if both values of the preference don't actually work correctly.
> 
> It might be somewhat straightforward to fix this, although it might not be.  If it's not, it's not clear to me why we need to convert this to a preference before shipping it.  I think it's OK to use compile flags when we need to.

I guess this can be fixed via adding a separate branch in Declaration::GetPropertyValueInternal to handle mask-as-longhand separately.
Seeing a bunch of test failures on pref-off TRY testing[1].
Taking mask property as longhand and shorthand is theoretically working but probably needs more care on relevant codes especially for IsShorthand condition test which we need to change eCSSProperty_mask to eCSSProperty_mask_image. I'm also concerning for those cases we don't have enough test coverage and it will just happen on beta/release.
Per discussion with CJ, mask shorthand follow-up bugs are almost cleaned up and target to SHIP this feature on Firefox 53. Therefore, I'll leave this bug WON'T-FIX unless we found something we have to.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e83c41d9a826714b83ef41b3278765f8a9b41de4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: