Bug 71931 – CSS 2.1 failure: outline-color-* tests fail

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

[Robert Hogan]

They do not render an outline when the div has zero size

[Robert Hogan]

Created attachment 114315 [details]
Patch

[Robert Hogan]

Created attachment 114322 [details]
Patch

[Julien Chaffraix]

Comment on attachment 114322 [details]
Patch

Looks great. Too bad the EWS was borked the day you submitted the patch.

2 comments:
* it looks like most of the tests would love being reftests using the near-to-landing patch on bug 60605. Do you think could be possible without having you rewrite all the tests?
* the change is painting related so do we need the DRT dumps? If yes, they look like they should not be Chromium specific.

[Robert Hogan]

(In reply to comment #3)
> (From update of attachment 114322 [details])
> Looks great. Too bad the EWS was borked the day you submitted the patch.
> 
> 2 comments:
> * it looks like most of the tests would love being reftests using the near-to-landing patch on bug 60605. Do you think could be possible without having you rewrite all the tests?

They're from the CSS 2.1 test suite so although I agree they would be better as reftests the place to do that is in the public css test suite I think.

> * the change is painting related so do we need the DRT dumps? If yes, they look like they should not be Chromium specific.

Yes, unfortunately the text in the tests makes their results platform specific. If you look at CSS2.1/20110323 all the results sit in platform folders.

On a positive note there *are* some ref tests in the CSS2.1 test suite so we will be able to adopt them as such once 60605 is landed.

[Julien Chaffraix]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 114322 [details] [details])
> > Looks great. Too bad the EWS was borked the day you submitted the patch.
> > 
> > 2 comments:
> > * it looks like most of the tests would love being reftests using the near-to-landing patch on bug 60605. Do you think could be possible without having you rewrite all the tests?
> 
> They're from the CSS 2.1 test suite so although I agree they would be better as reftests the place to do that is in the public css test suite I think.

I was thinking something along those line would happen. Could you make sure to post a follow-up on the public mailing list pointing this short-coming. For most browsers, this would be a good change as this would avoid the need of many different baselines.

> > * the change is painting related so do we need the DRT dumps? If yes, they look like they should not be Chromium specific.
> 
> Yes, unfortunately the text in the tests makes their results platform specific. If you look at CSS2.1/20110323 all the results sit in platform folders.

Fair enough, I should have known that the explanations for each tests would get us...

[Robert Hogan]

Committed r100652: <http://trac.webkit.org/changeset/100652>

[Robert Hogan]

See bug 12042

[Robert Hogan]

Was rolled out by bug 72649 and 72648

[Robert Hogan]

Created attachment 115843 [details]
Patch

[Julien Chaffraix]

Comment on attachment 115843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115843&action=review

> Source/WebCore/rendering/RenderObject.cpp:1155
> +    if (outer.isEmpty())

It took me some time to figure out why this was needed. Please add a comment about the need to bail out when offset (could you rename it to outlineOffset for coherency) is negative and bigger than outlineWidth (mentioning fast/borders/outline-offset-min-assert.html and/or bug 12042). Also this is a FIXME as this does not match FireFox anymore!

[Robert Hogan]

Committed r102526: <http://trac.webkit.org/changeset/102526>