Astley Chen (inactive) Assignee
	Description • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 2 • 8 years ago

Comment on attachment 8732785 [details]
reference html

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

	Astley Chen (inactive) Assignee
	Comment 3 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 4 • 8 years ago

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

	u459114
	Comment 5 • 8 years ago

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.

	Cameron McCormack (:heycam)
	Comment 6 • 8 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 7 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 8 • 8 years ago

(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.

	Astley Chen (inactive) Assignee
	Comment 10 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 11 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 12 • 8 years ago

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

	u459114
	Comment 14 • 8 years ago

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

	Ting-Yu Lin [:TYLin] (PDT, UTC-7)
	Comment 15 • 8 years ago

Re comment 14:
s/MOZ_UNREACHABLE/MOZ_ASSERT_UNREACHABLE

	Astley Chen (inactive) Assignee
	Comment 16 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 17 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 20 • 8 years ago

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

previous code was wrong and rectified in this patch.

	Astley Chen (inactive) Assignee
	Comment 22 • 8 years ago

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.

	Cameron McCormack (:heycam)
	Comment 23 • 8 years ago

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

	Cameron McCormack (:heycam)
	Comment 24 • 8 years ago

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.

	Astley Chen (inactive) Assignee
	Comment 26 • 8 years ago

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.

	Astley Chen (inactive) Assignee
	Comment 27 • 8 years ago

* * *
address review.

	Astley Chen (inactive) Assignee
	Comment 28 • 8 years ago

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.

	Cameron McCormack (:heycam)
	Comment 29 • 8 years ago

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.)

	Astley Chen (inactive) Assignee
	Comment 32 • 8 years ago

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

Carry r+. r=heycam.

	Astley Chen (inactive) Assignee
	Comment 33 • 8 years ago

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.

	Pulsebot
	Comment 34 • 8 years ago

Pushed by tlin@mozilla.com:
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

	Carsten Book [:Tomcat]
	Comment 35 • 8 years ago

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

	Pulsebot
	Comment 36 • 8 years ago

Backout by cbook@mozilla.com:
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

	Astley Chen (inactive) Assignee
	Comment 37 • 8 years ago

(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.

	Astley Chen (inactive) Assignee
	Comment 39 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 40 • 8 years ago

good to go now.

	Pulsebot
	Comment 41 • 8 years ago

Pushed by cbook@mozilla.com:
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

	Carsten Book [:Tomcat]
	Comment 42 • 8 years ago
bugherder

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

	Jean-Yves Perrier [:teoli]
	Comment 43 • 8 years ago

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.