Bug 130715

Summary: [CSS Regions] Overflow selection doesn't work properly
Product: WebKit Reporter: Andrei Bucur <abucur>
Component: Layout and RenderingAssignee: Radu Stavila <stavila>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hyatt, WebkitBugTracker
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 116295    
Attachments:
Description Flags
Repro
none
Patch
hyatt: review-
Patch implementing reviewer feedback - DO NOT LAND
hyatt: review+
Patch for landing
commit-queue: commit-queue-
Patch for landing none

Description Andrei Bucur 2014-03-25 03:17:38 PDT
Overflow belonging to a region is selectable in the next region. The selection is also painted in two places.
Comment 1 Andrei Bucur 2014-03-25 03:18:45 PDT
Created attachment 227735 [details]
Repro
Comment 2 Radu Stavila 2014-04-17 06:56:49 PDT
Created attachment 229541 [details]
Patch
Comment 3 Andrei Bucur 2014-04-17 08:42:03 PDT
+ dhyatt

I think you had a problem with positionAtPoint in the new multi-col code. This patch seems to touch that area.
Comment 4 Dave Hyatt 2014-04-22 12:21:58 PDT
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.
Comment 5 Radu Stavila 2014-04-23 01:41:49 PDT
Created attachment 229964 [details]
Patch implementing reviewer feedback - DO NOT LAND
Comment 6 Radu Stavila 2014-04-23 01:47:20 PDT
Created https://bugs.webkit.org/show_bug.cgi?id=132050 for the renaming of objectShouldPaintInFlowRegion.
Comment 7 Dave Hyatt 2014-04-24 08:57:46 PDT
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.
Comment 8 Dave Hyatt 2014-04-24 09:01:04 PDT
Comment on attachment 229964 [details]
Patch implementing reviewer feedback - DO NOT LAND

r=me
Comment 9 Radu Stavila 2014-04-25 02:07:12 PDT
Created attachment 230152 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2014-04-25 06:15:19 PDT
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
Comment 11 Radu Stavila 2014-04-25 06:19:00 PDT
Created attachment 230178 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2014-04-25 06:56:43 PDT
Comment on attachment 230178 [details]
Patch for landing

Clearing flags on attachment: 230178

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