Bug 113276 - [CSSRegions] Implement offsetParent for elements inside named flow
Summary: [CSSRegions] Implement offsetParent for elements inside named flow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: AdobeTracked
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2013-03-25 22:43 PDT by Mihnea Ovidenie
Modified: 2013-05-20 04:37 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.74 KB, patch)
2013-04-30 07:46 PDT, Radu Stavila
no flags Details | Formatted Diff | Diff
Patch (6.88 KB, patch)
2013-04-30 08:00 PDT, Radu Stavila
achicu: review-
achicu: commit-queue-
Details | Formatted Diff | Diff
Patch (13.42 KB, patch)
2013-05-07 05:20 PDT, Radu Stavila
achicu: review-
achicu: commit-queue-
Details | Formatted Diff | Diff
Patch (13.35 KB, patch)
2013-05-07 09:32 PDT, Radu Stavila
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (684.64 KB, application/zip)
2013-05-07 22:02 PDT, Build Bot
no flags Details
Patch (13.35 KB, patch)
2013-05-08 01:32 PDT, Radu Stavila
no flags Details | Formatted Diff | Diff
Patch (10.64 KB, patch)
2013-05-10 04:59 PDT, Radu Stavila
achicu: review-
achicu: commit-queue-
Details | Formatted Diff | Diff
Patch (10.66 KB, patch)
2013-05-14 04:16 PDT, Radu Stavila
darin: review+
darin: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (10.17 KB, patch)
2013-05-15 01:21 PDT, Radu Stavila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2013-03-25 22:43:09 PDT
Right now, offsetParent algorithm stops at named flow boundaries.. There is a discussion going on csswg mailing list about offsetParent for elements in regions. This bug is used to track the implementation and should be addressed after the resolution in the CSSRegions spec.
Comment 1 Mihnea Ovidenie 2013-04-26 05:44:56 PDT
http://dev.w3.org/csswg/css-regions/#cssomview-offset-attributes clarifies what should happen with the offsetParent
Comment 2 Radu Stavila 2013-04-30 07:46:44 PDT
Created attachment 200116 [details]
Patch
Comment 3 Mihai Maerean 2013-04-30 07:55:46 PDT
Comment on attachment 200116 [details]
Patch

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

> LayoutTests/ChangeLog:6
> +        [In the offsetParent algorithm, the nearest ancestor search skips from the topmost named flow elements directly to the body element.]

