Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817583006:
Add a use-counter for viewport-fixed position:fixed under a filter (Closed)
|
Created:
3 years, 8 months ago by chrishtr Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-frames_chromium.org, 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. |
DescriptionAdd a use-counter for viewport-fixed position:fixed under a filter
This will inform whether we can make the change suggested in
https://github.com/w3c/fxtf-drafts/issues/11
BUG=711736
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2817583006
Cr-Commit-Position: refs/heads/master@{#464853}
Committed: https://chromium.googlesource.com/chromium/src/+/e208bf05c117cf2fa0f7aab01a779d47fd92b8ef
Patch Set 1 #Patch Set 2 : none #Patch Set 3 : Merge branch 'master' into usecounterfilter #
Messages
Total messages: 29 (16 generated)
Description was changed from ========== none none none none BUG= ========== to ========== none none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== none none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Add a use-counter for viewport-fixed position:fixed under a filter This will inform whether we can make the change suggested in https://github.com/w3c/fxtf-drafts/issues/11 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by chrishtr@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...
chrishtr@chromium.org changed reviewers: + mpearson@chromium.org, trchen@chromium.org
histograms.xml lgtm
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_...)
ping
The implementation itself lgtm.
I have questions about the counting criteria though.
1. Do you want to count abs-pos descendants not positioned by the filter too?
2. How about filter and fixed-pos descendant both contained by a transformed
element? e.g.
<div style="transform:translateZ(0);">
<div style="overflow:scroll">
<div style="filter:blur(1px)">
<div style="position:fixed"></div>
</div>
</div>
</div>
I think it is possible to implement precise tracking in PropertyTreeBuilder.
This is a good question. I think we should add such use counters, as they are additional risks for making the change to force filters to contain their children. The particular use-case addressed in this CL is about how often it's the case that a filter is applied to a fixed-pos element that is fixed to the viewport of the frame. That use-case is especially important to me, because it is the only case in which the filter-as-containing-block change would result in web content that has no workaround. This is because there is no way to add a filter that is above the implicit "LayoutView" containing block of regular fixed-pos element. I'm also looking into the other parts now. It appears I'll first need a patch to address the TODO on line 1046 of PaintPropertyTreeBuilder.cpp - we don't pass along the containing block as part of the recursive context.
Description was changed from ========== Add a use-counter for viewport-fixed position:fixed under a filter This will inform whether we can make the change suggested in https://github.com/w3c/fxtf-drafts/issues/11 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Add a use-counter for viewport-fixed position:fixed under a filter This will inform whether we can make the change suggested in https://github.com/w3c/fxtf-drafts/issues/11 BUG=711736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Do you think this CL is reasonable to commit, and follow up with the other parts you mentioned?
On 2017/04/14 18:32:13, chrishtr wrote: > This is a good question. I think we should add such use counters, as they are > additional risks for making the change to force filters to contain their > children. > The particular use-case addressed in this CL is about how often it's the case > that > a filter is applied to a fixed-pos element that is fixed to the viewport of the > frame. > > That use-case is especially important to me, because it is the only > case in which the filter-as-containing-block change would result in web content > that has no workaround. This is because there is no way to add a filter that is > above the implicit "LayoutView" containing block of regular fixed-pos element. I see. It is possible to have a filter that applies to either "elements scroll along document" or "elements fixed to viewport", but not both of them. In this case, do you want to exclude filters that are fixed to viewport themselves? > I'm also looking into the other parts now. It appears I'll first need a patch > to address the TODO on line 1046 of PaintPropertyTreeBuilder.cpp - we don't > pass along the containing block as part of the recursive context. I think the two things are orthogonal. For the purpose of this CL, we can just add a bit in the position-specific context, indicating that "this context would be different if we changed filter definition", then use-count if we encounter any element that uses marked context. > Do you think this CL is reasonable to commit, and follow up with the other parts > you > mentioned? Sure! Didn't mean to block you.
The CQ bit was checked by chrishtr@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: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, trchen@chromium.org Link to the patchset: https://codereview.chromium.org/2817583006/#ps40001 (title: "Merge branch 'master' into usecounterfilter")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chrishtr@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": 40001, "attempt_start_ts": 1492217192915580,
"parent_rev": "097db1d2d5de77aec5c32c710bd90b7a481deb09", "commit_rev":
"e208bf05c117cf2fa0f7aab01a779d47fd92b8ef"}
Message was sent while issue was closed.
Description was changed from ========== Add a use-counter for viewport-fixed position:fixed under a filter This will inform whether we can make the change suggested in https://github.com/w3c/fxtf-drafts/issues/11 BUG=711736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Add a use-counter for viewport-fixed position:fixed under a filter This will inform whether we can make the change suggested in https://github.com/w3c/fxtf-drafts/issues/11 BUG=711736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2817583006 Cr-Commit-Position: refs/heads/master@{#464853} Committed: https://chromium.googlesource.com/chromium/src/+/e208bf05c117cf2fa0f7aab01a77... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e208bf05c117cf2fa0f7aab01a77... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
