Bug 119546 - [CSS Regions] clipping rectangle for "overflow: hidden" should be based on the padding box, not the content box
Summary: [CSS Regions] clipping rectangle for "overflow: hidden" should be based on th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Radu Stavila
URL:
Keywords: AdobeTracked
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2013-08-07 07:30 PDT by Mihai Maerean
Modified: 2014-02-11 09:28 PST (History)
7 users (show)

See Also:


Attachments
repro file (428 bytes, text/html)
2013-08-07 07:30 PDT, Mihai Maerean
no flags Details
patch (12.12 KB, patch)
2013-08-27 05:48 PDT, Mihai Maerean
no flags Details | Formatted Diff | Diff
patch. incorporates the feedback: works with writing modes, defines font in tests, spelling. (28.54 KB, patch)
2013-08-29 08:22 PDT, Mihai Maerean
no flags Details | Formatted Diff | Diff
Tests (12.03 KB, patch)
2014-02-11 07:44 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
Tests (12.03 KB, patch)
2014-02-11 08:09 PST, Radu Stavila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Maerean 2013-08-07 07:30:38 PDT
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
Comment 1 Mihai Maerean 2013-08-27 05:48:41 PDT
Created attachment 209757 [details]
patch
Comment 2 Radu Stavila 2013-08-27 06:11:07 PDT
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?
Comment 3 Andrei Bucur 2013-08-27 07:06:36 PDT
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
Comment 4 Mihai Maerean 2013-08-29 08:22:56 PDT
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.
Comment 5 Mihai Maerean 2014-02-05 23:50:48 PST
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.
Comment 6 Radu Stavila 2014-02-11 07:44:05 PST
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).
Comment 7 Radu Stavila 2014-02-11 08:09:19 PST
Created attachment 223864 [details]
Tests
Comment 8 WebKit Commit Bot 2014-02-11 09:28:40 PST
Comment on attachment 223864 [details]
Tests

Clearing flags on attachment: 223864

Committed r163880: <http://trac.webkit.org/changeset/163880>
Comment 9 WebKit Commit Bot 2014-02-11 09:28:42 PST
All reviewed patches have been landed.  Closing bug.