Bug 98950 – Bad copy constructor in StyleRareNonInheritedData

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rik Cabanier]

The blend mode is not correctly inherited which causes problems when you try to query it later.

[Rik Cabanier]

Created attachment 168060 [details]
first try

[Zoltan Horvath]

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!

[Ryosuke Niwa]

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.

[Eric Seidel (no email)]

Comment on attachment 168060 [details]
first try

Yes, please test this.

[Ryosuke Niwa]

Comment on attachment 168060 [details]
first try

r- due to the lack of tests.

[Rik Cabanier]

(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

[Ryosuke Niwa]

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

[Rik Cabanier]

Created attachment 168281 [details]
Added a test

[Rik Cabanier]

Created attachment 168282 [details]
Fixed identation

[Ryosuke Niwa]

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?

[Rik Cabanier]

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

[Alexandru Chiculita]

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

[Rik Cabanier]

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

[Ryosuke Niwa]

Yeah, it'll be nice if we can simplify the test.

[Rik Cabanier]

Created attachment 168284 [details]
Simplified test

[Rik Cabanier]

Created attachment 168289 [details]
cleaned up output

[Rik Cabanier]

Created attachment 168306 [details]
Cleaned up output

[Kenneth Rohde Christiansen]

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?

[Rik Cabanier]

(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

[Rik Cabanier]

Created attachment 168311 [details]
more testfile cleanup

[Rik Cabanier]

Created attachment 168314 [details]
More testfile cleanup

[Ryosuke Niwa]

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>

[Eric Seidel (no email)]

Comment on attachment 168314 [details]
More testfile cleanup

The test is larger than needed, still, but LGTM.

[Rik Cabanier]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Rik Cabanier]

Created attachment 168343 [details]
strange failure. Try again for EWS output

[Build Bot]

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

[Rik Cabanier]

Created attachment 168347 [details]
Updated SVG transitions test

[Build Bot]

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

[Rik Cabanier]

Created attachment 168355 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 168355 [details]
Patch for landing

Clearing flags on attachment: 168355

Committed r131146: <http://trac.webkit.org/changeset/131146>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.