Closed Bug 970348 Opened 6 years ago Closed 6 years ago

Add extra tests for the background-blend-mode CSS property

Categories

(Core :: Web Painting, defect, minor)

x86
All
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: olaru, Assigned: olaru)

Details

Attachments

(9 files, 6 obsolete files)

9.42 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.14 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.82 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.01 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.79 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.16 KB, patch
Details | Diff | Splinter Review
7.56 KB, patch
Details | Diff | Splinter Review
602 bytes, patch
Details | Diff | Splinter Review
1.20 KB, patch
heycam
: review+
Details | Diff | Splinter Review
There is a work-in-progress test plan for the CSS blending and compositing spec at:
http://test.csswg.org/source/test-plans/compositing-1/css-blending-test-plan-proposal.html. This is an older version of the document, and the newer draft can be found at:
http://mire.github.io/css-blending-test-plan-proposal/css-blending-test-plan-proposal.html

The test plan has a number of tests for the background-blend-mode property, covered in section 5.3 (4.3 in the old version).

Some of the tests in the document were implemented as reftests and submitted with the implementation of the background-blend-mode property, in issue 841601.

This issue is to track adding the implementation of the remaining tests.
Assignee: nobody → olaru
Attachment #8373577 - Flags: review?(roc)
Attachment #8373578 - Flags: review?(roc)
Attachment #8373579 - Flags: review?(roc)
Attachment #8373580 - Flags: review?(roc)
Attachment #8373582 - Flags: review?(roc)
Attachment #8373583 - Flags: review?(roc)
Attachment #8373584 - Flags: review?(roc)
Attachment #8373587 - Flags: review?(roc)
Attachment #8373588 - Flags: review?(roc)
Sorry if there are a lot of patches. There were quite a few tests, and I thought they would be easier to manage this way. I can fold a few if that makes things easier.

Also, Robert, I feel like I am overburdening you with review requests. Please let me know if I should add someone else.
Comment on attachment 8373578 [details] [diff] [review]
02.background-blending-tests-5.3.10.patch

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

::: layout/reftests/css-blending/background-blending-background-origin-content-box.html
@@ +8,5 @@
> +  background-origin: content-box;
> +  width: 200px;
> +  height: 200px;
> +  margin: 10px;
> +  background-blend-mode: multiply

For this test, wouldn't you want some padding so that the content-box is different from the border-box?
Comment on attachment 8373583 [details] [diff] [review]
06.background-blending-tests-5.3.2.patch

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

One thing I realized --- all these tests need <!DOCTYPE HTML>.
Attachment #8373583 - Flags: review?(roc) → review+
Comment on attachment 8373584 [details] [diff] [review]
07.background-blending-tests-5.3.5.patch

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

We probably don't really need this test if you add some invalid values for background-blend-mode to property_database.js.
I have added padding, and at the same time reduced the width and height to match the old, common ref.
Attachment #8373578 - Attachment is obsolete: true
Attachment #8373578 - Flags: review?(roc)
Attachment #8373688 - Flags: review?(roc)
Added <!DOCTYPE HTML> to all files.
Attachment #8373583 - Attachment is obsolete: true
Found and fixed a couple of extra files that were missing <!DOCTYPE HTML>.
Attachment #8373587 - Attachment is obsolete: true
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Comment on attachment 8373584 [details] [diff] [review]
> 07.background-blending-tests-5.3.5.patch
> 
> Review of attachment 8373584 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We probably don't really need this test if you add some invalid values for
> background-blend-mode to property_database.js.

I followed the test plan on this one, even though it seems a bit redundant. The purpose of the test is to check that when only the background shorthand is specified, the background-blend-mode is set to the default value.

I am not entirely sure what invalid values I should add in property_database.js, because I do not completely understand the purpose for the invalid values array. Is it to prevent the shorthand value of another background- property from being misinterpreted as being the value for background-blend-mode?
Flags: needinfo?(roc)
Someone who knows the style system better than I should answer that.
Cameron, see comment #17.
Flags: needinfo?(roc) → needinfo?(cam)
If the definition of background-blend-mode had some interesting cases where parsing of the property should fail -- for example if it took a <number> but only values >= 1 were allowed, then I'd add in a couple of values like -1, 0 and 0.5.  Since background-blend-mode only takes a single keyword, I would perhaps just add a completely wrong datatype value, like "10px" and a duplicate keyword like "multiply multiply".\

The only way invalid_values are used is in test_property_syntax_errors.html, where it checks that they don't cause a property to be added to the declaration when parsed.

For 07.background-blending-tests-5.3.5.patch, it looks like that's testing the initial value of the property, rather than the correct handling of an invalid value.  Initial values are already tested by having values in initial_values in property_database.js.  So 07.background-blending-tests-5.3.5.patch is strictly redundant with some tests in test_value_computation.html.  (And I think various tests will fail if incorrect values are used in initial_values.)
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #20)
> If the definition of background-blend-mode had some interesting cases where
> parsing of the property should fail -- for example if it took a <number> but
> only values >= 1 were allowed, then I'd add in a couple of values like -1, 0
> and 0.5.  Since background-blend-mode only takes a single keyword, I would
> perhaps just add a completely wrong datatype value, like "10px" and a
> duplicate keyword like "multiply multiply".\
> 
> The only way invalid_values are used is in test_property_syntax_errors.html,
> where it checks that they don't cause a property to be added to the
> declaration when parsed.
> 
> For 07.background-blending-tests-5.3.5.patch, it looks like that's testing
> the initial value of the property, rather than the correct handling of an
> invalid value.  Initial values are already tested by having values in
> initial_values in property_database.js.  So
> 07.background-blending-tests-5.3.5.patch is strictly redundant with some
> tests in test_value_computation.html.  (And I think various tests will fail
> if incorrect values are used in initial_values.)

Thanks for the explanation. I will remove the redundant test patch, and re-upload the following patches to be consistent with the removed code. I will also add a patch with some invalid values.
Attachment #8373584 - Attachment is obsolete: true
Attachment #8373690 - Attachment is obsolete: true
Attachment #8373584 - Flags: review?(roc)
Attachment #8373588 - Attachment is obsolete: true
Attachment #8377202 - Flags: review?(cam) → review+
Keywords: checkin-needed
I was having a hard time getting these commit messages into a pushlog/TBPL-friendly format due to the repeated information at the beginning of each, so I ended up folding the patches together. Feel free to yell at me if you're not happy with the outcome.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b629a1dfd431
Flags: in-testsuite+
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #26)
> I was having a hard time getting these commit messages into a
> pushlog/TBPL-friendly format due to the repeated information at the
> beginning of each, so I ended up folding the patches together. Feel free to
> yell at me if you're not happy with the outcome.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b629a1dfd431

Sound good. Sorry for inconveniencing you. I kept them like that for easier management and reviewing.
Totally fine and understandable!

https://hg.mozilla.org/mozilla-central/rev/b629a1dfd431
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.