1258286 - initial value for mask-origin property should be border-box, not padding-box

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Astley Chen (inactive)]

Attachment: mask-origin-1-ref.html

initial value of mask-origin is border-box. From the attached test cases, we observed that the default value of current impl is padding-box instead of border-box.

property spec- https://drafts.fxtf.org/css-masking-1/#the-mask-origin

Comment 1

[Astley Chen (inactive)]

Attachment: reference html

Comment 2

[Astley Chen (inactive)]

Comment on attachment 8732785 [details]
reference html

>http://astleychen.github.io/css-masking/bug1258286/mask-origin-1-ref.html

Comment 3

[Astley Chen (inactive)]

Reference html: http://astleychen.github.io/css-masking/bug1258286/mask-origin-1-ref.html
Test html: http://astleychen.github.io/css-masking/bug1258286/mask-origin-1.html

Comment 4

[Astley Chen (inactive)]

cjku, can you suggest where to start to solve this issue ? I'd like to work on this issue directly. Thanks.

Comment 5

[u459114]

I guess you may try to modify the default value here
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2529
Change mOrigin(NS_STYLE_IMAGELAYER_ORIGIN_PADDING) to mOrigin(NS_STYLE_IMAGELAYER_ORIGIN_BORDER), and see the result.

If you get correct rendering result after that... you know what to do.

Comment 6

[Cameron McCormack (:heycam)]

Also here: https://dxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#9909

Comment 7

[David Baron :dbaron:]

Will this change bring us closer or further from matching other implementations?

Comment 8

[Astley Chen (inactive)]

