Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2890583002: Fix incorrect use of coords conversion for sticky elements (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 1 week ago by yigu
Modified:
3 months, 3 weeks ago
Reviewers:
flackr, chrishtr, trchen
CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix incorrect use of coords conversion for sticky elements Normally, a compositing_container of a sticky element is a descendant of its ancestor_overflow_layer. We therefore convert the coordinates and apply the offset to the sticky element. However, the assumption above may not be true and accordingly applying the computed offset makes no sense. BUG=718188 TEST=compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Patch Set 1 #

Total comments: 12

Patch Set 2 : Address comments #

Patch Set 3 : Add comments #

Total comments: 5

Patch Set 4 : Bug fix #

Total comments: 2

Patch Set 5 : Apply source_offset #

Patch Set 6 : nit #

Messages

Total messages: 21 (7 generated)
yigu
PTAL. Thanks!
4 months, 1 week ago (2017-05-17 14:46:00 UTC) #3
flackr
https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html (right): https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html#newcode25 third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html:25: <div class="composited container"> No need to separately composite anything ...
4 months, 1 week ago (2017-05-17 19:44:03 UTC) #4
yigu
Thanks Rob. PTAL. https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html (right): https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html#newcode25 third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html:25: <div class="composited container"> On 2017/05/17 19:44:03, ...
4 months, 1 week ago (2017-05-17 20:28:15 UTC) #5
flackr
https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2890583002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode318 third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:318: bool is_descendant = false; On 2017/05/17 20:28:15, yigu wrote: ...
4 months, 1 week ago (2017-05-18 15:54:08 UTC) #6
yigu
Thanks Rob for the investigation! PTAL.
4 months, 1 week ago (2017-05-18 21:16:04 UTC) #7
chrishtr
+trchen for review while I am out.
4 months, 1 week ago (2017-05-18 22:03:38 UTC) #11
trchen
On 2017/05/18 22:03:38, chrishtr wrote: > +trchen for review while I am out. Okie dokie. ...
4 months, 1 week ago (2017-05-19 00:41:21 UTC) #12
yigu
Thanks! I've updated the patch. PTAL.
4 months, 1 week ago (2017-05-19 15:33:11 UTC) #13
flackr
https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html (right): https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html#newcode5 third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container-expected.html:5: } Remove unused class. https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html (right): https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html#newcode4 ...
4 months, 1 week ago (2017-05-19 18:28:31 UTC) #14
yigu
Thanks Rob. I've updated the patch which doesn't break Stephen's layout test (https://output.jsbin.com/pukoso). Let's see ...
4 months, 1 week ago (2017-05-19 19:50:45 UTC) #16
flackr
This looks good, I think you missed some comments in patch set 3 though?
4 months ago (2017-05-23 18:54:43 UTC) #17
yigu
Thanks Rob. I've removed the unused class. But the compositing container should be fix positioned ...
4 months ago (2017-05-23 19:16:40 UTC) #18
flackr
https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html File third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html (right): https://codereview.chromium.org/2890583002/diff/40001/third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html#newcode4 third_party/WebKit/LayoutTests/compositing/overflow/ancestor-overflow-layer-of-sticky-child-of-compositing-container.html:4: position: fixed; On 2017/05/23 19:16:40, yigu wrote: > On ...
4 months ago (2017-05-23 19:19:59 UTC) #19
flackr
3 months, 3 weeks ago (2017-05-31 13:51:31 UTC) #20
Are we abandoning this in favor of https://codereview.chromium.org/2911463002/ ?
If so, can you close this patch? Thanks.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b