Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519163003:
LayoutView's paint layer should clip to the full viewport size. (Closed)
|
Created:
4 years, 1 month ago by bokan Modified:
4 years ago Reviewers:
skobes CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, dshwang, jchaffraix+rendering, blink-reviews-paint_chromium.org, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLayoutView's paint layer should clip to the full viewport size.
This patch makes it so that the LayoutView's painting size is always at least
the size of the layout viewport. Today (without root-layer-scrolls), the
LayoutView's PaintLayer (and its parent root content layer) is sized to contain
all the document's content and is clipped and scrolled by the PaintLayers
belonging to the Frame's PaintLayerCompositor (the Frame Scroll and Clipping
layers). This is problematic if the content is smaller than the viewport since
position: fixed elements are attached to the viewport but are not counted as
part of the LayoutView's "layout overflow".
This occurs in two situations (both occur only on Android):
1. Bug 666806 - With inert top controls on and the top controls hidden, a page
whose document is empty (for e.g.) will produce a LayoutView layer whose height
is the height of the viewport minus the height of the top controls. This means
bottom position: fixed elements will get clipped.
2. Bug 436871 - If the content on a page is wider than the ICB, Chrome will
grow the layout viewport until its width covers all the content (or the minimum
scale is reached), keeping the aspect-ratio fixed. When this happens, the
layout viewport can become taller than the content resulting in clipping of
bottom position: fixed elements. (This is related to crbug.com/492871).
This patch fixes both issues.
BUG=666806, 436871
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/a226f0e367ee7e064cc398d0b76d347d53103295
Cr-Commit-Position: refs/heads/master@{#434303}
Patch Set 1 #Patch Set 2 : Fix test #Patch Set 3 : Fix test #
Total comments: 2
Patch Set 4 : Add test #
Dependent Patchsets: Messages
Total messages: 37 (24 generated)
Description was changed from ========== LayoutView's paint layer should clip to the full viewport size. BUG=666806 ========== to ========== LayoutView's paint layer should clip to the full viewport size. BUG=666806 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by bokan@chromium.org 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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bokan@chromium.org 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bokan@chromium.org 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...
Description was changed from ========== LayoutView's paint layer should clip to the full viewport size. BUG=666806 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== LayoutView's paint layer should clip to the full viewport size. BUG=666806,436871 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== LayoutView's paint layer should clip to the full viewport size. BUG=666806,436871 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== LayoutView's paint layer should clip to the full viewport size. This patch makes it so that the LayoutView's painting size is always at least the size of the layout viewport. Today (without root-layer-scrolls), the LayoutView's PaintLayer (and its parent root content layer) is sized to contain all the document's content and is clipped and scrolled by the PaintLayers belonging to the Frame's PaintLayerCompositor (the Frame Scroll and Clipping layers). This is problematic if the content is smaller than the viewport since position: fixed elements are attached to the viewport but are not counted as part of the LayoutView's "layout overflow". This occurs in two situations (both occur only on Android): 1. Bug 666806 - With inert top controls on and the top controls hidden, a page whose document is empty (for e.g.) will produce a LayoutView layer whose height is the height of the viewport minus the height of the top controls. This means bottom position: fixed elements will get clipped. 2. Bug 436871 - If the content on a page is wider than the ICB, Chrome will grow the layout viewport until its width covers all the content (or the minimum scale is reached), keeping the aspect-ratio fixed. When this happens, the layout viewport can become taller than the content resulting in clipping of bottom position: fixed elements. (This is related to crbug.com/492871). This patch fixes both issues. BUG=666806,436871 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
bokan@chromium.org changed reviewers: + skobes@chromium.org
Skobes, wdyt? This passes all the tests and feels ok to me but I don't really understand paint/layout well enough to judge. Are you the best person to review this? Happy to discuss alternate approaches.
Oh, and I haven't added a test yet. I'll wait for validation of the general approach to add one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
This seems reasonable. It's only affecting the non-RLS case, right? https://codereview.chromium.org/2519163003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2519163003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2551: if (isRootLayer()) { You can keep this line as it was and remove the !needsCompositedScrolling() check below.
On 2016/11/23 00:13:24, skobes wrote: > This seems reasonable. It's only affecting the non-RLS case, right? No, this will affect RLS as well. Since we've shipped inert-top-controls, hiding the url bar won't cause a relayout which means the LayoutView remains the same size. We'll need to resize the LayoutView's scrolling layer in this case too. That said, the RLS case needs some work here and I want to keep this patch small since this needs to be merged back to M56. > > https://codereview.chromium.org/2519163003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): > > https://codereview.chromium.org/2519163003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayer.cpp:2551: if (isRootLayer()) { > You can keep this line as it was and remove the !needsCompositedScrolling() > check below.
Test added, ptal. https://codereview.chromium.org/2519163003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2519163003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2551: if (isRootLayer()) { On 2016/11/23 00:13:24, skobes wrote: > You can keep this line as it was and remove the !needsCompositedScrolling() > check below. Done.
lgtm
The CQ bit was checked by bokan@chromium.org
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
Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by bokan@chromium.org
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by bokan@chromium.org
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": 60001, "attempt_start_ts": 1479959270793970,
"parent_rev": "ede44504b8c6e7a90522f66aec48bd2c433db76f", "commit_rev":
"b336be3fa5bb1e8c1f4f9a35d96e67f2b464ac09"}
Message was sent while issue was closed.
Description was changed from ========== LayoutView's paint layer should clip to the full viewport size. This patch makes it so that the LayoutView's painting size is always at least the size of the layout viewport. Today (without root-layer-scrolls), the LayoutView's PaintLayer (and its parent root content layer) is sized to contain all the document's content and is clipped and scrolled by the PaintLayers belonging to the Frame's PaintLayerCompositor (the Frame Scroll and Clipping layers). This is problematic if the content is smaller than the viewport since position: fixed elements are attached to the viewport but are not counted as part of the LayoutView's "layout overflow". This occurs in two situations (both occur only on Android): 1. Bug 666806 - With inert top controls on and the top controls hidden, a page whose document is empty (for e.g.) will produce a LayoutView layer whose height is the height of the viewport minus the height of the top controls. This means bottom position: fixed elements will get clipped. 2. Bug 436871 - If the content on a page is wider than the ICB, Chrome will grow the layout viewport until its width covers all the content (or the minimum scale is reached), keeping the aspect-ratio fixed. When this happens, the layout viewport can become taller than the content resulting in clipping of bottom position: fixed elements. (This is related to crbug.com/492871). This patch fixes both issues. BUG=666806,436871 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== LayoutView's paint layer should clip to the full viewport size. This patch makes it so that the LayoutView's painting size is always at least the size of the layout viewport. Today (without root-layer-scrolls), the LayoutView's PaintLayer (and its parent root content layer) is sized to contain all the document's content and is clipped and scrolled by the PaintLayers belonging to the Frame's PaintLayerCompositor (the Frame Scroll and Clipping layers). This is problematic if the content is smaller than the viewport since position: fixed elements are attached to the viewport but are not counted as part of the LayoutView's "layout overflow". This occurs in two situations (both occur only on Android): 1. Bug 666806 - With inert top controls on and the top controls hidden, a page whose document is empty (for e.g.) will produce a LayoutView layer whose height is the height of the viewport minus the height of the top controls. This means bottom position: fixed elements will get clipped. 2. Bug 436871 - If the content on a page is wider than the ICB, Chrome will grow the layout viewport until its width covers all the content (or the minimum scale is reached), keeping the aspect-ratio fixed. When this happens, the layout viewport can become taller than the content resulting in clipping of bottom position: fixed elements. (This is related to crbug.com/492871). This patch fixes both issues. BUG=666806,436871 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== LayoutView's paint layer should clip to the full viewport size. This patch makes it so that the LayoutView's painting size is always at least the size of the layout viewport. Today (without root-layer-scrolls), the LayoutView's PaintLayer (and its parent root content layer) is sized to contain all the document's content and is clipped and scrolled by the PaintLayers belonging to the Frame's PaintLayerCompositor (the Frame Scroll and Clipping layers). This is problematic if the content is smaller than the viewport since position: fixed elements are attached to the viewport but are not counted as part of the LayoutView's "layout overflow". This occurs in two situations (both occur only on Android): 1. Bug 666806 - With inert top controls on and the top controls hidden, a page whose document is empty (for e.g.) will produce a LayoutView layer whose height is the height of the viewport minus the height of the top controls. This means bottom position: fixed elements will get clipped. 2. Bug 436871 - If the content on a page is wider than the ICB, Chrome will grow the layout viewport until its width covers all the content (or the minimum scale is reached), keeping the aspect-ratio fixed. When this happens, the layout viewport can become taller than the content resulting in clipping of bottom position: fixed elements. (This is related to crbug.com/492871). This patch fixes both issues. BUG=666806,436871 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== LayoutView's paint layer should clip to the full viewport size. This patch makes it so that the LayoutView's painting size is always at least the size of the layout viewport. Today (without root-layer-scrolls), the LayoutView's PaintLayer (and its parent root content layer) is sized to contain all the document's content and is clipped and scrolled by the PaintLayers belonging to the Frame's PaintLayerCompositor (the Frame Scroll and Clipping layers). This is problematic if the content is smaller than the viewport since position: fixed elements are attached to the viewport but are not counted as part of the LayoutView's "layout overflow". This occurs in two situations (both occur only on Android): 1. Bug 666806 - With inert top controls on and the top controls hidden, a page whose document is empty (for e.g.) will produce a LayoutView layer whose height is the height of the viewport minus the height of the top controls. This means bottom position: fixed elements will get clipped. 2. Bug 436871 - If the content on a page is wider than the ICB, Chrome will grow the layout viewport until its width covers all the content (or the minimum scale is reached), keeping the aspect-ratio fixed. When this happens, the layout viewport can become taller than the content resulting in clipping of bottom position: fixed elements. (This is related to crbug.com/492871). This patch fixes both issues. BUG=666806,436871 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/a226f0e367ee7e064cc398d0b76d347d53103295 Cr-Commit-Position: refs/heads/master@{#434303} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a226f0e367ee7e064cc398d0b76d347d53103295 Cr-Commit-Position: refs/heads/master@{#434303} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
