[Webkit-unassigned] [Bug 113276] [CSSRegions] Implement offsetParent for elements inside named flow

Tue Apr 30 10:11:17 PDT 2013

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

Alexandru Chiculita <achicu at adobe.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |achicu at adobe.com

--- Comment #9 from Alexandru Chiculita <achicu at adobe.com>  2013-04-30 10:09:38 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=200117&action=review

Looks good, just a couple of comments below. Also, I think you need more tests for this patch.

>> LayoutTests/fast/regions/offsetParent-in-flow-thread.html:6
>>      <body>
> 
> I would like to see more tests for this, not only for the elements that are at the flow thread boundary, but also for other elements inside the flow thread that do not reach the flow thread boundary.
> Also, what happens if you collect the body element of a document in a flow thread and then you call offsetParent on that element, and offsetParent for other elements for such case.

Mihnea is right, you should add more tests. I also want a test to check for the correctness of the offset* values when the offsetParent==body. You might need to check for the different writing directions too.

>>> LayoutTests/fast/regions/offsetParent-in-flow-thread.html:9
>>> +            var body = document.body;
>> 
>> this variable doesn't seem to be used anywhere. if so, please remove it.
> 
> It is used below:
> 
> shouldBe("article.offsetParent", "body") - the second parameter is the variable's name.

I think Mihai has a point though, the string you pass to shouldBe is used for error messages and might be better to see "document.body" instead of just "body". Also, if you moved the code to a function it wouldn't work.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:504
> +            while (curr && curr != offsetParent) {

This is what I think the problem is here: the RenderObjects inside a flow are attached to the RenderFlowThread. However, the RenderFlowThread is attached to the RenderView directly, meaning that we are going to bypass the <body>'s RenderObject while iterating the parents.

I would rather check if the curr object is a RenderFlowThread and just get out of the loop. At that point we know we reached the offsetParent anyway. Also might be worth adding a comment explaining why the offset parent is not on the parents chain and link to the specification.

Also, can you add a separate test case that triggers this problem? I know another test was crashing, but it just seems to be a side-effect of that test-case.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:508
> +                

nit: I don't think there's a need for all these empty lines.

>> Source/WebCore/rendering/RenderObject.cpp:3001
>> +            return (document ? document->body() : 0);
> 
> When is document null? Also, as a preference, instead of returning from this loop, i would set node = document->body() and the loop will exit when will hit node->hasTagName(HTMLNames::bodyTag) condition.

If a node has a RenderObject it must be attached to the Document, so there must be no need to check for document.

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