(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #7)
> Will this change bring us closer or further from matching other
> implementations?

Yes. AFAIK, Webkit/Blink/Opera behave correctly as spec said. We need to fix this bug before ship.

Comment 9

[Astley Chen (inactive)]

Attachment: Part 1 - add layer types to nsStyleImageLayers and layer initialization

Comment 10

[Astley Chen (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=880c8ef2faff

Comment 11

[Astley Chen (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dc28ebb7eae

Comment 12

[Astley Chen (inactive)]

Attachment: Part 1 - add layer types to nsStyleImageLayers and layer initialization

* * *
try: -b do -p all -u all -t none

Comment 13

[Astley Chen (inactive)]

Attachment: Part 2 - update w3c css tests

Comment 14

[u459114]

Comment on attachment 8756840 [details] [diff] [review]
Part 1 - add layer types to nsStyleImageLayers and layer initialization

Review of attachment 8756840 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsComputedDOMStyle.cpp
@@ +6108,3 @@
>        !firstLayer.mSize.IsInitialValue() ||
>        !(firstLayer.mImage.GetType() == eStyleImageType_Null ||
> +        firstLayer.mImage.GetType() == eStyleImageType_Image)) {

Have nsStyleImages::IsInitialValue(aLyaerIndex) to hide all these detail

::: layout/style/nsStyleStruct.cpp
@@ +2551,5 @@
> +  } else if (aType == LayerType::Mask) {
> +    mXRepeat = NS_STYLE_IMAGELAYER_REPEAT_NO_REPEAT;
> +    mYRepeat = NS_STYLE_IMAGELAYER_REPEAT_NO_REPEAT;
> +  } else {
> +    MOZ_ASSERT(false, "not supported layer type.");

MOZ_UNREACHABLE

@@ +2580,5 @@
> +    mOrigin = NS_STYLE_IMAGELAYER_ORIGIN_PADDING;    
> +  } else if (aType == LayerType::Mask) {
> +    mOrigin = NS_STYLE_IMAGELAYER_ORIGIN_BORDER;    
> +  } else {
> +    MOZ_ASSERT(false, "unsupported style image layer type.");

MOZ_UNREACHABLE

::: layout/style/nsStyleStruct.h
@@ +492,5 @@
> +    Background = 0,
> +    Mask
> +  };
> +
> +  nsStyleImageLayers(LayerType type);

nsStyleImageLayers() = delete, if possible

Comment 15

[Ting-Yu Lin [:TYLin] (UTC-8)]

Re comment 14:
s/MOZ_UNREACHABLE/MOZ_ASSERT_UNREACHABLE

Comment 16

[Astley Chen (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f39a534b5336

Comment 17

[Astley Chen (inactive)]

Attachment: Part 1 - add layer types to nsStyleImageLayers and layer initialization

* * *
try: -b do -p all -u all -t none
* * *
temp

Comment 18

[Astley Chen (inactive)]

Attachment: Part 2 - update w3c css tests

Comment 19

[Astley Chen (inactive)]

Attachment: Part 1 - add layer types to nsStyleImageLayers and layer initialization

Comment 20

[Astley Chen (inactive)]

Comment on attachment 8757181 [details] [diff] [review]
Part 2 - update w3c css tests

previous code was wrong and rectified in this patch.

Comment 21

[Astley Chen (inactive)]

Attachment: Part 1 - add layer types to nsStyleImageLayers and layer initialization

Comment 22

[Astley Chen (inactive)]

Comment on attachment 8757249 [details] [diff] [review]
Part 1 - add layer types to nsStyleImageLayers and layer initialization

Addressed "error: bad implicit conversion constructor for 'nsStyleImageLayers'" on try.

Comment 23

[Cameron McCormack (:heycam)]

(Sorry for the bug title spam, didn't realize mask-repeat is being handled in another bug.)

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8757249 [details] [diff] [review]
Part 1 - add layer types to nsStyleImageLayers and layer initialization

Review of attachment 8757249 [details] [diff] [review]:
-----------------------------------------------------------------

Can we avoid storing the layer type in the nsStyleImageLayers object?  It looks like we generally pass in the layer type into functions on the nsStyleImageLayers object.  We only use mLayerType outside of the constructor in nsComputedDOMStyle::DoGetMask, and in there we know this is for a mask-* property so we can just pass in nsStyleImageLayers::LayerType::Mask directly.

::: layout/style/nsComputedDOMStyle.cpp
@@ +6103,5 @@
>        firstLayer.mOrigin != NS_STYLE_IMAGELAYER_ORIGIN_PADDING ||
>        firstLayer.mComposite != NS_STYLE_MASK_COMPOSITE_ADD ||
>        firstLayer.mMaskMode != NS_STYLE_MASK_MODE_MATCH_SOURCE ||
>        !firstLayer.mPosition.IsInitialValue() ||
> +      !firstLayer.mRepeat.IsInitialValue(svg->mMask.GetLayerType()) ||

Just pass in nsStyleImageLayers::LayerType::Mask.

::: layout/style/nsStyleStruct.cpp
@@ +2559,3 @@
>  }
>  
>  nsStyleImageLayers::Layer::Layer()

Can you add a comment in nsStyleStruct.h that this constructor does not initialize mRepeat or mOrigin and that Initialize() must be called to do that.

@@ +2573,5 @@
>  nsStyleImageLayers::Layer::~Layer()
>  {
>  }
>  
> +void nsStyleImageLayers::Layer::Initialize(nsStyleImageLayers::LayerType aType)

Newline after "void".

@@ +2582,5 @@
> +    mOrigin = NS_STYLE_IMAGELAYER_ORIGIN_PADDING;
> +  } else if (aType == LayerType::Mask) {
> +    mOrigin = NS_STYLE_IMAGELAYER_ORIGIN_BORDER;
> +  } else {
> +    MOZ_ASSERT_UNREACHABLE("unsupported layer type.");

Let's make the unexpected case still do something reasonable.  So something like:

if (aType == LayerType::Mask) {
  mOrigin = ...;
} else {
  MOZ_ASSERT(aType == LayerType::Background, ...);
  mOrigin = ...;
}

@@ +2650,5 @@
>  
>  nsStyleBackground::nsStyleBackground(StyleStructContext aContext)
> +  : mImage(nsStyleImageLayers::LayerType::Background)
> +  , mBackgroundColor(NS_RGBA(0, 0, 0, 0))
> +  

Nit: Remove the blank line.

Comment 25

[Astley Chen (inactive)]

Attachment: Part 1 - add layer types to nsStyleImageLayers and layer initialization. r?=heycam

Comment 26

[Astley Chen (inactive)]

Comment on attachment 8757775 [details] [diff] [review]
Part 1 - add layer types to nsStyleImageLayers and layer initialization. r?=heycam

Addressed review comment 24 and re-submitted the patch for 2nd review.

Comment 27

[Astley Chen (inactive)]

Attachment: Part 1 - add layer types to nsStyleImageLayers and layer initialization. r?=heycam

* * *
address review.

Comment 28

[Astley Chen (inactive)]

Comment on attachment 8757780 [details] [diff] [review]
Part 1 - add layer types to nsStyleImageLayers and layer initialization. r?=heycam

Updated patch since I forgot to qref on last upload. Sorry for this.

Comment 29

[Cameron McCormack (:heycam)]

Comment on attachment 8757181 [details] [diff] [review]
Part 2 - update w3c css tests

Review of attachment 8757181 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with this change.

::: layout/reftests/w3c-css/submitted/masking/mask-repeat-1-ref.html
@@ +23,5 @@
> +        width: 50px; height: 50px;
> +      }
> +
> +      #repeat-x {
> +        position: absolute; top: 0;

The left: 0; top: 0; properties aren't needed since these divs are positioned at (0, 0) anyway.  So either remove them from the #default, #repeat-x, #repeat-y rules or consistently set both of them.  (At the moment you set only top in #repeat-x and only left in #repeat-y.)

Comment 30

[Astley Chen (inactive)]

Attachment: Part 1 - add layer types to nsStyleImageLayers and layer initialization. r=heycam

Comment 31

[Astley Chen (inactive)]

Attachment: Part 2 - update w3c css masking mask-repeat ref test case. r=heycam

Comment 32

[Astley Chen (inactive)]

Comment on attachment 8758525 [details] [diff] [review]
Part 1 - add layer types to nsStyleImageLayers and layer initialization. r=heycam

Carry r+. r=heycam.

Comment 33

[Astley Chen (inactive)]

Comment on attachment 8758526 [details] [diff] [review]
Part 2 - update w3c css masking mask-repeat ref test case. r=heycam

Addressed review comment 29 and carry r+. r=heycam.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9db7e5634d7
Part 1 - add layer types to nsStyleImageLayers and layer initialization. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5832177887d
Part 2 - update w3c css masking mask-repeat ref test case. r=heycam

Comment 35

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=29183980&repo=mozilla-inbound

Comment 36

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/b8dddcad3c5e
Backed out changeset d5832177887d 
https://hg.mozilla.org/mozilla-central/rev/c2b9ede08259
Backed out changeset a9db7e5634d7 for test_smilCSSFromTo.xhtml test failures

Comment 37

[Astley Chen (inactive)]

(In reply to Carsten Book [:Tomcat] from comment #35)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=29183980&repo=mozilla-
> inbound

Thanks for doing this and sorry for the fault I'd made here. I'll address this failure carefully.

Comment 38

[Astley Chen (inactive)]

Attachment: Part 1 - add layer types to nsStyleImageLayers and layer initialization. r=heycam

Comment 39

[Astley Chen (inactive)]

Comment on attachment 8759910 [details] [diff] [review]
Part 1 - add layer types to nsStyleImageLayers and layer initialization. r=heycam

Addressed try failure. New try result looks good now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e1bdb5d41a5&selectedJob=21953549

Comment 40

[Astley Chen (inactive)]

good to go now.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0839610737b4
Part 1 - add layer types to nsStyleImageLayers and layer initialization. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/f930971eaa0d
Part 2 - update w3c css masking mask-repeat ref test case. r=heycam

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0839610737b4
https://hg.mozilla.org/mozilla-central/rev/f930971eaa0d

Comment 43

[Jean-Yves Perrier [:teoli]]

Updated https://developer.mozilla.org/en-US/Firefox/Releases/49#CSS
The page https://developer.mozilla.org/en-US/docs/Web/CSS/mask-origin was already correct.