Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916563003:
Compute effective touch action in StyleAdjuster. (Closed)
|
Created:
3 years, 6 months ago by sunxd Modified:
3 years, 5 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, dtapuska+blinkwatch_chromium.org, hayleyferr, Navid Zolghadr, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCompute effective touch action in StyleAdjuster.
We used to store touch action bits in ComputedStyle and recompute the
effective touch action in TouchActionUtil whenever we need to use it.
In order to process touch action info in cc, we need to adjust this
method as cc has no access to TouchActionUtil. We can compute them when
recomputing styles or in cc. As cc lacks hit testing information, it is
more feasible to do it in StyleAdjuster. Pre-computing the style also
saves time by avoiding tree walks.
This CL adds the EffectiveTouchAction to non-css RareInheritedData and
uses it when computing styles. There are also special cases where touch
action is not supported in atomic inline level elements and there are
exceptions if an element is an iframe or scrolls.
BUG=727853
Review-Url: https://codereview.chromium.org/2916563003
Cr-Commit-Position: refs/heads/master@{#486363}
Committed: https://chromium.googlesource.com/chromium/src/+/4d9c0082acd8bd604a1a44260dc6f62a400e5ddb
Patch Set 1 #Patch Set 2 : Fix clang compiling error. #Patch Set 3 : Rebase!! A conflict patch was landed, reverted and landed again. #Patch Set 4 : fix layout test failures #
Total comments: 37
Patch Set 5 : Move ComputeEffectiveTouchAction out of AdjustComputedStyle. #
Total comments: 19
Patch Set 6 : Unifying ScrollsOverflow #
Total comments: 4
Patch Set 7 : Iframe touch action updates trigger descendant style recalc. #
Total comments: 11
Patch Set 8 : early out in style adjuster #
Total comments: 2
Patch Set 9 : resolve comments #
Total comments: 4
Patch Set 10 : resolve comments #Patch Set 11 : Add rendering test that tests iframe style change #
Total comments: 4
Patch Set 12 : Add StyleAdjusterTest #
Total comments: 6
Patch Set 13 : resolve comments #Patch Set 14 : add test case #
Total comments: 8
Patch Set 15 : resolve comments #
Total comments: 2
Patch Set 16 : nit change #Patch Set 17 : Do not traverse across frames #
Total comments: 8
Patch Set 18 : Skip elements that don't need a layout object #
Total comments: 3
Patch Set 19 : Rebase #Messages
Total messages: 115 (69 generated)
The CQ bit was checked by sunxd@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunxd@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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-...) 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 sunxd@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 ========== Compute effective touch action in StyleAdjuster. We used to store touch action bits in ComputedStyle and recompute the effective touch action in TouchActionUtil whenever we need to use it. In order to process touch action info in cc, we need to adjust this method as cc has no access to TouchActionUtil. We can compute them when recomputing styles or in cc. As cc lacks hit testing information, it is more feasible to do it in StyleAdjuster. Pre-computing the style also saves time by avoiding tree walks. This CL adds the EffectiveTouchAction to non-css RareInheritedData and uses it when computing styles. There are also special cases where touch action is not supported in atomic inline level elements and there are exceptions if an element is an iframe or scrolls. BUG=727853 ========== to ========== Compute effective touch action in StyleAdjuster. We used to store touch action bits in ComputedStyle and recompute the effective touch action in TouchActionUtil whenever we need to use it. In order to process touch action info in cc, we need to adjust this method as cc has no access to TouchActionUtil. We can compute them when recomputing styles or in cc. As cc lacks hit testing information, it is more feasible to do it in StyleAdjuster. Pre-computing the style also saves time by avoiding tree walks. This CL adds the EffectiveTouchAction to non-css RareInheritedData and uses it when computing styles. There are also special cases where touch action is not supported in atomic inline level elements and there are exceptions if an element is an iframe or scrolls. BUG=727853 ==========
sunxd@chromium.org changed reviewers: + flackr@chromium.org, majidvp@chromium.org, wkorman@chromium.org, xidachen@chromium.org
PTAL. Per our discussion offline, I'm not sure if the new logic is totally compatible with the old one, especially with iframes. I will include more people once our local folks are fine with the change.
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 sunxd@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I am new to StyleAdjuster work, so please take my feedback with that grain of salt! I am excited to see this, thanks for working on it. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:456: bool is_svg_root = false; Please add unit tests for this new logic. I haven't modified StyleAdjuster myself and don't see StyleAdjusterTest but there should be somewhere or if not we should start one. We should be able to test this with a RenderingTest (see Paint tests of which there are many that use these) but it may be that Style focused code has their own preferred way of unit testing so I would look for that under core/style first. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:511: bool is_table_row_or_column = style.Display() == EDisplay::kTableRow || Can we use IsDisplayTableType() or is that too broad? https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:515: if (is_non_replaced_inline_elements || is_table_row_or_column) { What made us choose this filtering criteria? If there is a relevant section of W3C spec worth referencing in a comment here. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:528: action |= TouchAction::kTouchActionPan; Add a comment re: why we or-in kTouchActionPan here and below. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:530: if (element && element == element->GetDocument().documentElement() && Add a brief comment re: what we are doing at a high level below. I think you're adding in the touch action bits for frames as long as they are not OOPIFs but I am not 100% on that last bit. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:540: // Apply TouchAction bits from its parent style Clarify comment to note that we've incorporated other potential bits as well per above logic. Since it reads as if, if we make it here, we're only applying bits from the parent style otherwise unadulterated, but in actuality at this point |action| has been updated to at least potentially incorporate more. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:74: // This code is to prevent failure when node does not have a layout object, in Is this method planned to stay around? I had impression we thought it would go away once we had the effective touch action in the rare data as is done in the rest of this change. If we do need to keep it around this seems like it could be a separate change and would want its own unit test. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:75: // this case, we use the effective_touch_action of its nearest ancestor that |effective_touch_action| (to make it easier to see var refs vs prose in a comment) Or just change to prose ("effective touch action") and rm '_'
https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:456: bool is_svg_root = false; On 2017/06/02 03:07:05, wkorman wrote: > Please add unit tests for this new logic. I haven't modified StyleAdjuster > myself and don't see StyleAdjusterTest but there should be somewhere or if not > we should start one. We should be able to test this with a RenderingTest (see > Paint tests of which there are many that use these) but it may be that Style > focused code has their own preferred way of unit testing so I would look for > that under core/style first. nit: Calculate this here, i.e. bool is_svg_root = is_svg_element && !(parent is svg element); https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:517: return; Seems to me early returns are dangerous in a long generic function like this, people may add more adjusted style code afterwards and not have it run. I'd suggest moving this to a specific function like AdjustEffectiveTouchAction. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:522: style.OverflowX() == EOverflow::kOverlay; Seems like this could use a helper function on ComputedStyle. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:525: style.OverflowY() == EOverflow::kWebkitPagedY || Why only kWebkitPagedY? Should overflow_x also check kWebkitPagedX? https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:536: frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan; This won't re-enable panning since it is strictly removing bits from action. Is this what you wanted to do? If so, you should probably move the code from 528 below here and re-enable panning for scrollers or frames.
I'm working on some of the comments, but there are also questions raised by comments that probably needs some discussion. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:456: bool is_svg_root = false; On 2017/06/02 03:07:05, wkorman wrote: > Please add unit tests for this new logic. I haven't modified StyleAdjuster > myself and don't see StyleAdjusterTest but there should be somewhere or if not > we should start one. We should be able to test this with a RenderingTest (see > Paint tests of which there are many that use these) but it may be that Style > focused code has their own preferred way of unit testing so I would look for > that under core/style first. Sure I'll look into this. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:456: bool is_svg_root = false; On 2017/06/05 18:43:10, flackr wrote: > On 2017/06/02 03:07:05, wkorman wrote: > > Please add unit tests for this new logic. I haven't modified StyleAdjuster > > myself and don't see StyleAdjusterTest but there should be somewhere or if not > > we should start one. We should be able to test this with a RenderingTest (see > > Paint tests of which there are many that use these) but it may be that Style > > focused code has their own preferred way of unit testing so I would look for > > that under core/style first. > > nit: Calculate this here, i.e. > bool is_svg_root = is_svg_element && !(parent is svg element); Done % StyleAdjusterTest. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:456: bool is_svg_root = false; On 2017/06/02 03:07:05, wkorman wrote: > Please add unit tests for this new logic. I haven't modified StyleAdjuster > myself and don't see StyleAdjusterTest but there should be somewhere or if not > we should start one. We should be able to test this with a RenderingTest (see > Paint tests of which there are many that use these) but it may be that Style > focused code has their own preferred way of unit testing so I would look for > that under core/style first. Acknowledged. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:511: bool is_table_row_or_column = style.Display() == EDisplay::kTableRow || On 2017/06/02 03:07:05, wkorman wrote: > Can we use IsDisplayTableType() or is that too broad? Yes that function is too broad. I think table captions, for example, should support touch actions, and there are existing touch-action tests that fail if we use IsDisplayTableType(). https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:515: if (is_non_replaced_inline_elements || is_table_row_or_column) { On 2017/06/02 03:07:05, wkorman wrote: > What made us choose this filtering criteria? If there is a relevant section of > W3C spec worth referencing in a comment here. Sure I'll add one here. According to spec, the touch-action property only applies to elements that support both the CSS width and height properties. The criteria here means atomic inline level elements, I believe, and the tests are happy with it. But I'm really unsure if it includes all situations. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:515: if (is_non_replaced_inline_elements || is_table_row_or_column) { On 2017/06/02 03:07:05, wkorman wrote: > What made us choose this filtering criteria? If there is a relevant section of > W3C spec worth referencing in a comment here. Acknowledged. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:517: return; On 2017/06/05 18:43:10, flackr wrote: > Seems to me early returns are dangerous in a long generic function like this, > people may add more adjusted style code afterwards and not have it run. I'd > suggest moving this to a specific function like AdjustEffectiveTouchAction. Acknowledged. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:522: style.OverflowX() == EOverflow::kOverlay; On 2017/06/05 18:43:10, flackr wrote: > Seems like this could use a helper function on ComputedStyle. Acknowledged. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:525: style.OverflowY() == EOverflow::kWebkitPagedY || On 2017/06/05 18:43:10, flackr wrote: > Why only kWebkitPagedY? Should overflow_x also check kWebkitPagedX? I adopted the logic from here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... The change was introduced in https://chromium.googlesource.com/chromium/src/+/862ec6da145e3a91cd547e40b79f... It seems we already automatically apply a scrollbar for -webkit-paged-x, but it's not used to determine if a layout box scrolls? https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:528: action |= TouchAction::kTouchActionPan; On 2017/06/02 03:07:05, wkorman wrote: > Add a comment re: why we or-in kTouchActionPan here and below. Acknowledged. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:530: if (element && element == element->GetDocument().documentElement() && On 2017/06/02 03:07:05, wkorman wrote: > Add a brief comment re: what we are doing at a high level below. I think you're > adding in the touch action bits for frames as long as they are not OOPIFs but I > am not 100% on that last bit. Acknowledged. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:536: frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan; On 2017/06/05 18:43:10, flackr wrote: > This won't re-enable panning since it is strictly removing bits from action. Is > this what you wanted to do? If so, you should probably move the code from 528 > below here and re-enable panning for scrollers or frames. Yes. Scrollers / frames only mask its ancestors panning-related touch actions, but it has no influence on its descendants. The action here is from element which is a descendant of the iframe. Iframe's panning should only cancel panning-restricted actions from its ancestors, but should not have any effect on element's action. Another problem about iframe stuff is that I cannot find a way to know whether the element we are processing is an iframe, because an iframe's document is the one that contains the iframe. I can only know that the element is the first descendant of the iframe's document. So the above logic is applied when the element is a scroller, the logic here is applied when the "parent" (not actually) is a scroller. Also in the old code, iframe does not get the panning exception. The panning is added to layout view which does not have a corresponding element (I'm not sure, but it seems so from the code). https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:540: // Apply TouchAction bits from its parent style On 2017/06/02 03:07:05, wkorman wrote: > Clarify comment to note that we've incorporated other potential bits as well per > above logic. Since it reads as if, if we make it here, we're only applying bits > from the parent style otherwise unadulterated, but in actuality at this point > |action| has been updated to at least potentially incorporate more. Acknowledged. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:74: // This code is to prevent failure when node does not have a layout object, in On 2017/06/02 03:07:05, wkorman wrote: > Is this method planned to stay around? I had impression we thought it would go > away once we had the effective touch action in the rare data as is done in the > rest of this change. > > If we do need to keep it around this seems like it could be a separate change > and would want its own unit test. So I'm a little worried about iframe's touch action here and would like to have some guarding code to test the correctness in the wild, but maybe we can simply throw the new logic to the wild and revert the patch if anything breaks. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:75: // this case, we use the effective_touch_action of its nearest ancestor that On 2017/06/02 03:07:06, wkorman wrote: > |effective_touch_action| (to make it easier to see var refs vs prose in a > comment) > > Or just change to prose ("effective touch action") and rm '_' Acknowledged.
The CQ bit was checked by sunxd@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...
PTAL. I made some changes according to the comments. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:511: bool is_table_row_or_column = style.Display() == EDisplay::kTableRow || On 2017/06/05 20:12:03, sunxd wrote: > On 2017/06/02 03:07:05, wkorman wrote: > > Can we use IsDisplayTableType() or is that too broad? > > Yes that function is too broad. I think table captions, for example, should > support touch actions, and there are existing touch-action tests that fail if we > use IsDisplayTableType(). Done. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:515: if (is_non_replaced_inline_elements || is_table_row_or_column) { On 2017/06/02 03:07:05, wkorman wrote: > What made us choose this filtering criteria? If there is a relevant section of > W3C spec worth referencing in a comment here. Done. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:517: return; On 2017/06/05 20:12:04, sunxd wrote: > On 2017/06/05 18:43:10, flackr wrote: > > Seems to me early returns are dangerous in a long generic function like this, > > people may add more adjusted style code afterwards and not have it run. I'd > > suggest moving this to a specific function like AdjustEffectiveTouchAction. > > Acknowledged. Done. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:522: style.OverflowX() == EOverflow::kOverlay; On 2017/06/05 20:12:03, sunxd wrote: > On 2017/06/05 18:43:10, flackr wrote: > > Seems like this could use a helper function on ComputedStyle. > > Acknowledged. Done. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:528: action |= TouchAction::kTouchActionPan; On 2017/06/02 03:07:05, wkorman wrote: > Add a comment re: why we or-in kTouchActionPan here and below. Done. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:530: if (element && element == element->GetDocument().documentElement() && On 2017/06/02 03:07:05, wkorman wrote: > Add a brief comment re: what we are doing at a high level below. I think you're > adding in the touch action bits for frames as long as they are not OOPIFs but I > am not 100% on that last bit. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:74: // This code is to prevent failure when node does not have a layout object, in On 2017/06/05 20:12:04, sunxd wrote: > On 2017/06/02 03:07:05, wkorman wrote: > > Is this method planned to stay around? I had impression we thought it would go > > away once we had the effective touch action in the rare data as is done in the > > rest of this change. > > > > If we do need to keep it around this seems like it could be a separate change > > and would want its own unit test. > > So I'm a little worried about iframe's touch action here and would like to have > some guarding code to test the correctness in the wild, but maybe we can simply > throw the new logic to the wild and revert the patch if anything breaks. Yes, I think this is the right thing to do. https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:386: // support both the CSS width and height properties. Should we remove the computed touch action then? i.e. style.SetTouchAction(kTouchActionAuto); https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:394: // of the touched element and all its ancestors up to the one that implements Since we won't actually do a walk intersecting the touch-action when this effective touch action computation lands we should phrase this differently, perhaps: The effective touch action is the intersection of the touch-action values of the current element and all of its ancestors up to the one that implements the gesture. Since panning is implemented by the scroller it is re-enabled for scrolling elements. https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:406: element->GetDocument().LocalOwner()->GetComputedStyle(); This confuses me, why do we need to mask with the iframe's document's touch action as well as the local touch action style? https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:409: frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan; Shouldn't we re-enable panning if it was disabled by an ancestor since the iframe handles the gesture? https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:36: if (cur_node->GetLayoutObject()) { I wonder if we can use Node::GetComputedStyle? What are the cases we get a Node without a layout object? https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3034: OverflowY() == EOverflow::kOverlay; Can you move the code from HasAutoVerticalScrollbar and HasAutoHorizontalScrollbar to calls into the ComputedStyle as well and share the code?
The CQ bit was checked by sunxd@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...
PTAL. Resolved some comments. I'm going to try the TouchActionUtil stuff and recall the reason for making a tree walk there. https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:386: // support both the CSS width and height properties. On 2017/06/07 18:21:47, flackr wrote: > Should we remove the computed touch action then? i.e. > style.SetTouchAction(kTouchActionAuto); I think in the original code, we continue with tree walking even if the current layout object doesn't support touch action. This means an element without height or width can have an effective touch action inherited from its ancestor. I think we should maintain the same behavior here? In the pointer events specs, it is said: A touch behavior is supported if it conforms to the touch-action property of each element between the hit tested element and its nearest ancestor with the default touch behavior (including both the hit tested element and the element with the default touch behavior). I think we do not allow CSS touch-action property on those elements (their touch-action is equal to auto?), but it can still have a touch behavior? https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:394: // of the touched element and all its ancestors up to the one that implements On 2017/06/07 18:21:47, flackr wrote: > Since we won't actually do a walk intersecting the touch-action when this > effective touch action computation lands we should phrase this differently, > perhaps: > The effective touch action is the intersection of the touch-action values of the > current element and all of its ancestors up to the one that implements the > gesture. Since panning is implemented by the scroller it is re-enabled for > scrolling elements. Done. https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:406: element->GetDocument().LocalOwner()->GetComputedStyle(); On 2017/06/07 18:21:47, flackr wrote: > This confuses me, why do we need to mask with the iframe's document's touch > action as well as the local touch action style? I think we are masking with iframe's touch action style. In this situation (from touch-action-iframe.html): <iframe style='touch-action: none;' srcdoc=" <div expected-action='pan-x-y'> Zoom (but not scroll) related touch-action bits propagate into iframes </div> "></iframe> The internal div should get a panning touch action. If we do not mask with the iframe's effective touch action, we would get auto instead. https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:409: frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan; On 2017/06/07 18:21:47, flackr wrote: > Shouldn't we re-enable panning if it was disabled by an ancestor since the > iframe handles the gesture? Oh I think I should write: action = frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan instead. The parent action of a document element is always empty. https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:36: if (cur_node->GetLayoutObject()) { On 2017/06/07 18:21:47, flackr wrote: > I wonder if we can use Node::GetComputedStyle? What are the cases we get a Node > without a layout object? There are cases where the function is called with a non-element node and we do not have styles calculated for those nodes. I vaguely remember that sometimes the node does not have a layout object. I need to double check it. I think there are cases like this: +-node (with layout object) +---node (without element or layout object) +-----node (without element or layout object) https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.h:3034: OverflowY() == EOverflow::kOverlay; On 2017/06/07 18:21:47, flackr wrote: > Can you move the code from HasAutoVerticalScrollbar and > HasAutoHorizontalScrollbar to calls into the ComputedStyle as well and share the > code? Done.
https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:386: // support both the CSS width and height properties. On 2017/06/08 19:08:11, sunxd wrote: > On 2017/06/07 18:21:47, flackr wrote: > > Should we remove the computed touch action then? i.e. > > style.SetTouchAction(kTouchActionAuto); > > I think in the original code, we continue with tree walking even if the current > layout object doesn't support touch action. This means an element without height > or width can have an effective touch action inherited from its ancestor. I think > we should maintain the same behavior here? > > In the pointer events specs, it is said: > A touch behavior is supported if it conforms to the touch-action property of > each element between the hit tested element and its nearest ancestor with the > default touch behavior (including both the hit tested element and the element > with the default touch behavior). > > I think we do not allow CSS touch-action property on those elements (their > touch-action is equal to auto?), but it can still have a touch behavior? Sorry, I think there may be a misunderstanding, Still set the *effective* touch action to be |action| so that it is properly inherited, but you should also set the touch action to be auto since it's not allowed on that element. This will mean that getComputedStyle(elem).touchAction == 'auto' but the effective touch action will still inherit. https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:406: element->GetDocument().LocalOwner()->GetComputedStyle(); On 2017/06/08 19:08:11, sunxd wrote: > On 2017/06/07 18:21:47, flackr wrote: > > This confuses me, why do we need to mask with the iframe's document's touch > > action as well as the local touch action style? > > I think we are masking with iframe's touch action style. > > In this situation (from touch-action-iframe.html): > > <iframe style='touch-action: none;' srcdoc=" > <div expected-action='pan-x-y'> > Zoom (but not scroll) related touch-action bits propagate into iframes > </div> > "></iframe> > > The internal div should get a panning touch action. If we do not mask with the > iframe's effective touch action, we would get auto instead. Ah I see, so if I understand correctly, we're grabbing the iframe's touch action because it normally wouldn't inherit automatically? https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:409: frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan; On 2017/06/08 19:08:11, sunxd wrote: > On 2017/06/07 18:21:47, flackr wrote: > > Shouldn't we re-enable panning if it was disabled by an ancestor since the > > iframe handles the gesture? > > Oh I think I should write: > > action = frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan > > instead. > > The parent action of a document element is always empty. Yes, that makes more sense. https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:36: if (cur_node->GetLayoutObject()) { On 2017/06/08 19:08:12, sunxd wrote: > On 2017/06/07 18:21:47, flackr wrote: > > I wonder if we can use Node::GetComputedStyle? What are the cases we get a > Node > > without a layout object? > > There are cases where the function is called with a non-element node and we do > not have styles calculated for those nodes. > > I vaguely remember that sometimes the node does not have a layout object. I need > to double check it. I think there are cases like this: > > +-node (with layout object) > +---node (without element or layout object) > +-----node (without element or layout object) Acknowledged. https://codereview.chromium.org/2916563003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:403: // allow (panning & local touch action) on the first descendant element of a local touch action? Is this the owning frame node's action? Will this function be called on the child frame if the parent owner's effective touch action changes? It seems like it wouldn't know to since the parent_style didn't change. I think you might need to check if element is a HTMLFrameOwnerElement and then set the effective touch action on its contentDocument(). https://codereview.chromium.org/2916563003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2916563003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:3003: bool HasAutoHorizontalScrollbar() const { nit: We should avoid saying scrollbar, since it just may have a scrollbar and this depends on HasOverflowClip. HasAutoHorizontalScroll perhaps?
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...)
I haven't uploaded the patch as I'm still trying to work on the iframe case. https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:386: // support both the CSS width and height properties. On 2017/06/08 19:35:21, flackr wrote: > On 2017/06/08 19:08:11, sunxd wrote: > > On 2017/06/07 18:21:47, flackr wrote: > > > Should we remove the computed touch action then? i.e. > > > style.SetTouchAction(kTouchActionAuto); > > > > I think in the original code, we continue with tree walking even if the > current > > layout object doesn't support touch action. This means an element without > height > > or width can have an effective touch action inherited from its ancestor. I > think > > we should maintain the same behavior here? > > > > In the pointer events specs, it is said: > > A touch behavior is supported if it conforms to the touch-action property of > > each element between the hit tested element and its nearest ancestor with the > > default touch behavior (including both the hit tested element and the element > > with the default touch behavior). > > > > I think we do not allow CSS touch-action property on those elements (their > > touch-action is equal to auto?), but it can still have a touch behavior? > > Sorry, I think there may be a misunderstanding, Still set the *effective* touch > action to be |action| so that it is properly inherited, but you should also set > the touch action to be auto since it's not allowed on that element. This will > mean that getComputedStyle(elem).touchAction == 'auto' but the effective touch > action will still inherit. OK, that sounds reasonable to me, done! https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:409: frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan; On 2017/06/08 19:35:20, flackr wrote: > On 2017/06/08 19:08:11, sunxd wrote: > > On 2017/06/07 18:21:47, flackr wrote: > > > Shouldn't we re-enable panning if it was disabled by an ancestor since the > > > iframe handles the gesture? > > > > Oh I think I should write: > > > > action = frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan > > > > instead. > > > > The parent action of a document element is always empty. > > Yes, that makes more sense. Done. https://codereview.chromium.org/2916563003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:403: // allow (panning & local touch action) on the first descendant element of a On 2017/06/08 19:35:21, flackr wrote: > local touch action? Is this the owning frame node's action? > > Will this function be called on the child frame if the parent owner's effective > touch action changes? It seems like it wouldn't know to since the parent_style > didn't change. I think you might need to check if element is a > HTMLFrameOwnerElement and then set the effective touch action on its > contentDocument(). Yeah that's a good point, I have verified that a change of iframe's touch-action style cannot propagate to the document contents. I tried setting the effective touch actions of the contentDocument but it didn't trigger a recalcStyle of the internal element. Is there any way to manually trigger that? https://codereview.chromium.org/2916563003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/style/ComputedStyle.h (right): https://codereview.chromium.org/2916563003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/style/ComputedStyle.h:3003: bool HasAutoHorizontalScrollbar() const { On 2017/06/08 19:35:21, flackr wrote: > nit: We should avoid saying scrollbar, since it just may have a scrollbar and > this depends on HasOverflowClip. HasAutoHorizontalScroll perhaps? Done.
The CQ bit was checked by sunxd@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...
PTAL. There are only two tests remaining here: 1) Add StyleAdjuster unit test that tests "SupportsTouchAction", suggested by Walter. 2) Add a test case where a change of iframe's touch action would influence its descendants. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:525: style.OverflowY() == EOverflow::kWebkitPagedY || On 2017/06/05 20:12:03, sunxd wrote: > On 2017/06/05 18:43:10, flackr wrote: > > Why only kWebkitPagedY? Should overflow_x also check kWebkitPagedX? > > I adopted the logic from here: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > The change was introduced in > https://chromium.googlesource.com/chromium/src/+/862ec6da145e3a91cd547e40b79f... > > It seems we already automatically apply a scrollbar for -webkit-paged-x, but > it's not used to determine if a layout box scrolls? Done. https://codereview.chromium.org/2916563003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:540: // Apply TouchAction bits from its parent style On 2017/06/05 20:12:04, sunxd wrote: > On 2017/06/02 03:07:05, wkorman wrote: > > Clarify comment to note that we've incorporated other potential bits as well > per > > above logic. Since it reads as if, if we make it here, we're only applying > bits > > from the parent style otherwise unadulterated, but in actuality at this point > > |action| has been updated to at least potentially incorporate more. > > Acknowledged. Done. https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:406: element->GetDocument().LocalOwner()->GetComputedStyle(); On 2017/06/08 19:35:21, flackr wrote: > On 2017/06/08 19:08:11, sunxd wrote: > > On 2017/06/07 18:21:47, flackr wrote: > > > This confuses me, why do we need to mask with the iframe's document's touch > > > action as well as the local touch action style? > > > > I think we are masking with iframe's touch action style. > > > > In this situation (from touch-action-iframe.html): > > > > <iframe style='touch-action: none;' srcdoc=" > > <div expected-action='pan-x-y'> > > Zoom (but not scroll) related touch-action bits propagate into iframes > > </div> > > "></iframe> > > > > The internal div should get a panning touch action. If we do not mask with the > > iframe's effective touch action, we would get auto instead. > > Ah I see, so if I understand correctly, we're grabbing the iframe's touch action > because it normally wouldn't inherit automatically? Yes, now I made iframe updates only trigger the recalc, when recalculating the descendant document, the document will fetch effective touch action actively.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rune@opera.com changed reviewers: + rune@opera.com
https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:376: Element* element) { This method looks like it's doing a lot of checks in common cases like normal block level elements. I don't know how much this affects perf, but is it possible to return earlier for the case where you don't use touch-property in author style at all? https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:393: style.SetTouchAction(TouchAction::kTouchActionAuto); There's a difference between applies to, and computed style. This will not yield the correct computed style, I think. It makes a difference for explicit inheritance.
PTAL. I attempted to early out in style adjuster, but as the early out also involves several non-trivial condition checks, I'm not sure about the performance improvement. https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:376: Element* element) { On 2017/06/12 21:09:02, rune wrote: > This method looks like it's doing a lot of checks in common cases like normal > block level elements. I don't know how much this affects perf, but is it > possible to return earlier for the case where you don't use touch-property in > author style at all? Normally we can only early out when the element is atomic inline level which is the cases covered by the criteria in line 391. Unfortunately I think the early out can only happen if four conditions are met: 1) style.touch_action == Auto; 2) the element does not scroll overflow; 3) the element is not the first descendant of an iframe document; 4) the element is not an iframe. I added the early out logic in the new patch, but not sure if this saves performance. https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:393: style.SetTouchAction(TouchAction::kTouchActionAuto); On 2017/06/12 21:09:02, rune wrote: > There's a difference between applies to, and computed style. This will not yield > the correct computed style, I think. It makes a difference for explicit > inheritance. I think forcing touch action to be auto in line 393 is correct but may be redundant, as we only refer to effective touch action when handling touch events. It kinda eliminates violation of the rule. I think the same thing is done to sticky properties.
PTAL. I attempted to early out in style adjuster, but as the early out also involves several non-trivial condition checks, I'm not sure about the performance improvement.
Patchset #8 (id:140001) has been deleted
https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:376: Element* element) { On 2017/06/14 17:55:58, sunxd wrote: > On 2017/06/12 21:09:02, rune wrote: > > This method looks like it's doing a lot of checks in common cases like normal > > block level elements. I don't know how much this affects perf, but is it > > possible to return earlier for the case where you don't use touch-property in > > author style at all? > > Normally we can only early out when the element is atomic inline level which is > the cases covered by the criteria in line 391. Unfortunately I think the early > out can only happen if four conditions are met: > > 1) style.touch_action == Auto; > 2) the element does not scroll overflow; > 3) the element is not the first descendant of an iframe document; > 4) the element is not an iframe. > > I added the early out logic in the new patch, but not sure if this saves > performance. OK. Just remove it again then. https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:393: style.SetTouchAction(TouchAction::kTouchActionAuto); On 2017/06/14 17:55:57, sunxd wrote: > On 2017/06/12 21:09:02, rune wrote: > > There's a difference between applies to, and computed style. This will not > yield > > the correct computed style, I think. It makes a difference for explicit > > inheritance. > > I think forcing touch action to be auto in line 393 is correct but may be > redundant, as we only refer to effective touch action when handling touch > events. It kinda eliminates violation of the rule. I think the same thing is > done to sticky properties. I was thinking about the cssom computed style value. I suspect this will, incorrectly, output "auto" since the span is a non-replaced inline: <span style="touch-action: pan-x"></span> <script> document.write("should be 'pan-x': " + getComputedStyle(document.querySelector("span")).touchAction); </script>
Can you add some unit tests for effective touch action? https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:393: style.SetTouchAction(TouchAction::kTouchActionAuto); On 2017/06/15 00:04:40, rune wrote: > On 2017/06/14 17:55:57, sunxd wrote: > > On 2017/06/12 21:09:02, rune wrote: > > > There's a difference between applies to, and computed style. This will not > > yield > > > the correct computed style, I think. It makes a difference for explicit > > > inheritance. > > > > I think forcing touch action to be auto in line 393 is correct but may be > > redundant, as we only refer to effective touch action when handling touch > > events. It kinda eliminates violation of the rule. I think the same thing is > > done to sticky properties. > > I was thinking about the cssom computed style value. I suspect this will, > incorrectly, output "auto" since the span is a non-replaced inline: > > <span style="touch-action: pan-x"></span> > <script> > document.write("should be 'pan-x': " + > getComputedStyle(document.querySelector("span")).touchAction); > </script> Is this not consistent with how we rewrite position values which do not apply to certain display types? e.g. <table><thead style="position: relative;"></thead></table> <script> document.write(is 'static': " + getComputedStyle(document.querySelector("thead")).position); </script> I think if touch-action doesn't apply on certain elements it should show in computed style. https://codereview.chromium.org/2916563003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:434: content_document_element->SetNeedsStyleRecalc( Maybe SetEffectiveTouchAction on the content document and then let the inheritance proceed from there?
https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:393: style.SetTouchAction(TouchAction::kTouchActionAuto); On 2017/06/15 00:32:42, flackr wrote: > Is this not consistent with how we rewrite position values which do not apply to > certain display types? > > e.g. > <table><thead style="position: relative;"></thead></table> > <script> > document.write(is 'static': " + > getComputedStyle(document.querySelector("thead")).position); > </script> Yes, it's consistently buggy. Gecko gets this right. > I think if touch-action doesn't apply on certain elements it should show in > computed style. Not according to spec.
https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:393: style.SetTouchAction(TouchAction::kTouchActionAuto); On 2017/06/15 08:19:03, rune wrote: > On 2017/06/15 00:32:42, flackr wrote: > > > Is this not consistent with how we rewrite position values which do not apply > to > > certain display types? > > > > e.g. > > <table><thead style="position: relative;"></thead></table> > > <script> > > document.write(is 'static': " + > > getComputedStyle(document.querySelector("thead")).position); > > </script> > > Yes, it's consistently buggy. Gecko gets this right. > > > I think if touch-action doesn't apply on certain elements it should show in > > computed style. > > Not according to spec. My apologies, I thought it was intentional that we rewrote ComputedStyle properties which do not apply in StyleAdjuster.
PTAL. https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:376: Element* element) { On 2017/06/15 00:04:40, rune wrote: > On 2017/06/14 17:55:58, sunxd wrote: > > On 2017/06/12 21:09:02, rune wrote: > > > This method looks like it's doing a lot of checks in common cases like > normal > > > block level elements. I don't know how much this affects perf, but is it > > > possible to return earlier for the case where you don't use touch-property > in > > > author style at all? > > > > Normally we can only early out when the element is atomic inline level which > is > > the cases covered by the criteria in line 391. Unfortunately I think the early > > out can only happen if four conditions are met: > > > > 1) style.touch_action == Auto; > > 2) the element does not scroll overflow; > > 3) the element is not the first descendant of an iframe document; > > 4) the element is not an iframe. > > > > I added the early out logic in the new patch, but not sure if this saves > > performance. > > OK. Just remove it again then. Done. https://codereview.chromium.org/2916563003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:393: style.SetTouchAction(TouchAction::kTouchActionAuto); On 2017/06/15 14:16:20, flackr wrote: > On 2017/06/15 08:19:03, rune wrote: > > On 2017/06/15 00:32:42, flackr wrote: > > > > > Is this not consistent with how we rewrite position values which do not > apply > > to > > > certain display types? > > > > > > e.g. > > > <table><thead style="position: relative;"></thead></table> > > > <script> > > > document.write(is 'static': " + > > > getComputedStyle(document.querySelector("thead")).position); > > > </script> > > > > Yes, it's consistently buggy. Gecko gets this right. > > > > > I think if touch-action doesn't apply on certain elements it should show in > > > computed style. > > > > Not according to spec. > > My apologies, I thought it was intentional that we rewrote ComputedStyle > properties which do not apply in StyleAdjuster. Done. https://codereview.chromium.org/2916563003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:434: content_document_element->SetNeedsStyleRecalc( On 2017/06/15 00:32:42, flackr wrote: > Maybe SetEffectiveTouchAction on the content document and then let the > inheritance proceed from there? So "mysteriously" setting the effective touch action of the content document element does not trigger updates of that document's descendants. I tried that before, but only the HTML element got updated, BODY and touch-action DIV didn't get recomputed.
https://codereview.chromium.org/2916563003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:412: frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan; I still think it'd be cleaner to re-enable kTouchActionPan as the same for scrollsoverflow and subframes. I.e. something like this: bool is_child_document = element && element->GetDocument().documentElement() && element->GetDocument().LocalOwner() if (is_child_document) action = frame_style->GetEffectiveTouchAction(); if (style.scrollsOverflow() || is_child_document) action |= kTouchActionPan https://codereview.chromium.org/2916563003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:426: StyleChangeReason::kStyleSheetChange)); kStyleSheetChange is not the correct reason to set here. We should probably add a reason, maybe something like "InheritedStyleFromParentFrame"?
The CQ bit was checked by sunxd@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: This issue passed the CQ dry run.
The CQ bit was checked by sunxd@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...
PTAL. This patch mainly resolves iframe style change problem. https://codereview.chromium.org/2916563003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:412: frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan; On 2017/06/19 15:45:42, flackr wrote: > I still think it'd be cleaner to re-enable kTouchActionPan as the same for > scrollsoverflow and subframes. I.e. something like this: > > bool is_child_document = element && element->GetDocument().documentElement() && > element->GetDocument().LocalOwner() > > if (is_child_document) > action = frame_style->GetEffectiveTouchAction(); > > if (style.scrollsOverflow() || is_child_document) > action |= kTouchActionPan Done. https://codereview.chromium.org/2916563003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:426: StyleChangeReason::kStyleSheetChange)); On 2017/06/19 15:45:42, flackr wrote: > kStyleSheetChange is not the correct reason to set here. We should probably add > a reason, maybe something like "InheritedStyleFromParentFrame"? Done.
Also please add other tests for the effective touch action in non iframe cases. I.e. tests for: - re-enabling panning on scrollers - disabling cumulative touch actions from all descendants - modifying a parent touch action affecting a descendant https://codereview.chromium.org/2916563003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:410: if (frame_style) Is it possible for frame_style to be null? This is coming from an "ancestor" element so it should already have computed style shouldn't it? https://codereview.chromium.org/2916563003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutIFrameTest.cpp (right): https://codereview.chromium.org/2916563003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutIFrameTest.cpp:18: TEST_F(LayoutIFrameTest, TouchActionPropagatedAcrossIframes) { Can you make this part of StyleAdjusterTest to keep it with the code it is testing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 sunxd@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...
PTAL. I've added tests that reflects logic when changing touch-action styles. But I don't get this situation: "disabling cumulative touch actions from all descendants", I think cumulative touch actions are from ancestors? In the test, I wrote a case where parent's pan-x is changed to pan-y, is that what this statement refers to? https://codereview.chromium.org/2916563003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:410: if (frame_style) On 2017/06/21 15:24:34, flackr wrote: > Is it possible for frame_style to be null? This is coming from an "ancestor" > element so it should already have computed style shouldn't it? In many webkit_unit_tests, e.g. PrintContextFrameTest.WithScrolledSubframe, computing child's properties can trigger a nullptr here. I think the reason is that a lot of the tests do not call UpdateAllLifecyclePhases. I don't know whether we should fix this (add a TODO now and fix it with a new patch, as there are too many test failures) or we add a guarding if-condition here. https://codereview.chromium.org/2916563003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutIFrameTest.cpp (right): https://codereview.chromium.org/2916563003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutIFrameTest.cpp:18: TEST_F(LayoutIFrameTest, TouchActionPropagatedAcrossIframes) { On 2017/06/21 15:24:34, flackr wrote: > Can you make this part of StyleAdjusterTest to keep it with the code it is > testing? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The other case I was thinking was something like
<div id="a" style="touch-action: pan-x pan-y">
<div id="b" style="touch-action: pan-right pan-y">
<div id="c" style="touch-action: pan-x">
</div>
</div>
</div>
This way we test the further restriction of the touch action by #b on #c (to
become pan-right) and if you change #b's touch-action to "auto" #c should become
pan-x again.
https://codereview.chromium.org/2916563003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp (right):
https://codereview.chromium.org/2916563003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp:30:
ASSERT_TRUE(target && target->GetComputedStyle());
These asserts are unnecessary - it will crash if these don't exist when you use
them.
https://codereview.chromium.org/2916563003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp:42:
TEST_F(StyleAdjusterTest, TouchActionPanningReEnabledByScrolles) {
nit:s/Scrolles/Scroller
https://codereview.chromium.org/2916563003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp:45:
"<style>#ancestor { margin: 0; touch-action: none; } "
Can we set something like pinch-zoom to verify that pan is added rather than
replaces the touch-action?
The CQ bit was checked by sunxd@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 checked by sunxd@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...
PTAL. https://codereview.chromium.org/2916563003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp (right): https://codereview.chromium.org/2916563003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp:30: ASSERT_TRUE(target && target->GetComputedStyle()); On 2017/06/22 19:15:31, flackr wrote: > These asserts are unnecessary - it will crash if these don't exist when you use > them. Done. https://codereview.chromium.org/2916563003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp:42: TEST_F(StyleAdjusterTest, TouchActionPanningReEnabledByScrolles) { On 2017/06/22 19:15:31, flackr wrote: > nit:s/Scrolles/Scroller Done. https://codereview.chromium.org/2916563003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp:45: "<style>#ancestor { margin: 0; touch-action: none; } " On 2017/06/22 19:15:31, flackr wrote: > Can we set something like pinch-zoom to verify that pan is added rather than > replaces the touch-action? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:402: // iframe element. nit: I think this comment would be better placed by the if on 414 where we re-enable panning. https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:420: if (element && element->IsFrameOwnerElement() && nit: Document that we inherit touch action across frames. https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp (right): https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp:95: target->GetComputedStyle()->GetEffectiveTouchAction()); And then set touch-action: auto on #parent and verify that the effective touch action changes to pan-x? https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:17: return node.GetComputedStyle() Do some nodes have a null ComputedStyle? Maybe we still need to walk to the nearest node which has a ComputedStyle.
The CQ bit was checked by sunxd@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...
PTAL. https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:402: // iframe element. On 2017/06/29 14:34:06, flackr wrote: > nit: I think this comment would be better placed by the if on 414 where we > re-enable panning. Done. https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:420: if (element && element->IsFrameOwnerElement() && On 2017/06/29 14:34:06, flackr wrote: > nit: Document that we inherit touch action across frames. Done. https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp (right): https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjusterTest.cpp:95: target->GetComputedStyle()->GetEffectiveTouchAction()); On 2017/06/29 14:34:06, flackr wrote: > And then set touch-action: auto on #parent and verify that the effective touch > action changes to pan-x? Done. https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:17: return node.GetComputedStyle() On 2017/06/29 14:34:06, flackr wrote: > Do some nodes have a null ComputedStyle? Maybe we still need to walk to the > nearest node which has a ComputedStyle. Done.
LGTM with nit https://codereview.chromium.org/2916563003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:34: return node.GetComputedStyle()->GetEffectiveTouchAction(); This check and early return seems unnecessary since it's checked again first thing in the for loop.
https://codereview.chromium.org/2916563003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:17: const Node* ParentNodeAcrossFrames(const Node* cur_node) { I think the document node will have a computed style so we shouldn't need to step across frames.
The CQ bit was checked by sunxd@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...
sunxd@chromium.org changed reviewers: + chrishtr@chromium.org
Hi Chris, Can you take a look at this change please? Thanks.
chrishtr@chromium.org changed reviewers: + meade@chromium.org
Rune is already reviewing. Addding meade@ for a second pair of style eyes. This is a style CL.
https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:391: if (is_non_replaced_inline_elements || is_table_row_or_column) { Does touch-action apply to display:contents? https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:20: return cur_node->GetComputedStyle()->GetEffectiveTouchAction(); GetComputedStyle() should only be null for display:none. Do we ever care about effective touch action for display:none nodes? Why do we need a traversal at all?
https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:20: return cur_node->GetComputedStyle()->GetEffectiveTouchAction(); On 2017/06/29 21:20:21, rune wrote: > GetComputedStyle() should only be null for display:none. Do we ever care about > effective touch action for display:none nodes? > > Why do we need a traversal at all? No, if something is display: none it can't be touched so we don't need to know the effective touch action.
+shend and nainar who have been working on ComputedStyle and co. and can review those. I don't have a particularly strong understanding on what should or shouldn't go in StyleAdjuster - rune is likely better placed to understand that.
meade@chromium.org changed reviewers: + nainar@chromium.org, shend@chromium.org
actually adding nainar and shend this time
https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:39: 'LineClampValue', 'OutlineValue', 'TouchAction', 'unsigned', 'size_t', 'int', (A) You won't need this if you put "field_size: 6" into the JSON. https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 (right): https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:492: field_group: "rare-inherited", Adding "field_size: 6" would pack the storage of this field as a bit field of size 6 (6 is from [1]). This saves space and it also means that you won't need to put TouchAction into the ALIGNMENT_ORDER list (see comment (A)). [1] https://cs.chromium.org/chromium/src/cc/input/touch_action.h?q=TouchAction&sq...
The CQ bit was checked by sunxd@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...
PTAL. I applied the suggested changes to json files. Regarding display:contents element, I checked the original code, those elements do not have layout objects at all. TouchActionUtil's traversal skips all elements that do not have a layout object, so I added that condition to the early out code path to style adjuster. https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_computed_style_base.py:39: 'LineClampValue', 'OutlineValue', 'TouchAction', 'unsigned', 'size_t', 'int', On 2017/06/30 03:05:40, shend wrote: > (A) You won't need this if you put "field_size: 6" into the JSON. Done. https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 (right): https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:492: field_group: "rare-inherited", On 2017/06/30 03:05:40, shend wrote: > Adding "field_size: 6" would pack the storage of this field as a bit field of > size 6 (6 is from [1]). This saves space and it also means that you won't need > to put TouchAction into the ALIGNMENT_ORDER list (see comment (A)). > > [1] > https://cs.chromium.org/chromium/src/cc/input/touch_action.h?q=TouchAction&sq... Done. https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchActionUtil.cpp (right): https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchActionUtil.cpp:20: return cur_node->GetComputedStyle()->GetEffectiveTouchAction(); On 2017/06/30 02:49:54, flackr wrote: > On 2017/06/29 21:20:21, rune wrote: > > GetComputedStyle() should only be null for display:none. Do we ever care about > > effective touch action for display:none nodes? > > > > Why do we need a traversal at all? > > No, if something is display: none it can't be touched so we don't need to know > the effective touch action. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/04 at 15:35:42, sunxd wrote: > PTAL. > > I applied the suggested changes to json files. > Regarding display:contents element, I checked the original code, those elements do not have layout objects at all. TouchActionUtil's traversal skips all elements that do not have a layout object, so I added that condition to the early out code path to style adjuster. > > https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... > File third_party/WebKit/Source/build/scripts/make_computed_style_base.py (right): > > https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/build/scripts/make_computed_style_base.py:39: 'LineClampValue', 'OutlineValue', 'TouchAction', 'unsigned', 'size_t', 'int', > On 2017/06/30 03:05:40, shend wrote: > > (A) You won't need this if you put "field_size: 6" into the JSON. > > Done. > > https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 (right): > > https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:492: field_group: "rare-inherited", > On 2017/06/30 03:05:40, shend wrote: > > Adding "field_size: 6" would pack the storage of this field as a bit field of > > size 6 (6 is from [1]). This saves space and it also means that you won't need > > to put TouchAction into the ALIGNMENT_ORDER list (see comment (A)). > > > > [1] > > https://cs.chromium.org/chromium/src/cc/input/touch_action.h?q=TouchAction&sq... > > Done. lgtm for ComputedStyle and JSON.
On 2017/07/04 23:26:50, shend wrote: > On 2017/07/04 at 15:35:42, sunxd wrote: > > PTAL. > > > > I applied the suggested changes to json files. > > Regarding display:contents element, I checked the original code, those > elements do not have layout objects at all. TouchActionUtil's traversal skips > all elements that do not have a layout object, so I added that condition to the > early out code path to style adjuster. > > > > > https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/build/scripts/make_computed_style_base.py > (right): > > > > > https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... > > third_party/WebKit/Source/build/scripts/make_computed_style_base.py:39: > 'LineClampValue', 'OutlineValue', 'TouchAction', 'unsigned', 'size_t', 'int', > > On 2017/06/30 03:05:40, shend wrote: > > > (A) You won't need this if you put "field_size: 6" into the JSON. > > > > Done. > > > > > https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5 > (right): > > > > > https://codereview.chromium.org/2916563003/diff/340001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/css/ComputedStyleExtraFields.json5:492: > field_group: "rare-inherited", > > On 2017/06/30 03:05:40, shend wrote: > > > Adding "field_size: 6" would pack the storage of this field as a bit field > of > > > size 6 (6 is from [1]). This saves space and it also means that you won't > need > > > to put TouchAction into the ALIGNMENT_ORDER list (see comment (A)). > > > > > > [1] > > > > https://cs.chromium.org/chromium/src/cc/input/touch_action.h?q=TouchAction&sq... > > > > Done. > > lgtm for ComputedStyle and JSON. Hi meade@, Since rune@ is out on vacation, do you have any suggestion who else can review the StyleAdjuster change? Thanks!
In that case, StyleAdjuster lgtm so you're not blocked, but it is hit a lot, so pay attention to the perf bots after you land your CL.
One nit comment. But lgtm otherwise. https://codereview.chromium.org/2916563003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:379: bool is_svg_root = element && element->IsSVGElement() && Why not use this function instead? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... You can get a LayoutObject from an Element.
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
https://codereview.chromium.org/2916563003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:379: bool is_svg_root = element && element->IsSVGElement() && On 2017/07/12 06:27:59, nainar wrote: > Why not use this function instead? > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > You can get a LayoutObject from an Element. Hmm I think we still do not have a layout object created when recalc styles.
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 still https://codereview.chromium.org/2916563003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right): https://codereview.chromium.org/2916563003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:379: bool is_svg_root = element && element->IsSVGElement() && On 2017/07/12 at 16:06:28, sunxd wrote: > On 2017/07/12 06:27:59, nainar wrote: > > Why not use this function instead? > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > > > You can get a LayoutObject from an Element. > > Hmm I think we still do not have a layout object created when recalc styles. cool that makes sense
The CQ bit was checked by sunxd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, meade@chromium.org, shend@chromium.org Link to the patchset: https://codereview.chromium.org/2916563003/#ps380001 (title: "Rebase")
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": 380001, "attempt_start_ts": 1499954149702500,
"parent_rev": "850575d376f717c38fb6bb9c48d35d648705f6d2", "commit_rev":
"4d9c0082acd8bd604a1a44260dc6f62a400e5ddb"}
Message was sent while issue was closed.
Description was changed from ========== Compute effective touch action in StyleAdjuster. We used to store touch action bits in ComputedStyle and recompute the effective touch action in TouchActionUtil whenever we need to use it. In order to process touch action info in cc, we need to adjust this method as cc has no access to TouchActionUtil. We can compute them when recomputing styles or in cc. As cc lacks hit testing information, it is more feasible to do it in StyleAdjuster. Pre-computing the style also saves time by avoiding tree walks. This CL adds the EffectiveTouchAction to non-css RareInheritedData and uses it when computing styles. There are also special cases where touch action is not supported in atomic inline level elements and there are exceptions if an element is an iframe or scrolls. BUG=727853 ========== to ========== Compute effective touch action in StyleAdjuster. We used to store touch action bits in ComputedStyle and recompute the effective touch action in TouchActionUtil whenever we need to use it. In order to process touch action info in cc, we need to adjust this method as cc has no access to TouchActionUtil. We can compute them when recomputing styles or in cc. As cc lacks hit testing information, it is more feasible to do it in StyleAdjuster. Pre-computing the style also saves time by avoiding tree walks. This CL adds the EffectiveTouchAction to non-css RareInheritedData and uses it when computing styles. There are also special cases where touch action is not supported in atomic inline level elements and there are exceptions if an element is an iframe or scrolls. BUG=727853 Review-Url: https://codereview.chromium.org/2916563003 Cr-Commit-Position: refs/heads/master@{#486363} Committed: https://chromium.googlesource.com/chromium/src/+/4d9c0082acd8bd604a1a44260dc6... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:380001) as https://chromium.googlesource.com/chromium/src/+/4d9c0082acd8bd604a1a44260dc6... |
