1295062 - TEST-UNEXPECTED-FAIL | layout/style/test/test_initial_computation.html when enabling mask longhand support

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Astley Chen (inactive)]

Encountered various test failures when enabling mask-* longhand properties.
This bug is used to track below failures.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2255d4bcde5b&selectedJob=25675029

754 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_initial_computation.html | unset should cause initial value for 'mask' - got "border-box ; none ; match-source ; border-box ; 0% ; 0% ; no-repeat ; auto auto ; add", expected "border-box ; none ; match-source ; border-box ; 50% ; 50% ; no-repeat ; auto auto ; add"
797 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_initial_computation.html | initial should cause initial value for 'mask-position' - got "0% 0%", expected "50% 50%" 
810 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_initial_computation.html | unset should cause initial value for 'mask-position-x' - got "0%", expected "50%"
813 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_initial_computation.html | initial should cause initial value for 'mask-position-y' - got "0%", expected "50%" 

1488 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_property_database.html | 'mask' listed as CSS_TYPE_LONGHAND - got 2, expected +0
4288 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_cloning.html | serialization should be nonempty for -webkit-mask: center centerp no-repeat none add - didn't expect "", but got it

Comment 1

[u459114]

Attachment: Bug 1295062 - Fix mask-position initial value in nsRuleNode::ComputeSVGResetData.

Review commit: https://reviewboard.mozilla.org/r/71520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71520/

Comment 2

[u459114]

Comment on attachment 8781017 [details]
Bug 1295062 - Fix mask-position initial value in nsRuleNode::ComputeSVGResetData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71520/diff/1-2/

Comment 3

[Astley Chen (inactive)]

Comment on attachment 8781017 [details]
Bug 1295062 - Fix mask-position initial value in nsRuleNode::ComputeSVGResetData.

https://reviewboard.mozilla.org/r/71520/#review69038

::: layout/style/nsStyleStruct.h:571
(Diff revision 2)
>      // Initialize nothing
>      Position() {}
>  
>      bool IsInitialValue(LayerType aType) const;
>  
> +    static float GetInitialValue(LayerType aType) {

I think one day we should consider to put Position as a base class and primitive for generic use and then create a subclass LayerPosition which is simply inherited from Position to define its specific behavior. It looks more sensible to me.

Comment 4

[Astley Chen (inactive)]

Comment on attachment 8781017 [details]
Bug 1295062 - Fix mask-position initial value in nsRuleNode::ComputeSVGResetData.

https://reviewboard.mozilla.org/r/71520/#review69040

LGTM.

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8781017 [details]
Bug 1295062 - Fix mask-position initial value in nsRuleNode::ComputeSVGResetData.

https://reviewboard.mozilla.org/r/71520/#review69788

I am glad the tests caught this.  But for future features that are behind a pref, we should turn the pref on in testing/profiles/prefs_general.js so that we can test the feature before we turn the pref on for Nightly users.

::: layout/style/nsRuleNode.cpp:7257
(Diff revision 2)
>                      conditions);
>  
>    // background-position-x/y: enum, length, percent (flags), inherit [list]
>    nsStyleImageLayers::Position::PositionCoord initialPositionCoord;
> -  initialPositionCoord.mPercent = 0.0f;
> +  initialPositionCoord.mPercent =
> +    nsStyleImageLayers::Position::GetInitialValue(nsStyleImageLayers::LayerType::Background);

Nit: please wrap this to fit to 80 columns (a line break after the "(").

::: layout/style/nsRuleNode.cpp:10032
(Diff revision 2)
>                      conditions);
>  
>    // mask-position-x/y: enum, length, percent (flags), inherit [list]
>    nsStyleImageLayers::Position::PositionCoord initialPositionCoord;
> -  initialPositionCoord.mPercent = 0.0f;
> +  initialPositionCoord.mPercent =
> +    nsStyleImageLayers::Position::GetInitialValue(nsStyleImageLayers::LayerType::Mask);

Nit: here too.

Comment 6

[u459114]

Comment on attachment 8781017 [details]
Bug 1295062 - Fix mask-position initial value in nsRuleNode::ComputeSVGResetData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71520/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc88aaa9ae20
Fix mask-position initial value in nsRuleNode::ComputeSVGResetData. r=astley,heycam

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cc88aaa9ae20