Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2454693003:
adjustedPositionRelativeTo() couldn't find offsetParent. (Closed)
|
Created:
4 years, 1 month ago by Karl Øygard Modified:
4 years, 1 month ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionadjustedPositionRelativeTo() couldn't find offsetParent.
LayoutBoxModelObject::adjustedPositionRelativeTo() could get confused
by inline continuations, and could fail if offsetParent itself was a split
continuation. If the child belongs to the second part of the continuation,
we'll instead race to the root of the tree. By comparing with the node instead,
we correctly identify the offsetParent and stop the search.
BUG=638187
Committed: https://crrev.com/c3485960aedb34468b06c02f7d132501a26c0d2d
Cr-Commit-Position: refs/heads/master@{#429541}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update comment, since the problem described has been fixed. #
Messages
Total messages: 27 (16 generated)
The CQ bit was checked by karlo@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== adjustedPositionRelativeTo() couldn't find offsetParent. LayoutBoxModelObject::adjustedPositionRelativeTo() could get confused by inline continuations, and could fail to find offsetParent, instead racing to the root of the tree. By comparing with the node instead, we correctly identify the offsetParent and stop the search. BUG=638187 ========== to ========== adjustedPositionRelativeTo() couldn't find offsetParent. LayoutBoxModelObject::adjustedPositionRelativeTo() could get confused by inline continuations, and could fail if offsetParent itself was a split continuation. If the child belongs to the second part of the continuation, we'll instead race to the root of the tree. By comparing with the node instead, we correctly identify the offsetParent and stop the search. BUG=638187 ==========
karlo@opera.com changed reviewers: + glebl@chromium.org
ptal
https://codereview.chromium.org/2454693003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2454693003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1019: current && current->node() != element; I'm not very familiar with this code. I would suggest mstensho@ as a reviewer. According to git blame he authored this code. From my point of view I have 2 comments: 1) should we change the comment at line 1012 as well? 2) if we don't depend on offsetParent anymore then perhaps we can move this part of code out of the "offsetParent = element->layoutBoxModelObject()" IF statement?
https://codereview.chromium.org/2454693003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2454693003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1019: current && current->node() != element; On 2016/10/31 22:29:53, Gleb Lanbin wrote: > I'm not very familiar with this code. I would suggest mstensho@ as a reviewer. > According to git blame he authored this code. > > From my point of view I have 2 comments: > > 1) should we change the comment at line 1012 as well? > 2) if we don't depend on offsetParent anymore then perhaps we can move this part > of code out of the "offsetParent = element->layoutBoxModelObject()" IF > statement? Agreed, I've updated the comment with another problem that I know of that isn't addressed by this fix. I can't see an easy way of moving the code out without some code duplication, so I've let it remain as is.
The CQ bit was checked by karlo@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by karlo@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
mstensho@opera.com changed reviewers: + mstensho@opera.com
rslgtm
On 2016/11/03 09:35:47, mstensho wrote: > rslgtm ltghaaah! lgtm!
The CQ bit was checked by karlo@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== adjustedPositionRelativeTo() couldn't find offsetParent. LayoutBoxModelObject::adjustedPositionRelativeTo() could get confused by inline continuations, and could fail if offsetParent itself was a split continuation. If the child belongs to the second part of the continuation, we'll instead race to the root of the tree. By comparing with the node instead, we correctly identify the offsetParent and stop the search. BUG=638187 ========== to ========== adjustedPositionRelativeTo() couldn't find offsetParent. LayoutBoxModelObject::adjustedPositionRelativeTo() could get confused by inline continuations, and could fail if offsetParent itself was a split continuation. If the child belongs to the second part of the continuation, we'll instead race to the root of the tree. By comparing with the node instead, we correctly identify the offsetParent and stop the search. BUG=638187 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== adjustedPositionRelativeTo() couldn't find offsetParent. LayoutBoxModelObject::adjustedPositionRelativeTo() could get confused by inline continuations, and could fail if offsetParent itself was a split continuation. If the child belongs to the second part of the continuation, we'll instead race to the root of the tree. By comparing with the node instead, we correctly identify the offsetParent and stop the search. BUG=638187 ========== to ========== adjustedPositionRelativeTo() couldn't find offsetParent. LayoutBoxModelObject::adjustedPositionRelativeTo() could get confused by inline continuations, and could fail if offsetParent itself was a split continuation. If the child belongs to the second part of the continuation, we'll instead race to the root of the tree. By comparing with the node instead, we correctly identify the offsetParent and stop the search. BUG=638187 Committed: https://crrev.com/c3485960aedb34468b06c02f7d132501a26c0d2d Cr-Commit-Position: refs/heads/master@{#429541} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c3485960aedb34468b06c02f7d132501a26c0d2d Cr-Commit-Position: refs/heads/master@{#429541} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
