[CSS Regions] Two regions reftests are failing the image match

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alan Stearns]

These two reftests pass on mac and efl, but are failing on chromium:

  fast/regions/positioned-objects-block-static-spanning-regions-rtl.html -> unexpected image mismatch
  fast/regions/positioned-objects-block-static-spanning-regions.html -> unexpected image mismatch

[Ojan Vafai]

Looking at the image diffs, it looks like the problem is that the green text on a green background isn't a solid green due to anti-aliasing. To my eye, it looks fine, so I'm inclined to say that it's not a bug per-se.

I bet these tests could easily be fixed by using the ahem font.

[Ojan Vafai]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20webkit.org&showLargeExpectations=true&tests=fast%2Fregions%2Fpositioned-objects-block-static-spanning-regions-rtl.html%20fast%2Fregions%2Fpositioned-objects-block-static-spanning-regions.html

[Alan Stearns]

If it's anti-aliasing, wouldn't the Ahem font have the same issues (around the em square instead of the glyph shape)?

For the rtl result diff I'm seeing some red leaking out of the bottom of the first region. That looks like a bug to me.

[Ojan Vafai]

(In reply to comment #3)
> If it's anti-aliasing, wouldn't the Ahem font have the same issues (around the em square instead of the glyph shape)?

Hm. Not sure. I thought Ahem never anti-aliased.

> For the rtl result diff I'm seeing some red leaking out of the bottom of the first region. That looks like a bug to me.

Oh yeah. You're right. That's definitely a bug.

[Mihnea Ovidenie]

Created attachment 178566 [details]
Patch

[Ojan Vafai]

Do we have a separate bug filed for the rtl failure? There still seems to be a bug there. Otherwise, this patch looks fine to me.

[Mihnea Ovidenie]

(In reply to comment #6)
> Do we have a separate bug filed for the rtl failure? There still seems to be a bug there. Otherwise, this patch looks fine to me.

I am unable to see any red issue for rtl test. I have tested it both on linux and mac, but still i saw no red.

[Ojan Vafai]

On the Chromium bots, there's red below the top box (to the right of the middle green box): http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=fast%2Fregions%2Fpositioned-objects-block-static-spanning-regions-rtl.html

Here's one of the expected.png files: http://build.chromium.org/f/chromium/layout_test_results/WebKit_Linux_32/results/layout-test-results/fast/regions/positioned-objects-block-static-spanning-regions-rtl-expected.png

That said, now that I look closely, this isn't a bug. It's the expected.html file that's overflowing, which is what you'd expect given the html.

[Mihnea Ovidenie]

(In reply to comment #8)
> On the Chromium bots, there's red below the top box (to the right of the middle green box): http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=fast%2Fregions%2Fpositioned-objects-block-static-spanning-regions-rtl.html
> 
> Here's one of the expected.png files: http://build.chromium.org/f/chromium/layout_test_results/WebKit_Linux_32/results/layout-test-results/fast/regions/positioned-objects-block-static-spanning-regions-rtl-expected.png
> 
> That said, now that I look closely, this isn't a bug. It's the expected.html file that's overflowing, which is what you'd expect given the html.

Right, but i have rewritten the tests because there are differences between platforms regarding default properties of fonts used in testing. With the new line-height, the tests should behave the same on all platforms. For instance, the GTK port had the same issue, with overflowing content in the first region.

[Mihnea Ovidenie]

Created attachment 178769 [details]
Patch 2

[Julien Chaffraix]

Comment on attachment 178769 [details]
Patch 2

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

> LayoutTests/ChangeLog:9
> +        values. I added a line-height:20px that would set the amount of space taken by the lines displayed in regions. I also changed the color of inline text(yellow), so that we

Shouldn't we just set up the font-family and font-size too while we are making the tests more robust? (thinking our usual font: 20px/1 Ahem)

> LayoutTests/ChangeLog:17
> +        * platform/efl/TestExpectations:

Also you probably need to update efl-wk2/TestExpectations which contains an entry for positioned-objects-block-static-spanning-regions.html

> LayoutTests/ChangeLog:19
> +

Why don't you also update qt?

> LayoutTests/platform/efl/TestExpectations:766
>  # CSS Regions Reftest issues.

This should be also removed as it was added at the same time as the entry.

[Mihnea Ovidenie]

Created attachment 179000 [details]
Patch 3

[Mihnea Ovidenie]

(In reply to comment #11)
> (From update of attachment 178769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178769&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        values. I added a line-height:20px that would set the amount of space taken by the lines displayed in regions. I also changed the color of inline text(yellow), so that we
> 
> Shouldn't we just set up the font-family and font-size too while we are making the tests more robust? (thinking our usual font: 20px/1 Ahem)
> 

I have added font-size/font-family for the text too.

> > LayoutTests/ChangeLog:17
> > +        * platform/efl/TestExpectations:
> 
> Also you probably need to update efl-wk2/TestExpectations which contains an entry for positioned-objects-block-static-spanning-regions.html
> 

Done.

> > LayoutTests/ChangeLog:19
> > +
> 
> Why don't you also update qt?
>

Done, nice catch :)
 
> > LayoutTests/platform/efl/TestExpectations:766
> >  # CSS Regions Reftest issues.
> 
> This should be also removed as it was added at the same time as the entry.

Done.

[Julien Chaffraix]

Comment on attachment 179000 [details]
Patch 3

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

> LayoutTests/fast/regions/positioned-objects-block-static-spanning-regions-expected.html:33
> +        line-height: 1.25;
> +        font-size: 16px;
> +        font-family: monospace;

Nit: The shorthand is good to use as it's more compact. Here it would be: font: 16px/1.25 monospace;

Also couldn't it be simplified to: font: 20px/1 monospace? (ie font-size: 20px and line-height: 1)

[Mihnea Ovidenie]

Created attachment 179067 [details]
Patch for landing

[Mihnea Ovidenie]

(In reply to comment #14)
> (From update of attachment 179000 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179000&action=review
> 
> > LayoutTests/fast/regions/positioned-objects-block-static-spanning-regions-expected.html:33
> > +        line-height: 1.25;
> > +        font-size: 16px;
> > +        font-family: monospace;
> 
> Nit: The shorthand is good to use as it's more compact. Here it would be: font: 16px/1.25 monospace;
> 

Done, thanks for the advice.

> Also couldn't it be simplified to: font: 20px/1 monospace? (ie font-size: 20px and line-height: 1)

No, because the text lines would have been too long for the first region and the text lines would have been broken at the region boundary. In this test, the 16px/1.25 is a better choice.

[WebKit Review Bot]

Comment on attachment 179067 [details]
Patch for landing

Clearing flags on attachment: 179067

Committed r137479: <http://trac.webkit.org/changeset/137479>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.