Bug 120769 – [CSS Regions] Selection focusNode set to the "region" block, instead of the "source" block

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Javier Fernandez]

While selecting content inside a region block, the focusNode is being moved across the corresponding "source" block at the specific offset. 

Once the selection overflows the last Line Box element, the focusNode is set to the "region" block with offset 0. This is wrong since the "region" block has no children in the DOM Tree, hence no content can be retrieved from it. Besides, it does not follow the way regular blocks manage this situation, which keep the focusNode at the last children of the block with the maximum offset.

I've verified that this bug comes from the missing specific CSSRegions implementation of the RenderBlock::positionForPoint method, which derives the logic to the RenderBox::positionForPoint method.

[Javier Fernandez]

Created attachment 210608 [details]
Test to reproduce the issue.

[Javier Fernandez]

Created attachment 210629 [details]
WIP patch (no review needed)

This patch solves most of the issues related to the positionForPoint implementation for CSS Regions. there are still some problems when selection overflows rightwards, causing the remaining lines to be selected.

The patch uses the dom:Range defined for each RenderRegion instance. I was wondering whether it should be more elegant (not sure about efficiency yet) to determine the RenderBlock associated to the RenderRegion in the corresponding RenderFlowThred; hence, we could, I think, execute the rest of the code already implemented in the positionForPoint method. I mean, instead of getting the Position from the DOM nodes, using the lastRenderBox approach already implemented.

If we prefer to go that way, the question is: is there any way to get the RenderBlock child in the corresponding RenderFlowThread, associated to a RenderRegion ? I saw there are Maps and List to implement the opposite association, determining the RenderRegion associated to a RenderBlock. Im thinking on implementing a new Map/List associating the RenderBlock to the corresponding RenderRegion; do you think it makes sense ?

[Javier Fernandez]

Created attachment 211435 [details]
WIP patch - RenderRegion and RenderFlowThread positionForPoint implementation

This patch provides a different approach, avoiding accessing the DOM Ranges associated to each RenderRegion. The new idea is to map the RenderRegion point to the corresponding RenderFlowThread block.

[Mihnea Ovidenie]

Comment on attachment 211435 [details]
WIP patch - RenderRegion and RenderFlowThread positionForPoint implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=211435&action=review

> Source/WebCore/rendering/RenderBlock.cpp:4483
>      return createVisiblePosition(0, DOWNSTREAM);

I would like to have someone with much more experience review the approach but i would also like to see a real patch, with layout tests and explanations.

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

Why do you need to pass the region and the point here? Please note the the box may not always be a block, in which case you will hit a security assertion. Is the region check always required or only for the content that is inside the flow thread?

> Source/WebCore/rendering/RenderRegion.cpp:66
> +bool RenderRegion::isChildHitTestCandidate(RenderBox* box, const LayoutPoint& point)

The same function in RenderBlock is made static, basically the functions are the same. Again, the same observation about the cast from box to block. In fact, with the patch applied, it crashed my build.

> Source/WebCore/rendering/RenderRegion.cpp:72
> +VisiblePosition RenderRegion::positionForPoint(const LayoutPoint& point)

In the following patch, i would like to see a more detailed comment about what this function is trying to achieve, it will help review the approach/patch.

