[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 #5 from Javier Fernandez <jfernandez at igalia.com>  2013-09-17 14:38:14 PST ---
(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.

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