[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





--- Comment #10 from Javier Fernandez <jfernandez at igalia.com>  2013-10-02 15:24:31 PST ---
I'll post a patch with all the suggested changes, but first of all it should be clarified whether the approach of using the flow thread rect is valid or not. 

(In reply to comment #9)
> (From update of attachment 213166 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213166&action=review
> 
> 
> The approach here of computing a point within the flow thread portion rect seems peculiar to me.

If I've understood it correctly, the m_flowThreadPortionRect describes the region in flow thread coordinates. My idea consists in mapping the RenderRegion point to flow thread coordinates to find out the block with the content rendered by that region.


> > 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.

The idea here is that when the point is, for instance, at the region bottom margin, the VisiblePosition pointing to the bottom boundary in flow thread coordinates. 

The reason of the -1 is that this point will be used in the RenderBlock class to determine which Region the block is associated to. In order to do that, the regionAtBlockOffset method is called, which performs an inorder traversal search via the PODIntervalTree class. Setting just maxY() led to an incorrect region retrieved by the regionAtBlockOffset method.

I also had doubts about this approach and shared your concerns of fractional positioning. I think I'll need to think more about this. 

> 
> > 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");

This test will be handled by the RenderBlock class, since the area below the region content boxes (in case of regions before the last one) hit directy the Flow Thread block. The Region is only hit when reaching the border. Hence, this tests validates the new isChildHitTestCandidate function.

> > +
> > +                clearSelection();
> > +
> > +                selectFromIdToPosition("word1", 100, 151);
> > +                checkResult("result2", "word2", "5", "word1 inside region inside region inside region inside region word2");

    if (point.y() > m_flowThreadPortionRect.height()) // hitting bottom margin, padding, border.
        newPoint.setY(m_flowThreadPortionRect.maxY() - 1);

> > +
> > +                clearSelection();
> > +
> > +                selectFromIdToPosition("word3", 100, 330);
> > +                checkResult("result3", "word5", "0", "word5 inside region inside region inside region inside region word6 ");

    else if (point.y() < 0) // hitting top margin, padding, border.
        newPoint.setY(m_flowThreadPortionRect.y());

> > +
> > +                clearSelection();
> > +
> > +                selectFromIdToPosition("word5", 100, 440);
> > +                checkResult("result4", "word6", "5", "word5 inside region inside region inside region inside region word6");

    else // hitting region area (content boxes will be handled by RenderBlock class)
        newPoint.setY(point.y() + m_flowThreadPortionRect.y());

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

So, I think those four tests do cover all the branches.

In addition, the 2 layout tests validate different branches of the already implemented RenderBlock::positionForPoint logic. In one of the tests, the -webkit-region-break-before style rule is used, while in the other one the Flow Thread block has got inline children.

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