Compute effective touch action in StyleAdjuster.

third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp

Index: third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp
diff --git a/third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp b/third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp
index 051088eea9a012c4fd362fdc75944d906903af5a..02c27fe5f1d4f89cd375e0039b7b39e870102b87 100644
--- a/third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp
+++ b/third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp
@@ -35,6 +35,7 @@
 #include "core/dom/ContainerNode.h"
 #include "core/dom/Document.h"
 #include "core/dom/Element.h"
+#include "core/dom/NodeComputedStyle.h"
 #include "core/frame/LocalFrameView.h"
 #include "core/frame/Settings.h"
 #include "core/frame/UseCounter.h"
@@ -44,6 +45,7 @@
 #include "core/html/HTMLPlugInElement.h"
 #include "core/html/HTMLTableCellElement.h"
 #include "core/html/HTMLTextAreaElement.h"
+#include "core/layout/LayoutObject.h"
 #include "core/layout/LayoutTheme.h"
 #include "core/style/ComputedStyle.h"
 #include "core/style/ComputedStyleConstants.h"
@@ -451,6 +453,7 @@ void StyleAdjuster::AdjustComputedStyle(
   AdjustStyleForEditing(style);
 
   bool is_svg_element = element && element->IsSVGElement();
+  bool is_svg_root = false;

[wkorman, 2017/06/02 03:07:05]

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.

[flackr, 2017/06/05 18:43:10]

nit: Calculate this here, i.e.
bool is_svg_root = is_svg_element && !(parent is svg element);

[sunxd, 2017/06/05 20:12:04]

Done % StyleAdjusterTest.

[sunxd, 2017/06/05 20:12:04]

Sure I'll look into this.

[sunxd, 2017/06/05 20:12:04]

Acknowledged.

   if (is_svg_element) {
     // display: contents computes to inline for replaced elements and form
     // controls, and isn't specified for other kinds of SVG content[1], so let's
@@ -467,8 +470,11 @@ void StyleAdjuster::AdjustComputedStyle(
     // Only the root <svg> element in an SVG document fragment tree honors css
     // position.
     if (!(isSVGSVGElement(*element) && element->parentNode() &&
-          !element->parentNode()->IsSVGElement()))
+          !element->parentNode()->IsSVGElement())) {
       style.SetPosition(ComputedStyle::InitialPosition());
+    } else {
+      is_svg_root = true;
+    }
 
     // SVG text layout code expects us to be a block-level style element.
     if ((isSVGForeignObjectElement(*element) || isSVGTextElement(*element)) &&
@@ -496,6 +502,43 @@ void StyleAdjuster::AdjustComputedStyle(
     if (parent_style.JustifyItemsPositionType() == kLegacyPosition)
       style.SetJustifyItems(parent_style.JustifyItems());
   }
+
+  TouchAction action = parent_style.GetEffectiveTouchAction();
+  bool is_non_replaced_inline_elements =
+      style.IsDisplayInlineType() &&
+      !(style.IsDisplayReplacedType() || is_svg_root ||
+        isHTMLImageElement(element));
+  bool is_table_row_or_column = style.Display() == EDisplay::kTableRow ||

[wkorman, 2017/06/02 03:07:05]

Can we use IsDisplayTableType() or is that too broad?

[sunxd, 2017/06/05 20:12:03]

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().

[sunxd, 2017/06/06 15:24:59]

Done.

+                                style.Display() == EDisplay::kTableRowGroup ||
+                                style.Display() == EDisplay::kTableColumn ||
+                                style.Display() == EDisplay::kTableColumnGroup;
+  if (is_non_replaced_inline_elements || is_table_row_or_column) {

[wkorman, 2017/06/02 03:07:05]

What made us choose this filtering criteria? If there is a relevant section of
W3C spec worth referencing in a comment here.

[sunxd, 2017/06/05 20:12:03]

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.

[sunxd, 2017/06/05 20:12:04]

Acknowledged.

[sunxd, 2017/06/06 15:24:58]

Done.

+    style.SetEffectiveTouchAction(action);
+    return;

[flackr, 2017/06/05 18:43:10]

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.

[sunxd, 2017/06/05 20:12:04]

Acknowledged.

[sunxd, 2017/06/06 15:24:58]

Done.

+  }
+
+  bool overflow_x = style.OverflowX() == EOverflow::kScroll ||
+                    style.OverflowX() == EOverflow::kAuto ||
+                    style.OverflowX() == EOverflow::kOverlay;

[flackr, 2017/06/05 18:43:10]

Seems like this could use a helper function on ComputedStyle.

[sunxd, 2017/06/05 20:12:03]

Acknowledged.

[sunxd, 2017/06/06 15:24:59]

Done.

+  bool overflow_y = style.OverflowY() == EOverflow::kScroll ||
+                    style.OverflowY() == EOverflow::kAuto ||
+                    style.OverflowY() == EOverflow::kWebkitPagedY ||

[flackr, 2017/06/05 18:43:10]

Why only kWebkitPagedY? Should overflow_x also check kWebkitPagedX?

[sunxd, 2017/06/05 20:12:03]

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?

[sunxd, 2017/06/12 17:27:04]

Done.

+                    style.OverflowY() == EOverflow::kOverlay;
+  if (overflow_x || overflow_y)
+    action |= TouchAction::kTouchActionPan;

[wkorman, 2017/06/02 03:07:05]

Add a comment re: why we or-in kTouchActionPan here and below.

[sunxd, 2017/06/05 20:12:04]

Acknowledged.

[sunxd, 2017/06/06 15:24:58]

Done.

+
+  if (element && element == element->GetDocument().documentElement() &&

[wkorman, 2017/06/02 03:07:05]

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.

[sunxd, 2017/06/05 20:12:03]

Acknowledged.

[sunxd, 2017/06/06 15:24:59]

Done.

+      element->GetDocument().LocalOwner()) {
+    const ComputedStyle* frame_style =
+        element->GetDocument().LocalOwner()->GetComputedStyle();
+    if (frame_style) {
+      action &=
+          frame_style->GetEffectiveTouchAction() | TouchAction::kTouchActionPan;

[flackr, 2017/06/05 18:43:10]

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.

[sunxd, 2017/06/05 20:12:04]

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).

+    }
+  }
+
+  // Apply TouchAction bits from its parent style

[wkorman, 2017/06/02 03:07:05]

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.

[sunxd, 2017/06/05 20:12:04]

Acknowledged.

[sunxd, 2017/06/12 17:27:04]

Done.

+  style.SetEffectiveTouchAction(style.GetTouchAction() & action);
 }
 
 }  // namespace blink