Bug 130715

Created attachment 227735 [details] Repro

Created attachment 229541 [details] Patch

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

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.

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

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

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 on attachment 229964 [details] Patch implementing reviewer feedback - DO NOT LAND r=me

Created attachment 230152 [details] Patch for landing

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

Created attachment 230178 [details] Patch for landing

Comment on attachment 230178 [details] Patch for landing Clearing flags on attachment: 230178 Committed r167803: <http://trac.webkit.org/changeset/167803>