> Source/WebCore/rendering/RenderRegion.cpp:75
> +    if (thread->firstChild()) {

What are you trying to test here?

> Source/WebCore/rendering/RenderRegion.cpp:76
> +        RenderBox* candidate = toRenderBlock(thread->firstChild())->lastChildBox();

Again, if the first child of the thread is a replaced element, the conversion to block fails.

[Javier Fernandez]

(In reply to comment #4)
> I would like to have someone with much more experience review the approach but i would also like to see a real patch, with layout tests and explanations.
> 

Yes, of course. I'll provide that in the next patch.

> > Source/WebCore/rendering/RenderBlock.cpp:4486
> > +static inline bool isChildHitTestCandidate(RenderBox* box, RenderRegion *region, const LayoutPoint& point)
> 
> Why do you need to pass the region and the point here? Please note the the box may not always be a block, in which case you will hit a security assertion. Is the region check always required or only for the content that is inside the flow thread?

The region argument is precisely the key of the solution; the idea is that last candidate box inside a flow thread should belong to the region associated to the point. Current algorithm would select always the flow thread's last box as candidate, even when selection range covers only the first region. 

The region check is only required for content inside the flow thread, yes. 

The positionForPoint is always called by the flow thread's first child, while point is in the text region boundaries; once leaving them, the RenderRegion block takes the control. This logic could have been implemented as a new special case in the RenderBlock class, but I thought overwriting the positionForPoint method would be clearer.

I've already realized about the security assertions, indeed. I'll fix that.

> 
> > Source/WebCore/rendering/RenderRegion.cpp:66
> > +bool RenderRegion::isChildHitTestCandidate(RenderBox* box, const LayoutPoint& point)
> 
> The same function in RenderBlock is made static, basically the functions are the same. Again, the same observation about the cast from box to block. In fact, with the patch applied, it crashed my build.

Yeah, the same check has to be made for both cases, flow thread and render region. In the later, I have to chose the box candidate compatible with the region instance. It could be static, of course. I also thought about exporting the RenderBlock method, but this approach looked like simpler.

Again, very aware of the security assertions derived form the casts and working on it.
> 
> > Source/WebCore/rendering/RenderRegion.cpp:72
> > +VisiblePosition RenderRegion::positionForPoint(const LayoutPoint& point)
> 
> In the following patch, i would like to see a more detailed comment about what this function is trying to achieve, it will help review the approach/patch.

I'll do.

> 
> > Source/WebCore/rendering/RenderRegion.cpp:75
> > +    if (thread->firstChild()) {
> 
> What are you trying to test here?

It crashed to me when using empty regions; perhaps there is a better way to check it out. Some tests failed, actually.

> 
> > Source/WebCore/rendering/RenderRegion.cpp:76
> > +        RenderBox* candidate = toRenderBlock(thread->firstChild())->lastChildBox();
> 
> Again, if the first child of the thread is a replaced element, the conversion to block fails.

Yes.


Thanks for the feedback, Ill provide a new patch soon.

[Javier Fernandez]

Created attachment 212560 [details]
Special case in the RenderBlock::positionForPoint implementation for Region blocks.

[Mihnea Ovidenie]

Comment on attachment 212560 [details]
Special case in the RenderBlock::positionForPoint implementation for Region blocks.

View in context: https://bugs.webkit.org/attachment.cgi?id=212560&action=review

> Source/WebCore/rendering/RenderBlock.cpp:4463
> +    return isChildHitTestCandidate(box) && (region ? toRenderBlock(box)->regionAtBlockOffset(point.y()) == region : true);

Is it safe here to convert a box to RenderBlock? It is not clear to me why.

> Source/WebCore/rendering/RenderBlock.cpp:4482
> +    if (isRenderRegion()) {

Maybe you should you create a positionForPoint in RenderRegion so that you avoid casts etc.

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

You need to explain what you are doing here.

> Source/WebCore/rendering/RenderBlock.cpp:4493
> +            return toRenderBlock(thread->firstChild())->positionForPoint(newPoint);

Is it always safe to cast here? What happens if the first child is not a block, for instance an image? Also, what happens if the flow thread has more than one child:
<div style="-webkit-flow-into: flow"></div>
<div style="-webkit-flow-into: flow"></div>

> Source/WebCore/rendering/RenderBlock.cpp:4505
> +    RenderRegion* region = regionAtBlockOffset(point.y());

Not a huge fan of changing the RenderBlock::positionForPoint method with regions specific code.

[Javier Fernandez]

Created attachment 213166 [details]
Patch

[Darin Adler]

Comment on attachment 213166 [details]
Patch

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?

[Javier Fernandez]

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.

[Javier Fernandez]

Created attachment 213405 [details]
Patch

[Radu Stavila]

Comment on attachment 213166 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213166&action=review

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

No point in initialising newPoint's y with point.y() since you're going to override it by calling newPoint.setY(...) in all cases.

> Source/WebCore/rendering/RenderRegion.cpp:77
> +        return toRenderBlock(m_flowThread->firstChild())->positionForPoint(newPoint);

I think you should perform the firstChild() check before computing newPoint because you could end up computing it for nothing (since you're not using it if firstChild() == 0).

[Radu Stavila]

Comment on attachment 213405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213405&action=review

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

No point in initialising newPoint's y with point.y() since you're going to override it by calling newPoint.setY(...) in all cases.

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

Does this work well when the last region is overflowing downwards?

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

Same question for regions overflowing upwards (for instance by setting a negative top-margin on the flowed content).

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

I think you should perform the firstChild() check before computing newPoint because you could end up computing it for nothing (since you're not using it if firstChild() == 0).

[Javier Fernandez]

Created attachment 213876 [details]
Patch

[Javier Fernandez]

Created attachment 214161 [details]
Patch

patch rebased and some changes on mapRegionPointIntoFlowThreadCoordinates function to consider the X coordinate as well.

[Dave Hyatt]

Comment on attachment 214161 [details]
Patch

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?

[Javier Fernandez]

Created attachment 214452 [details]
Patch

made the patch writing-mode aware and provided the corresponding Layout Tests.

[Build Bot]

Comment on attachment 214452 [details]
Patch

Attachment 214452 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4029008

New failing tests:
fast/regions/selection/position-for-point-vert-lr.html
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/regions/selection/position-for-point-vert-rl.html
fast/regions/selection/position-for-point.html
fast/regions/selection/position-for-point-1.html

[Build Bot]

Created attachment 214457 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 214452 [details]
Patch

Attachment 214452 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4141013

New failing tests:
fast/regions/selection/position-for-point-vert-lr.html
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/regions/selection/position-for-point-vert-rl.html
fast/regions/selection/position-for-point.html
fast/regions/selection/position-for-point-1.html

[Build Bot]

Created attachment 214459 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 214452 [details]
Patch

Attachment 214452 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4029027

New failing tests:
fast/regions/selection/position-for-point-vert-lr.html
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/regions/selection/position-for-point-vert-rl.html
fast/regions/selection/position-for-point.html
fast/regions/selection/position-for-point-1.html

[Build Bot]

Created attachment 214461 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Javier Fernandez]

Created attachment 214485 [details]
Patch

Removing font description from both tests which were passing before adding the writing-mode awareness logic.

[Build Bot]

Comment on attachment 214485 [details]
Patch

Attachment 214485 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3575070

New failing tests:
fast/regions/selection/position-for-point-vert-lr.html
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/regions/selection/position-for-point-vert-rl.html
fast/regions/selection/position-for-point.html
fast/regions/selection/position-for-point-1.html

[Build Bot]

Created attachment 214495 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 214485 [details]
Patch

Attachment 214485 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3321307

New failing tests:
fast/regions/selection/position-for-point-vert-lr.html
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/regions/selection/position-for-point-vert-rl.html
fast/regions/selection/position-for-point.html
fast/regions/selection/position-for-point-1.html

[Build Bot]

Created attachment 214500 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Javier Fernandez]

Created attachment 214719 [details]
Patch

Besides writing-mode awareness logic, mapRegionPointIntoFlowThreadCoordinates uses now '>=' to detect the region's bottom margin.

[Build Bot]

Comment on attachment 214719 [details]
Patch

Attachment 214719 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/8528020

New failing tests:
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-vert-rl.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/regions/selection/position-for-point-vert-lr.html

[Build Bot]

Created attachment 214721 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 214719 [details]
Patch

Attachment 214719 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/8448023

New failing tests:
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-vert-rl.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/regions/selection/position-for-point-vert-lr.html

[Build Bot]

Created attachment 214722 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Javier Fernandez]

Created attachment 214775 [details]
Patch

Besides writing-mode awareness logic, adapting the algorithm to the new RenderNamedFlowFragment class.

[Build Bot]

Comment on attachment 214775 [details]
Patch

Attachment 214775 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/8878134

New failing tests:
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-vert-rl.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/regions/selection/position-for-point-vert-lr.html

[Build Bot]

Created attachment 214780 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 214775 [details]
Patch

Attachment 214775 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/8878152

New failing tests:
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-vert-rl.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/regions/selection/position-for-point-vert-lr.html

[Build Bot]

Created attachment 214787 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 214775 [details]
Patch

Attachment 214775 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/8798143

New failing tests:
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-vert-rl.html
fast/regions/selection/position-for-point-1-vert-rl.html
fast/regions/selection/position-for-point-vert-lr.html

[Build Bot]

Created attachment 214796 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Javier Fernandez]

Created attachment 214956 [details]
Patch

Adjusting the vertical tests to make them fit into the DRT window.

[Build Bot]

Comment on attachment 214956 [details]
Patch

Attachment 214956 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6078062

New failing tests:
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-1-vert-rl.html

[Build Bot]

Created attachment 214964 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 214956 [details]
Patch

Attachment 214956 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/9968005

New failing tests:
fast/regions/selection/position-for-point-1-vert-lr.html
fast/regions/selection/position-for-point-1-vert-rl.html

[Build Bot]

Created attachment 214968 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Javier Fernandez]

Created attachment 214987 [details]
Patch

Adjusting the vertical tests to make them fit into the DRT window.

[Javier Fernandez]

Created attachment 214988 [details]
Patch

Adjusting the vertical tests to make them fit into the DRT window.

[Javier Fernandez]

Created attachment 215277 [details]
Patch

Considering the X coordinate too.

[Javier Fernandez]

Created attachment 215484 [details]
Patch

RenderRegion::positionForPoint with public visibility in order to use renderNamedFlowFragment() instead of the generic firstChild().

[Javier Fernandez]

Created attachment 215498 [details]
Patch

Patch rebased.

[Dave Hyatt]

Comment on attachment 215498 [details]
Patch

r=me. 

Don't much like this bit though:

if (isRenderNamedFlowFragmentContainer())
    toRenderBlockFlow(this)->renderNamedFlowFragment()->positionForPoint(point);

Can't just subclass positionForPoint?

[Javier Fernandez]

(In reply to comment #51)
> (From update of attachment 215498 [details])
> r=me. 
> 
> Don't much like this bit though:
> 
> if (isRenderNamedFlowFragmentContainer())
>     toRenderBlockFlow(this)->renderNamedFlowFragment()->positionForPoint(point);
> 
> Can't just subclass positionForPoint?


Yes, Ill define RenderBlockFlow::positionForPoint(), which just redirects the call to the associated RenderNamedFlowFragment instance.

[Javier Fernandez]

Created attachment 216553 [details]
Patch

Added RenderBlockFlow::positionForPoint as requested in the last review.

[EFL EWS Bot]

Comment on attachment 216553 [details]
Patch

Attachment 216553 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/22539331

[Javier Fernandez]

Created attachment 216569 [details]
Patch

Added RenderBlockFlow::positionForPoint as requested in the last review.

[WebKit Commit Bot]

Comment on attachment 216569 [details]
Patch

Clearing flags on attachment: 216569

Committed r159057: <http://trac.webkit.org/changeset/159057>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.