there's no need for []. please remove them.
you could add an URL for the part of spec that defines this behaviour (http://dev.w3.org/csswg/css-regions/#cssomview-offset-attributes).

> Source/WebCore/ChangeLog:6
> +        [In the offsetParent algorithm, the nearest ancestor search skips from the topmost named flow elements directly to the body element.]

same here with the [].
Comment 4 Radu Stavila 2013-04-30 08:00:01 PDT
Created attachment 200117 [details]
Patch
Comment 5 Mihai Maerean 2013-04-30 08:09:00 PDT
Comment on attachment 200117 [details]
Patch

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

> 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.
Comment 6 Radu Stavila 2013-04-30 08:12:48 PDT
Comment on attachment 200117 [details]
Patch

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

>> 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.
Comment 7 Mihnea Ovidenie 2013-04-30 09:01:08 PDT
Comment on attachment 200117 [details]
Patch

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

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

> Source/WebCore/ChangeLog:12
> +        end up calling curr->parent()->offsetForColumns(referencePoint) while curr->parent() was returning NULL.

It is not very clear to me - from your description - why you reached that. I suspect you were climbing up the RenderObject tree until you reached 0 for a curr object but you should not get there in the algorithm, you should probably stop the traversal sooner.

> 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.
Comment 8 Radu Stavila 2013-04-30 09:36:35 PDT
Comment on attachment 200117 [details]
Patch

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

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

Ok, I will add more tests.

>> Source/WebCore/ChangeLog:12
>> +        end up calling curr->parent()->offsetForColumns(referencePoint) while curr->parent() was returning NULL.
> 
> It is not very clear to me - from your description - why you reached that. I suspect you were climbing up the RenderObject tree until you reached 0 for a curr object but you should not get there in the algorithm, you should probably stop the traversal sooner.

The WebCore::RenderBoxModelObject::adjustedPositionRelativeToOffsetParent method contains this algorithm which climbs up the tree until it reaches the offsetParent. However, it does not account for the fact that the returned offsetParent could now be obtained by "jumping" directly to the BODY element. As such, before reaching the actual offsetParent, this simple algorithm will get to an object (RenderView I believe) which has no parent. As a consequence, calling curr->parent()->offsetForColumns(referencePoint) was causing a crash.

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

I considered it would be safer to check for NULL before using the pointer returned by the document() method. 

As for not interrupting the algorithm and just setting node = document->body(), I did not see the point in doing that since the spec states the body element will be returned, without any other checks. If you think this to be the better way (not using return), I will change it.
Comment 9 Alexandru Chiculita 2013-04-30 10:09:38 PDT
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.
Comment 10 Radu Stavila 2013-05-07 05:20:11 PDT
Created attachment 200893 [details]
Patch
Comment 11 Alexandru Chiculita 2013-05-07 08:34:42 PDT
Comment on attachment 200893 [details]
Patch

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

Looks good! I have a few small nits below.

> LayoutTests/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

Nit: this line usually comes after the bug link.

> LayoutTests/fast/regions/offsetLeft-offsetTop-in-flow-thread.html:4
> +		padding: 10px;

I think it's better to specify a font size here. This way you avoid differences between default styles. Also, Ahem font could reduce the differences across platforms.

> LayoutTests/fast/regions/offsetLeft-offsetTop-in-flow-thread.html:56
> +		shouldBe("table.offsetTop", "18");

Testing for an exact value in the test file would make it hard to rebate line on other platforms.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:500
> +            // In the offsetParent algorithm, the nearest ancestor search skips from the topmost named flow elements directly to the body element.

There is no element for the names flows. They are only render objects.

> Source/WebCore/rendering/RenderObject.cpp:3015
> +    // If this element is inside a named flow thread, the nearest ancestor search skips

I would avoid to use "element" when talking about render objects. It is a little confusing.
Comment 12 Radu Stavila 2013-05-07 09:10:58 PDT
Comment on attachment 200893 [details]
Patch

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

>> LayoutTests/fast/regions/offsetLeft-offsetTop-in-flow-thread.html:56
>> +		shouldBe("table.offsetTop", "18");
> 
> Testing for an exact value in the test file would make it hard to rebate line on other platforms.

If I use the Ahem font, would it be ok to keep testing for exact values?
Comment 13 Radu Stavila 2013-05-07 09:32:48 PDT
Created attachment 200919 [details]
Patch
Comment 14 Build Bot 2013-05-07 22:02:50 PDT
Comment on attachment 200919 [details]
Patch

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

New failing tests:
svg/text/inline-text-destroy-attributes-crash.xhtml
editing/selection/click-on-head-margin.html
fast/table/table-caption-moved-crash.html
editing/pasteboard/drag-drop-input-in-svg.svg
fast/frames/frameset-style-recalc.html
Comment 15 Build Bot 2013-05-07 22:02:52 PDT
Created attachment 201028 [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.2
Comment 16 Radu Stavila 2013-05-08 01:32:46 PDT
Created attachment 201042 [details]
Patch
Comment 17 Radu Stavila 2013-05-10 04:59:52 PDT
Created attachment 201335 [details]
Patch

I created a separate issue for the correct implementation of offsetLeft and offsetTop - https://bugs.webkit.org/show_bug.cgi?id=115899
Comment 18 Alexandru Chiculita 2013-05-13 09:46:02 PDT
Comment on attachment 201335 [details]
Patch

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

Thanks for reworking this. I still have a couple nits below.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:503
> +            // In the offsetParent algorithm, the nearest ancestor search skips from the topmost named flow objects directly

nit: I would mention this only applies on elements inside the regions flow: "In the offsetParent algorithm defined for elements inside CSS Regions flows," . Also add a link to that section here.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:504
> +            // to the body element, so the offsetParent might be the body

nit: I think "so the offsetParent might be the body" is not necessary. Make sure the phrases end with a dot.

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

Can offsetParent be null here? In that case you may need to change the order of the checks to avoid a null dereference.

> Source/WebCore/rendering/RenderObject.cpp:3019
> +        // The search reached the named flow thread, skip to the body element

nit: Missing dot. http://www.webkit.org/coding/coding-style.html#comments-sentences

> Source/WebCore/rendering/RenderObject.cpp:3020
> +        return document()->body()->renderBoxModelObject();

I think body() can return 0 in some early cases. You might as well just patch "curr" and let it go through.
Comment 19 Radu Stavila 2013-05-14 04:16:39 PDT
Created attachment 201697 [details]
Patch
Comment 20 Darin Adler 2013-05-14 09:53:31 PDT
Comment on attachment 201697 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:509
> +            // In the offsetParent algorithm defined for elements inside CSS Regions flows,
> +            // the nearest ancestor search skips from the topmost named flow object directly
> +            // to the body element.
> +            // However, the RenderNamedFlowThread is attached to the RenderView directly,
> +            // meaning that we are going to bypass the <body>'s RenderObject while iterating the parents.
> +            // As such, if the RenderNamedFlowThread is reached while iterating the parents, end this loop.
> +            // See http://dev.w3.org/csswg/css-regions/#cssomview-offset-attributes

I like the idea of this comment, but it would be much more useful if we could write a smaller one. There is so much text here that it’s hard to read. Is there a more concise way of saying the same thing? Could you follow this up with a patch to improve this comment by making it much shorter? I suggest this comment:

    // CSS region specification says that region flows should return the body element as their offsetParent.
    // Since we will bypass the body’s renderer anyway, just end the loop if we encounter a region flow (named flow thread).

> Source/WebCore/rendering/RenderObject.cpp:3015
> +    // If this object is inside a named flow thread, the nearest ancestor search skips
> +    // from the topmost named flow object directly to the body element.

The comment would be better if it was worded slightly differently. I would just say this:

    // CSS region specification says that region flows should return the body element as their offsetParent.

> Source/WebCore/rendering/RenderObject.cpp:3016
> +    // http://dev.w3.org/csswg/css-regions/#cssomview-offset-attributes

I don’t think the link to the spec is all that useful.

> Source/WebCore/rendering/RenderObject.cpp:3018
> +        // The search reached the named flow thread, skip to the body element.

This comment just repeats what the code does and should be removed.
Comment 21 Darin Adler 2013-05-14 09:54:12 PDT
Not sure my comments are worded exactly right, but they should point in the direction of the kind of comment we want to right.
Comment 22 Radu Stavila 2013-05-15 01:21:40 PDT
Created attachment 201802 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2013-05-15 03:53:52 PDT
Comment on attachment 201802 [details]
Patch for landing

Clearing flags on attachment: 201802

Committed r150108: <http://trac.webkit.org/changeset/150108>
Comment 24 Andrei Bucur 2013-05-20 04:37:07 PDT
The patch landed in r150108. Closing the bug.