Closed Bug 970348 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Web Painting, defect)

Product:
Core
Component:
Web Painting
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: olaru, Assigned: olaru)

Details

Attachments

(9 files, 6 obsolete files)

01.background-blending-tests-1.patch
9.42 KB, patch
roc
: review+
Details | Diff | Splinter Review
03.background-blending-tests-5.3.6.patch
4.14 KB, patch
roc
: review+
Details | Diff | Splinter Review
04.background-blending-tests-5.3.8.patch
3.82 KB, patch
roc
: review+
Details | Diff | Splinter Review
05.background-blending-tests-5.3.11.patch
5.01 KB, patch
roc
: review+
Details | Diff | Splinter Review
02.background-blending-tests-5.3.10.patch
3.79 KB, patch
roc
: review+
Details | Diff | Splinter Review
06.background-blending-tests-5.3.2.patch
6.16 KB, patch
Details | Diff | Splinter Review
08.background-blending-tests-5.3.1.patch
7.56 KB, patch
Details | Diff | Splinter Review
09.background-blending-svg-image-fix.patch
602 bytes, patch
Details | Diff | Splinter Review
10.background-blend-mode-add-invalid-values.patch
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: 12 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.

Attachment

General

Creator:
Created:
Updated:
Size: