Create test case for fallback logic between mask-mode and mask-type

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: u459114, Assigned: ethlin)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
css-masking spec said:
The mask-type property allows the author of the mask element to specify the preferred masking mode. However, the author can override this preference by setting the mask-mode value to something different than auto on the masked content.

Have test cases to verify this logic

[1] https://www.w3.org/TR/css-masking-1/#propdef-mask-type
(Reporter)

Updated

2 years ago
Assignee: nobody → cku

Updated

2 years ago
Target Milestone: --- → mozilla49
(Reporter)

Updated

2 years ago
Summary: Ask test case for fallback logic between mask-mode and mask-type → Create test case for fallback logic between mask-mode and mask-type
(Assignee)

Comment 1

2 years ago
I will work on this.
(Assignee)

Updated

2 years ago
Assignee: cku → ethlin
(Assignee)

Comment 2

2 years ago
Created attachment 8754701 [details] [diff] [review]
Add test to check the logic between mask-mode and mask-type.

The test case is for checking the logic between mask-type and mask-mode. So there should be six combinations ([mask-source, alpha, luminance] x [alpha, luminance]).
Attachment #8754701 - Flags: review?(cam)
Comment on attachment 8754701 [details] [diff] [review]
Add test to check the logic between mask-mode and mask-type.

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

::: layout/reftests/w3c-css/submitted/masking/mask-type.html
@@ +69,5 @@
> +    <div class="alpha-alpha"></div>
> +    <svg xmlns="http://www.w3.org/2000/svg">
> +      <defs>
> +        <mask id="svg-luminance" maskContentUnits="objectBoundingBox"
> +            style="mask-type: luminance">

Since most of the test details are in the <style> element above, I think it would make sense to move the mask-type declaration into the style sheet too.

@@ +71,5 @@
> +      <defs>
> +        <mask id="svg-luminance" maskContentUnits="objectBoundingBox"
> +            style="mask-type: luminance">
> +          <rect x="0" y="0" width="100%" height="100%"
> +              fill="blue" fill-opacity="1"/>

The initial value of fill-opacity is 1 anyway, so no need to include it here (and below).
Attachment #8754701 - Flags: review?(cam) → review+
(Assignee)

Comment 4

2 years ago
(In reply to Cameron McCormack (:heycam) from comment #3)
> Comment on attachment 8754701 [details] [diff] [review]
> Add test to check the logic between mask-mode and mask-type.
> 
> Review of attachment 8754701 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/w3c-css/submitted/masking/mask-type.html
> @@ +69,5 @@
> > +    <div class="alpha-alpha"></div>
> > +    <svg xmlns="http://www.w3.org/2000/svg">
> > +      <defs>
> > +        <mask id="svg-luminance" maskContentUnits="objectBoundingBox"
> > +            style="mask-type: luminance">
> 
> Since most of the test details are in the <style> element above, I think it
> would make sense to move the mask-type declaration into the style sheet too.
> 
I'll move it into style block.

> @@ +71,5 @@
> > +      <defs>
> > +        <mask id="svg-luminance" maskContentUnits="objectBoundingBox"
> > +            style="mask-type: luminance">
> > +          <rect x="0" y="0" width="100%" height="100%"
> > +              fill="blue" fill-opacity="1"/>
> 
> The initial value of fill-opacity is 1 anyway, so no need to include it here
> (and below).

Okay, I will remove it.
(Assignee)

Comment 5

2 years ago
Created attachment 8755370 [details] [diff] [review]
Add test to check the logic between mask-mode and mask-type. (carry r+: heycam)

Address :heycam's comments.
Attachment #8754701 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7d7c5acc9b1f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
One issue with the submission to the W3C reftests, from check-for-references.sh:

Missing link for ./masking/mask-type.html

which says that this test isn't going to be recognized as a reftest by the W3C harness due to lack of a link rel="match".


Also, mask-type.html is probably not a sufficiently-unique name for a reftest.  The name should say a little bit more about what's being tested.
Flags: needinfo?(ethlin)
(Assignee)

Updated

2 years ago
Blocks: 1276435
(Assignee)

Comment 10

2 years ago
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #9)
> One issue with the submission to the W3C reftests, from
> check-for-references.sh:
> 
> Missing link for ./masking/mask-type.html
> 
> which says that this test isn't going to be recognized as a reftest by the
> W3C harness due to lack of a link rel="match".
> 
> 
> Also, mask-type.html is probably not a sufficiently-unique name for a
> reftest.  The name should say a little bit more about what's being tested.

Sorry for that. I will be careful. I opened bug 1276435 to fix it.
Flags: needinfo?(ethlin)
You need to log in before you can comment on or make changes to this bug.