[CSS Regions] Overflow selection doesn't work properly

[Andrei Bucur]

Overflow belonging to a region is selectable in the next region. The selection is also painted in two places.

[Andrei Bucur]

Created attachment 227735 [details]
Repro

[Radu Stavila]

Created attachment 229541 [details]
Patch

[Andrei Bucur]

+ dhyatt

I think you had a problem with positionAtPoint in the new multi-col code. This patch seems to touch that area.

[Dave Hyatt]

Comment on attachment 229541 [details]
Patch

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

I'm kind of alarmed by the intrusiveness of this change. If there's no other way, then I would at least like it if you could avoid allowing the region to be null. We don't want people to mess up when they write new version of this method that have to recur into children.

> Source/WebCore/rendering/RenderBlock.cpp:2419
> +        if (!toRenderFlowThread(paintInfo->paintContainer)->objectShouldPaintInFlowRegion(this, paintInfo->renderNamedFlowFragment))

Consider renaming this method: objectShouldPaintInFlowRegion. You're using it for hit tests, so I don't think "paint" is a good term to be using.

> Source/WebCore/rendering/RenderBlock.cpp:3233
>      if (childrenInline())
>          return positionForPointWithInlineChildren(pointInLogicalContents);

Why are inline children not affected by this issue? They can be in different regions.

> Source/WebCore/rendering/RenderBlock.h:186
> +    virtual VisiblePosition positionForPoint(const LayoutPoint&, const RenderRegion* = nullptr) override;

It's really strange to me that this allows a nullptr. The problem I have with this is that someone could forget to pass the second argument when recurring into children, and it won't be a compile error. This seems fragile and error-prone to me.

[Radu Stavila]

Created attachment 229964 [details]
Patch implementing reviewer feedback - DO NOT LAND

[Radu Stavila]

Created https://bugs.webkit.org/show_bug.cgi?id=132050 for the renaming of objectShouldPaintInFlowRegion.

[Dave Hyatt]

Comment on attachment 229964 [details]
Patch implementing reviewer feedback - DO NOT LAND

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

Close! One issue.

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:921
> +                return multiColumnFlowThread()->positionForPoint(point, region);

This is wrong. You should be passing "this" in here... it's similar to what you did for RenderRegion.cpp.

> Source/WebCore/rendering/RenderMultiColumnSet.cpp:958
> +                return multiColumnFlowThread()->positionForPoint(point, region);

Same here.

[Dave Hyatt]

Comment on attachment 229964 [details]
Patch implementing reviewer feedback - DO NOT LAND

r=me

[Radu Stavila]

Created attachment 230152 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 230152 [details]
Patch for landing

Rejecting attachment 230152 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 230152, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/5440042235330560

[Radu Stavila]

Created attachment 230178 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 230178 [details]
Patch for landing

Clearing flags on attachment: 230178

Committed r167803: <http://trac.webkit.org/changeset/167803>