[webkit-reviews] review denied: [Bug 120769] [CSS Regions] Selection focusNode set to the "region" block, instead of the "source" block : [Attachment 214161] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 14 11:03:46 PDT 2013
Dave Hyatt <hyatt at apple.com> has denied Javier Fernandez
<jfernandez at igalia.com>'s request for review:
Bug 120769: [CSS Regions] Selection focusNode set to the "region" block,
instead of the "source" block
https://bugs.webkit.org/show_bug.cgi?id=120769
Attachment 214161: Patch
https://bugs.webkit.org/attachment.cgi?id=214161&action=review
------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=214161&action=review
r- for the writing mode issues. I want a vertical writing mode test added for
the next patch because of this.
> Source/WebCore/rendering/RenderBlock.cpp:3644
> + const RenderBlock &block = box.isRenderBlock() ? toRenderBlock(box) :
*box.containingBlock();
Surprised the style code didn't complain on this line. Move the & to be next to
RenderBlock, i.e.,
const RenderBlock& block
> Source/WebCore/rendering/RenderRegion.cpp:72
> + if (point.y() < 0)
> + return LayoutPoint(0, m_flowThreadPortionRect.y());
Not writing-mode-aware. For vertical writing modes it is x you need to check.
> Source/WebCore/rendering/RenderRegion.cpp:74
> + if (point.y() > m_flowThreadPortionRect.height()) {
Same problem.
> Source/WebCore/rendering/RenderRegion.cpp:82
> + return LayoutPoint(point.x(), point.y() + m_flowThreadPortionRect.y());
Same problem, etc.
> Source/WebCore/rendering/RenderRegion.cpp:88
> + return RenderBlock::positionForPoint(point);
Shouldn't this be RenderBlockFlow::positionForPoint?
More information about the webkit-reviews
mailing list