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.
Created attachment 210608 [details] Test to reproduce the issue.
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 ?
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.
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.
(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.
Created attachment 212560 [details] Special case in the RenderBlock::positionForPoint implementation for Region blocks.
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.
Created attachment 213166 [details] Patch
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?
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.
Created attachment 213405 [details] Patch
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).
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).
Created attachment 213876 [details] Patch
Created attachment 214161 [details] Patch patch rebased and some changes on mapRegionPointIntoFlowThreadCoordinates function to consider the X coordinate as well.
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?
Created attachment 214452 [details] Patch made the patch writing-mode aware and provided the corresponding Layout Tests.
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
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
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
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
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
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
Created attachment 214485 [details] Patch Removing font description from both tests which were passing before adding the writing-mode awareness logic.
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
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
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
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
Created attachment 214719 [details] Patch Besides writing-mode awareness logic, mapRegionPointIntoFlowThreadCoordinates uses now '>=' to detect the region's bottom margin.
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
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
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
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
Created attachment 214775 [details] Patch Besides writing-mode awareness logic, adapting the algorithm to the new RenderNamedFlowFragment class.
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
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
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
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
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
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
Created attachment 214956 [details] Patch Adjusting the vertical tests to make them fit into the DRT window.
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
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
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
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
Created attachment 214987 [details] Patch Adjusting the vertical tests to make them fit into the DRT window.
Created attachment 214988 [details] Patch Adjusting the vertical tests to make them fit into the DRT window.
Created attachment 215277 [details] Patch Considering the X coordinate too.
Created attachment 215484 [details] Patch RenderRegion::positionForPoint with public visibility in order to use renderNamedFlowFragment() instead of the generic firstChild().
Created attachment 215498 [details] Patch Patch rebased.
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?
(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.
Created attachment 216553 [details] Patch Added RenderBlockFlow::positionForPoint as requested in the last review.
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
Created attachment 216569 [details] Patch Added RenderBlockFlow::positionForPoint as requested in the last review.
Comment on attachment 216569 [details] Patch Clearing flags on attachment: 216569 Committed r159057: <http://trac.webkit.org/changeset/159057>
All reviewed patches have been landed. Closing bug.