|
|
Created:
3 years, 7 months ago by rhogan Modified:
3 years, 7 months ago Reviewers:
Xianzhu CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure visual overflow from style recalc gets propagated
When recalculating overflow due to style change, the overflow we calculate may
be clipped. We still need to treat it as having changed so that we can update
the ContentsVisualOverflowRect() of our container. This will allow the paint layer
to clip the overflow when painting it.
BUG=724453
Review-Url: https://codereview.chromium.org/2896363003
Cr-Commit-Position: refs/heads/master@{#474278}
Committed: https://chromium.googlesource.com/chromium/src/+/eac1506ac1502a44f10c1af30d0c4de6d7490a6b
Patch Set 1 #Patch Set 2 : bug 724453 #
Total comments: 9
Patch Set 3 : bug 724453 #
Messages
Total messages: 23 (17 generated)
The CQ bit was checked by robhogan@gmail.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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Ensure visual overflow from style recalc gets propagated BUG=724453 ========== to ========== Ensure visual overflow from style recalc gets propagated When recalculating overflow due to style change, the overflow we calculate may be clipped. We still need to treat it as having changed so that we can update the ContentsVisualOverflowRect() of our container. This will allow the paint layer to clip the overflow when painting it. BUG=724453 ==========
The CQ bit was checked by robhogan@gmail.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...
robhogan@gmail.com changed reviewers: + wangxianzhu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/inline-block-overflow-repaint.html (right): https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/inline-block-overflow-repaint.html:1: <!DOCTYPE html> Please put this file under paint/invalidation directory. https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/inline-block-overflow-repaint.html:3: p {overflow:hidden;} I suggest removing the above line, because whether the container has overflow clip doesn't affect the bug (though it hides the bug visually in some cases). Then having one test div seems enough. https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/inline-block-overflow-repaint.html:12: document.body.offsetTop; Please use <script src="../../resources/run-after-layout-and-paint.js"></script> <script> runAfterLayoutAndPaint(function() { test1.style.outline = ...; ... }, true); </script> This ensures a paint before the test, so the under-invalidation would reproduce. https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2069: return true; To keep the optimization for child overflow change not affecting overflow of ancestors, I think this should be changed to: return !HasOverflowClip() || self_needs_overflow_recalc; (with bool self_needed_overflow_recalc = SelfNeedsOverflowRecalcStyleChange() inserted before line 2052).
The CQ bit was checked by robhogan@gmail.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...
https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/inline-block-overflow-repaint.html (right): https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/inline-block-overflow-repaint.html:1: <!DOCTYPE html> On 2017/05/23 at 20:35:42, Xianzhu wrote: > Please put this file under paint/invalidation directory. Done. https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/inline-block-overflow-repaint.html:3: p {overflow:hidden;} On 2017/05/23 at 20:35:42, Xianzhu wrote: > I suggest removing the above line, because whether the container has overflow clip doesn't affect the bug (though it hides the bug visually in some cases). Then having one test div seems enough. Mm, not with you here - we do need the container to want to clip the outline don't we? https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/inline-block-overflow-repaint.html:12: document.body.offsetTop; On 2017/05/23 at 20:35:42, Xianzhu wrote: > Please use > <script src="../../resources/run-after-layout-and-paint.js"></script> > <script> > runAfterLayoutAndPaint(function() { > test1.style.outline = ...; > ... > }, true); > </script> > > This ensures a paint before the test, so the under-invalidation would reproduce. Done. https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2069: return true; On 2017/05/23 at 20:35:42, Xianzhu wrote: > To keep the optimization for child overflow change not affecting overflow of ancestors, I think this should be changed to: > return !HasOverflowClip() || self_needs_overflow_recalc; > (with > bool self_needed_overflow_recalc = SelfNeedsOverflowRecalcStyleChange() > inserted before line 2052). OK, done!
lgtm. https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/inline-block-overflow-repaint.html (right): https://codereview.chromium.org/2896363003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/inline-block-overflow-repaint.html:3: p {overflow:hidden;} On 2017/05/23 21:32:16, rhogan wrote: > On 2017/05/23 at 20:35:42, Xianzhu wrote: > > I suggest removing the above line, because whether the container has overflow > clip doesn't affect the bug (though it hides the bug visually in some cases). > Then having one test div seems enough. > > Mm, not with you here - we do need the container to want to clip the outline > don't we? I found I misunderstood the bug. I thought it were about incorrect visual overflow rect, but actually it was about incorrect clip.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495632301238120, "parent_rev": "dab01f63227a6d23fa61445444f0ae129c874686", "commit_rev": "eac1506ac1502a44f10c1af30d0c4de6d7490a6b"}
Message was sent while issue was closed.
Description was changed from ========== Ensure visual overflow from style recalc gets propagated When recalculating overflow due to style change, the overflow we calculate may be clipped. We still need to treat it as having changed so that we can update the ContentsVisualOverflowRect() of our container. This will allow the paint layer to clip the overflow when painting it. BUG=724453 ========== to ========== Ensure visual overflow from style recalc gets propagated When recalculating overflow due to style change, the overflow we calculate may be clipped. We still need to treat it as having changed so that we can update the ContentsVisualOverflowRect() of our container. This will allow the paint layer to clip the overflow when painting it. BUG=724453 Review-Url: https://codereview.chromium.org/2896363003 Cr-Commit-Position: refs/heads/master@{#474278} Committed: https://chromium.googlesource.com/chromium/src/+/eac1506ac1502a44f10c1af30d0c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/eac1506ac1502a44f10c1af30d0c... |