Bug 74219

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.

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.

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

Created attachment 178566 [details] Patch

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.

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

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.

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

Created attachment 178769 [details] Patch 2

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.

Created attachment 179000 [details] Patch 3

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

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)

Created attachment 179067 [details] Patch for landing

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

Comment on attachment 179067 [details] Patch for landing Clearing flags on attachment: 179067 Committed r137479: <http://trac.webkit.org/changeset/137479>

All reviewed patches have been landed. Closing bug.