Bug 113276

Summary:

[CSSRegions] Implement offsetParent for elements inside named flow

Product:

WebKit

Reporter:

Mihnea Ovidenie <mihnea>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abucur, achicu, buildbot, commit-queue, darin, esprehn+autocc, glenn, rniwa, WebkitBugTracker

Priority:

Keywords:

AdobeTracked

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

57312

Attachments:

Description	Flags
Patch	none
Patch	achicu: review-, achicu: commit-queue-
Patch	achicu: review-, achicu: commit-queue-
Patch	buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2	none
Patch	none
Patch	achicu: review-, achicu: commit-queue-
Patch	darin: review+, darin: commit-queue-
Patch for landing	none

Mihnea Ovidenie

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

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Mihnea Ovidenie

Comment 1 2013-04-26 05:44:56 PDT

http://dev.w3.org/csswg/css-regions/#cssomview-offset-attributes clarifies what should happen with the offsetParent

Radu Stavila

Comment 2 2013-04-30 07:46:44 PDT

Created attachment 200116 [details] Patch

Mihai Maerean

Comment 3 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 [].

Radu Stavila

Comment 4 2013-04-30 08:00:01 PDT

Created attachment 200117 [details] Patch

Mihai Maerean

Comment 5 2013-04-30 08:09:00 PDT

Radu Stavila

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

Mihnea Ovidenie

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

Radu Stavila

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

Alexandru Chiculita

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

Radu Stavila

Comment 10 2013-05-07 05:20:11 PDT

Created attachment 200893 [details] Patch

Alexandru Chiculita

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

Radu Stavila

Comment 12 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?

Radu Stavila

Comment 13 2013-05-07 09:32:48 PDT

Created attachment 200919 [details] Patch

Build Bot

Comment 14 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

Build Bot

Comment 15 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

Radu Stavila

Comment 16 2013-05-08 01:32:46 PDT

Created attachment 201042 [details] Patch

Radu Stavila

Comment 17 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

Alexandru Chiculita

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

Radu Stavila

Comment 19 2013-05-14 04:16:39 PDT

Created attachment 201697 [details] Patch

Darin Adler

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

Darin Adler

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

Radu Stavila

Comment 22 2013-05-15 01:21:40 PDT

Created attachment 201802 [details] Patch for landing

WebKit Commit Bot

Comment 23 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>

Andrei Bucur

Comment 24 2013-05-20 04:37:07 PDT

The patch landed in r150108. Closing the bug.

Note You need to log in before you can comment on or make changes to this bug.