| 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: | P2 | Keywords: | AdobeTracked | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||
| Bug Blocks: | 57312 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Mihnea Ovidenie
2013-03-25 22:43:09 PDT
http://dev.w3.org/csswg/css-regions/#cssomview-offset-attributes clarifies what should happen with the offsetParent Created attachment 200116 [details]
Patch
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 []. Created attachment 200117 [details]
Patch
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 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 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 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. 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. Created attachment 200893 [details]
Patch
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 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? Created attachment 200919 [details]
Patch
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 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
Created attachment 201042 [details]
Patch
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 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. Created attachment 201697 [details]
Patch
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. Not sure my comments are worded exactly right, but they should point in the direction of the kind of comment we want to right. Created attachment 201802 [details]
Patch for landing
Comment on attachment 201802 [details] Patch for landing Clearing flags on attachment: 201802 Committed r150108: <http://trac.webkit.org/changeset/150108> The patch landed in r150108. Closing the bug. |