[Webkit-unassigned] [Bug 120769] [CSSRegions] Selection focusNode set to the "region" block, instead of the "source" block

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=120769


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #213166|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #9 from Darin Adler <darin at apple.com>  2013-10-02 09:33:47 PST ---
(From update of attachment 213166)
View in context: https://bugs.webkit.org/attachment.cgi?id=213166&action=review

Looks generally good. Some things that must be fixed before landing. Someone a bit more expert in rendering might have comments on the basic approach beyond what I have said here.

> Source/WebCore/rendering/RenderBlock.cpp:4482
> +static inline bool isChildHitTestCandidate(RenderBox* box, RenderRegion *region, const LayoutPoint& point)

The RenderBox* argument should be RenderBox&, since it’s never null.

The RenderRegion* argument is formatted incorrectly. The * should be next to the type, not the argument name.

> Source/WebCore/rendering/RenderBlock.cpp:4489
> +        if (box->isRenderBlock())
> +            inRegion = toRenderBlock(box)->regionAtBlockOffset(point.y()) == region;
> +        else
> +            inRegion = box->containingBlock()->regionAtBlockOffset(point.y()) == region;

I think this is better:

    if (!isChildHitTestCandidate(box))
        return false;
    if (!region)
        return true;
    RenderBlock& block = box.isRenderBlock() ? toRenderBlock(box) : *box.containingBlock();
    return block.regionAtBlockOffset(point.y()) == region;

For one thing, we save extra work when isChildHitTestCandidate is false.

> Source/WebCore/rendering/RenderRegion.cpp:68
> +    LayoutPoint newPoint(0, point.y());

The approach here of computing a point within the flow thread portion rect seems peculiar to me.

> Source/WebCore/rendering/RenderRegion.cpp:69
> +    if (point.y() > m_flowThreadPortionRect.height()) // hitting bottom margin, padding, border, ...

No idea what these "…" mean. I don’t think these comments are helping much.

> Source/WebCore/rendering/RenderRegion.cpp:70
> +        newPoint.setY(m_flowThreadPortionRect.maxY() - 1);

The - 1 here seems quite strange to me and points to the approach possibly being incorrect. I understand the desire to pass in a point "at the bottom", but keep in mind that with fractional positioning there could be a box that is less than 1 point thick and it also is not clear to me that hit testing is always about a 1x1 box with the point at the top left corner.

> Source/WebCore/rendering/RenderRegion.cpp:71
> +    else if (point.y() < 0) // hitting top margin, padding, border, ...

Same thought here about the "…".

> Source/WebCore/rendering/RenderRegion.cpp:76
> +    if (m_flowThread->firstChild()) // checing out for empty region blocks

No idea what "checing out" means.

> Source/WebCore/rendering/RenderRegion.h:109
> +    virtual VisiblePosition positionForPoint(const LayoutPoint&);

Need to say OVERRIDE here. Also, is there a reason this needs to be public? If not, it should be private instead for now.

> LayoutTests/fast/regions/position-for-point.html:94
> +                clearSelection();
> +
> +                selectFromIdToPosition("word1", 100, 140);
> +                checkResult("result1", "word2", "5", "word1 inside region inside region inside region inside region word2");
> +
> +                clearSelection();
> +
> +                selectFromIdToPosition("word1", 100, 151);
> +                checkResult("result2", "word2", "5", "word1 inside region inside region inside region inside region word2");
> +
> +                clearSelection();
> +
> +                selectFromIdToPosition("word3", 100, 330);
> +                checkResult("result3", "word5", "0", "word5 inside region inside region inside region inside region word6 ");
> +
> +                clearSelection();
> +
> +                selectFromIdToPosition("word5", 100, 440);
> +                checkResult("result4", "word6", "5", "word5 inside region inside region inside region inside region word6");

Do these five tests really cover all the branches in the new code?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.