[CSS Regions] clipping rectangle for "overflow: hidden" should be based on the padding box, not the content box

[Mihai Maerean]

Created attachment 208266 [details]
repro file

CSS2.1, 11. Visual effects:

Whenever overflow occurs, the 'overflow' property specifies whether a box is clipped to its padding edge
http://www.w3.org/TR/CSS2/visufx.html

[Mihai Maerean]

Created attachment 209757 [details]
patch

[Radu Stavila]

Comment on attachment 209757 [details]
patch

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

> Source/WebCore/ChangeLog:24
> +        (WebCore::RenderRegion::transformedRectForClippingAtPaddingBox): Takes the clipping rectangle that's the size of

Duplicate comment?

[Andrei Bucur]

Comment on attachment 209757 [details]
patch

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

Does it work with various writing modes?

> LayoutTests/fast/regions/clip-to-padding-box-expected.html:8
> +				font-size: 125%;

I think explicitly setting the font size would be a good idea.

> LayoutTests/fast/regions/clip-to-padding-box.html:9
> +				margin-left: 3px;

These values apply for other ports as well?

> Source/WebCore/ChangeLog:10
> +        Except for the 1st region, we can't allow clipping above the content rectangle because that would paint content

You mean we can't allow painting?

> Source/WebCore/ChangeLog:11
> +        layed out in the previous region in the chain.

layed -> laid

> Source/WebCore/ChangeLog:27
> +        layed out in the previous region in the chain.

layed -> laid

> Source/WebCore/rendering/RenderRegion.cpp:144
> +    LayoutUnit deltaX = style()->overflowX() != OVISIBLE ? -paddingStart() : LayoutUnit();

I think you should use something like max(0, (paddingStart - (flowThreadPortionOverflowRect.x() - flowThreadPortionRect.x())) instead of paddingStart. You are painting from flowThreadPortionRect.x()/y() converted to regions coordinate while the regionClippingRect is expanded to account for the flowThreadPortionOverflowRect.
Even better, uniting the regionClippingRect with the padding box of the region should give you the right result without too much math.

The same for the the other deltas.

> Source/WebCore/rendering/RenderRegion.cpp:147
> +    // paint content layed out in the previous region in the chain.

layed -> laid

[Mihai Maerean]

Created attachment 209984 [details]
patch. incorporates the feedback: works with writing modes, defines font in tests, spelling.

to do: newly visible text should be selectable.

[Mihai Maerean]

Comment on attachment 209984 [details]
patch. incorporates the feedback: works with writing modes, defines font in tests, spelling.

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

> LayoutTests/ChangeLog:3
> +        [CSS Regions] clipping rectangle for "overflow: hidden" should be based on the padding box, not the content box

after the visual overflow patch, this patch doesn't pass the attached layout tests anymore.

[Radu Stavila]

Created attachment 223861 [details]
Tests

This problem was solved by https://bugs.webkit.org/show_bug.cgi?id=118665 and https://bugs.webkit.org/show_bug.cgi?id=128590.
Two writing modes (horiz-bt and vert-rl) are still not working properly and a separate issue has been created for them (https://bugs.webkit.org/show_bug.cgi?id=128600).

[Radu Stavila]

Created attachment 223864 [details]
Tests

[WebKit Commit Bot]

Comment on attachment 223864 [details]
Tests

Clearing flags on attachment: 223864

Committed r163880: <http://trac.webkit.org/changeset/163880>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.