The blend mode is not correctly inherited which causes problems when you try to query it later.
Created attachment 168060 [details] first try
Comment on attachment 168060 [details] first try Yeap, good catch, it's a typo copy/paste from the other constructor. I would give r+ to it!
Comment on attachment 168060 [details] first try View in context: https://bugs.webkit.org/attachment.cgi?id=168060&action=review > Source/WebCore/ChangeLog:10 > + No new tests: no change in functionality. There should certainly be a behavior change! You should be able to verify the fix via getComputedStyle.
Comment on attachment 168060 [details] first try Yes, please test this.
Comment on attachment 168060 [details] first try r- due to the lack of tests.
(In reply to comment #3) > (From update of attachment 168060 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168060&action=review > > > Source/WebCore/ChangeLog:10 > > + No new tests: no change in functionality. > > There should certainly be a behavior change! You should be able to verify the fix via getComputedStyle. I tried to write a test with GetComputedStyle, but it was working fine in the old version. I'm unsure how I can test this fix
(In reply to comment #6) > I tried to write a test with GetComputedStyle, but it was working fine in the old version. > I'm unsure how I can test this fix You need to make WebKit use this copy constructor.
Created attachment 168281 [details] Added a test
Created attachment 168282 [details] Fixed identation
Comment on attachment 168282 [details] Fixed identation View in context: https://bugs.webkit.org/attachment.cgi?id=168282&action=review > LayoutTests/transitions/blendmode-transitions-expected.txt:3 > +CONSOLE MESSAGE: line 240: Failed to pause '-webkit-blend-mode' transition on element 'box' > +BOX > +PASS - "-webkit-blend-mode" property for "box" element at 0.5s saw something close to: difference What kind of output do we get without your fix?
(In reply to comment #10) > (From update of attachment 168282 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168282&action=review > > > LayoutTests/transitions/blendmode-transitions-expected.txt:3 > > +CONSOLE MESSAGE: line 240: Failed to pause '-webkit-blend-mode' transition on element 'box' > > +BOX > > +PASS - "-webkit-blend-mode" property for "box" element at 0.5s saw something close to: difference > > What kind of output do we get without your fix? you get the default value: 'normal'
View in context: https://bugs.webkit.org/attachment.cgi?id=168282&action=review Looks good! I think the test could be simplified. > LayoutTests/platform/chromium/TestExpectations:110 > +# Fails because chromium doesn't have blendmode I think it is better to just say that Blend Mode is not enabled on Chromium yet :) > LayoutTests/transitions/blendmode-transitions.html:17 > + -webkit-transition-duration: 1s; > + -webkit-transition-timing-function: linear; > + -webkit-transition-property: -webkit-box-shadow, text-shadow; > + -webkit-blend-mode: difference; Why not just have a simple test to check the computed style before and after the change? This looks like it could be a simple JS test instead. Anyway, using a transition seems a little too much :) > LayoutTests/transitions/blendmode-transitions.html:33 > + -webkit-box-shadow: 0 -20px 10px red, 0 20px 10px blue; > + text-shadow: 0 -20px 10px red, 0 20px 10px blue; Do you really need the shadows? > LayoutTests/transitions/resources/transition-test-helpers.js:166 > + case CSSPrimitiveValue.CSS_IDENT: I think you are still not testing this one. You should pass in an arbitrary string and see what happens and have it as a bad parsing test (also a JS only test could handle this).
(In reply to comment #12) > View in context: https://bugs.webkit.org/attachment.cgi?id=168282&action=review > > Looks good! I think the test could be simplified. > > > LayoutTests/platform/chromium/TestExpectations:110 > > +# Fails because chromium doesn't have blendmode > > I think it is better to just say that Blend Mode is not enabled on Chromium yet :) OK Will do > > > LayoutTests/transitions/blendmode-transitions.html:17 > > + -webkit-transition-duration: 1s; > > + -webkit-transition-timing-function: linear; > > + -webkit-transition-property: -webkit-box-shadow, text-shadow; > > + -webkit-blend-mode: difference; > > Why not just have a simple test to check the computed style before and after the change? This looks like it could be a simple JS test instead. Anyway, using a transition seems a little too much :) The value is only wrong DURING the transition :-) So, I have to check midway through. > > > LayoutTests/transitions/blendmode-transitions.html:33 > > + -webkit-box-shadow: 0 -20px 10px red, 0 20px 10px blue; > > + text-shadow: 0 -20px 10px red, 0 20px 10px blue; > > Do you really need the shadows? I need something to transition. Does it matter if it's a shadow? > > > LayoutTests/transitions/resources/transition-test-helpers.js:166 > > + case CSSPrimitiveValue.CSS_IDENT: > > I think you are still not testing this one. You should pass in an arbitrary string and see what happens and have it as a bad parsing test (also a JS only test could handle this). The result of getcomputedstyle is an CSS_IDENT. I will get a JS error without it.
Yeah, it'll be nice if we can simplify the test.
Created attachment 168284 [details] Simplified test
Created attachment 168289 [details] cleaned up output
Created attachment 168306 [details] Cleaned up output
Comment on attachment 168306 [details] Cleaned up output View in context: https://bugs.webkit.org/attachment.cgi?id=168306&action=review > LayoutTests/transitions/blendmode-transitions.html:44 > + > + <div id="box" class="box">BOX</div> > + > + Why double newlines?
(In reply to comment #18) > (From update of attachment 168306 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168306&action=review > > > LayoutTests/transitions/blendmode-transitions.html:44 > > + > > + <div id="box" class="box">BOX</div> > > + > > + > > Why double newlines? no reason. I will clean up
Created attachment 168311 [details] more testfile cleanup
Created attachment 168314 [details] More testfile cleanup
Comment on attachment 168314 [details] More testfile cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=168314&action=review > LayoutTests/transitions/blendmode-transitions.html:1 > +<!DOCTYPE> It should be <!DOCTYPE html>
Comment on attachment 168314 [details] More testfile cleanup The test is larger than needed, still, but LGTM.
(In reply to comment #22) > (From update of attachment 168314 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168314&action=review > > > LayoutTests/transitions/blendmode-transitions.html:1 > > +<!DOCTYPE> > > It should be <!DOCTYPE html> There are other tests that have the incorrect doctype. If you file a bug, I will fix it.
Comment on attachment 168314 [details] More testfile cleanup Rejecting attachment 168314 [details] from commit-queue. New failing tests: transitions/svg-transitions.html Full output: http://queues.webkit.org/results/14252760
Comment on attachment 168314 [details] More testfile cleanup Attachment 168314 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14254747 New failing tests: transitions/svg-transitions.html
Created attachment 168343 [details] strange failure. Try again for EWS output
Comment on attachment 168314 [details] More testfile cleanup Attachment 168314 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14255802 New failing tests: transitions/blendmode-transitions.html transitions/svg-transitions.html
Created attachment 168347 [details] Updated SVG transitions test
Comment on attachment 168347 [details] Updated SVG transitions test Attachment 168347 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14262385 New failing tests: transitions/blendmode-transitions.html
Created attachment 168355 [details] Patch for landing
Comment on attachment 168355 [details] Patch for landing Clearing flags on attachment: 168355 Committed r131146: <http://trac.webkit.org/changeset/131146>
All reviewed patches have been landed. Closing bug.