Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1131093002:
Implement direction-specific touch-action values (chromium side). (Closed)
|
Created:
5 years, 7 months ago by dtapuska Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jdduke+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement direction-specific touch-action values.
Add PanLeft, PanRight, PanUp and PanDown support by generalizing the PanX and PanY code.
The fix for the bug is a 3 part change and is required
because of the dependencies of blink as there is a static
cast (causing a ABI dependency). To work around this ABI
problem this is part 1 adding the PanXXX definitions to the
content classes and disabling the static assertions and
casting. The second change is
https://codereview.chromium.org/1137483003/. Once that
change has landed a follow up change to this change one
will remove the #if 0 and re-add the static cast between
blink::WebTouchAction and content::TouchAction
BUG=476556
TEST=content_unittests
Committed: https://crrev.com/a98ac8d7b972c2ca598391e550bef58e2d537684
Cr-Commit-Position: refs/heads/master@{#329011}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fix nits from patch set 1 #
Total comments: 4
Patch Set 3 : Clarify the axis clearing code #
Messages
Total messages: 22 (3 generated)
dtapuska@chromium.org changed reviewers: + rbyers@chromium.org
https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/touch_action_filter.cc (right): https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/touch_action_filter.cc:195: } else if (gesture_event.data.scrollBegin.deltaXHint > 0 && Please check this logic.. I originally wrote it as < 0 for this line (and the converse for the pan_right). And similar changes on the pan-down/pan-up. But it seemed to move in the incorrect direction as to what I thought it would be. I can show you the implementation if you like.
This is excellent, thanks! Can you please clarify the CL description slightly to make it clear that this is the CL that's landing first (technically the blink CL depends on this to keep things from breaking, not the other away around). https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/touch_action_filter.cc (right): https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/touch_action_filter.cc:38: if (allowed_touch_action_ & TOUCH_ACTION_PAN_X && This is now sufficiently non-trivial that I think it deserves a comment. Eg: "Scrolls restricted to a specific axis shouldn't permit movement in the perpendicular axis.". I wrote this into the spec as "when scrolling is restricted to starting along a single axis (eg. pan-y), the axis cannot be changed during the scroll". https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/touch_action_filter.cc:193: if ((allowed_touch_action_ & TOUCH_ACTION_PAN_X) == TOUCH_ACTION_PAN_X) { nit: You could remove this case (it's covered by the combination of the below two), right? https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/touch_action_filter.cc:195: } else if (gesture_event.data.scrollBegin.deltaXHint > 0 && On 2015/05/07 19:06:30, Dave Tapuska wrote: > Please check this logic.. > > I originally wrote it as < 0 for this line (and the converse for the pan_right). > And similar changes on the pan-down/pan-up. > > But it seemed to move in the incorrect direction as to what I thought it would > be. I can show you the implementation if you like. Yes, this is correct but I've got a bug in the spec in this regard (confusion motion of the finger and motion of the content). Will update and cc you on public-pointer-events thread. https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/touch_action_filter.cc:206: return false; ditto https://codereview.chromium.org/1131093002/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1131093002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:2340: // TODO(dtapuska): A dependant change needs to land in blink to change nit: reference your bug number so someone who sees this can get the current status / history.
https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/touch_action_filter.cc (right): https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/touch_action_filter.cc:38: if (allowed_touch_action_ & TOUCH_ACTION_PAN_X && On 2015/05/07 20:37:37, Rick Byers wrote: > This is now sufficiently non-trivial that I think it deserves a comment. Eg: > "Scrolls restricted to a specific axis shouldn't permit movement in the > perpendicular axis.". > > I wrote this into the spec as "when scrolling is restricted to starting along a > single axis (eg. pan-y), the axis cannot be changed during the scroll". Done. https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/touch_action_filter.cc:193: if ((allowed_touch_action_ & TOUCH_ACTION_PAN_X) == TOUCH_ACTION_PAN_X) { On 2015/05/07 20:37:37, Rick Byers wrote: > nit: You could remove this case (it's covered by the combination of the below > two), right? Done. https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/touch_action_filter.cc:195: } else if (gesture_event.data.scrollBegin.deltaXHint > 0 && On 2015/05/07 20:37:37, Rick Byers wrote: > On 2015/05/07 19:06:30, Dave Tapuska wrote: > > Please check this logic.. > > > > I originally wrote it as < 0 for this line (and the converse for the > pan_right). > > And similar changes on the pan-down/pan-up. > > > > But it seemed to move in the incorrect direction as to what I thought it would > > be. I can show you the implementation if you like. > > Yes, this is correct but I've got a bug in the spec in this regard (confusion > motion of the finger and motion of the content). Will update and cc you on > public-pointer-events thread. Acknowledged. https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/touch_action_filter.cc:206: return false; On 2015/05/07 20:37:37, Rick Byers wrote: > ditto Done. https://codereview.chromium.org/1131093002/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1131093002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:2340: // TODO(dtapuska): A dependant change needs to land in blink to change On 2015/05/07 20:37:37, Rick Byers wrote: > nit: reference your bug number so someone who sees this can get the current > status / history. Done.
rbyers@chromium.org changed reviewers: + jdduke@chromium.org, jochen@chromium.org
LGTM +jdduke for OWNERS in content/browser/renderer_host/input +jochen for OWNERS in the rest of content/
https://codereview.chromium.org/1131093002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/touch_action_filter.cc (right): https://codereview.chromium.org/1131093002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/touch_action_filter.cc:40: if (allowed_touch_action_ & TOUCH_ACTION_PAN_X && Is this correct? What if I have TOUCH_ACTION_PAN_X and TOUCH_ACTION_PINCH_ZOOM? Is that allowed? If so, looks like that may already be broken. Would the following be more robust? if ((allowed_touch_action_ & TOUCH_ACTION_PAN_X) && !(allowed_touch_action_ & TOUCH_ACTION_PAN_Y)) { ... } I slightly prefer parentheses when both & and && are involved. We could also have a helper moved into an anonymous namespace at the top: bool IsScrollYDisallowed(action); bool IsScrollXDisallowed(action);
https://codereview.chromium.org/1131093002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/touch_action_filter.cc (right): https://codereview.chromium.org/1131093002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/touch_action_filter.cc:40: if (allowed_touch_action_ & TOUCH_ACTION_PAN_X && On 2015/05/08 16:46:41, jdduke wrote: > Is this correct? What if I have TOUCH_ACTION_PAN_X and TOUCH_ACTION_PINCH_ZOOM? > Is that allowed? If so, looks like that may already be broken. No it's not allowed. If pinch-zoom is allowed then we require panning to be allowed in all directions too. > Would the following be more robust? > > if ((allowed_touch_action_ & TOUCH_ACTION_PAN_X) && > !(allowed_touch_action_ & TOUCH_ACTION_PAN_Y)) { > ... > } That's fine with me too, given what's currently allowed by blink it's identical. I guess we could reason about what a compromised renderer could cause to happen... > > I slightly prefer parentheses when both & and && are involved. We could also > have a helper moved into an anonymous namespace at the top: > > bool IsScrollYDisallowed(action); > bool IsScrollXDisallowed(action);
https://codereview.chromium.org/1131093002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/touch_action_filter.cc (right): https://codereview.chromium.org/1131093002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/touch_action_filter.cc:40: if (allowed_touch_action_ & TOUCH_ACTION_PAN_X && On 2015/05/08 16:55:08, Rick Byers wrote: > On 2015/05/08 16:46:41, jdduke wrote: > > Is this correct? What if I have TOUCH_ACTION_PAN_X and > TOUCH_ACTION_PINCH_ZOOM? > > Is that allowed? If so, looks like that may already be broken. > > No it's not allowed. If pinch-zoom is allowed then we require panning to be > allowed in all directions too. > > > Would the following be more robust? > > > > if ((allowed_touch_action_ & TOUCH_ACTION_PAN_X) && > > !(allowed_touch_action_ & TOUCH_ACTION_PAN_Y)) { > > ... > > } > > That's fine with me too, given what's currently allowed by blink it's identical. > I guess we could reason about what a compromised renderer could cause to > happen... > > > > > I slightly prefer parentheses when both & and && are involved. We could also > > have a helper moved into an anonymous namespace at the top: > > > > bool IsScrollYDisallowed(action); > > bool IsScrollXDisallowed(action); > Gotcha, I slightly prefer the more explicit model as it's a bit easier to read/reason about. Dave would you mind making that change?
On 2015/05/08 16:58:35, jdduke wrote: > https://codereview.chromium.org/1131093002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/touch_action_filter.cc (right): > > https://codereview.chromium.org/1131093002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/touch_action_filter.cc:40: if > (allowed_touch_action_ & TOUCH_ACTION_PAN_X && > On 2015/05/08 16:55:08, Rick Byers wrote: > > On 2015/05/08 16:46:41, jdduke wrote: > > > Is this correct? What if I have TOUCH_ACTION_PAN_X and > > TOUCH_ACTION_PINCH_ZOOM? > > > Is that allowed? If so, looks like that may already be broken. > > > > No it's not allowed. If pinch-zoom is allowed then we require panning to be > > allowed in all directions too. > > > > > Would the following be more robust? > > > > > > if ((allowed_touch_action_ & TOUCH_ACTION_PAN_X) && > > > !(allowed_touch_action_ & TOUCH_ACTION_PAN_Y)) { > > > ... > > > } > > > > That's fine with me too, given what's currently allowed by blink it's > identical. > > I guess we could reason about what a compromised renderer could cause to > > happen... > > > > > > > > I slightly prefer parentheses when both & and && are involved. We could also > > > have a helper moved into an anonymous namespace at the top: > > > > > > bool IsScrollYDisallowed(action); > > > bool IsScrollXDisallowed(action); > > > > Gotcha, I slightly prefer the more explicit model as it's a bit easier to > read/reason about. Dave would you mind making that change? Ya I'll make the change..
can you please file a tracking bug an include it in the CL description?
On 2015/05/08 17:26:06, jochen wrote: > can you please file a tracking bug an include it in the CL description? hmm; looks like it got lost when I updated the description.. its there in my commit msg... will update.
https://codereview.chromium.org/1131093002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/touch_action_filter.cc (right): https://codereview.chromium.org/1131093002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/touch_action_filter.cc:40: if (allowed_touch_action_ & TOUCH_ACTION_PAN_X && On 2015/05/08 16:58:35, jdduke wrote: > On 2015/05/08 16:55:08, Rick Byers wrote: > > On 2015/05/08 16:46:41, jdduke wrote: > > > Is this correct? What if I have TOUCH_ACTION_PAN_X and > > TOUCH_ACTION_PINCH_ZOOM? > > > Is that allowed? If so, looks like that may already be broken. > > > > No it's not allowed. If pinch-zoom is allowed then we require panning to be > > allowed in all directions too. > > > > > Would the following be more robust? > > > > > > if ((allowed_touch_action_ & TOUCH_ACTION_PAN_X) && > > > !(allowed_touch_action_ & TOUCH_ACTION_PAN_Y)) { > > > ... > > > } > > > > That's fine with me too, given what's currently allowed by blink it's > identical. > > I guess we could reason about what a compromised renderer could cause to > > happen... > > > > > > > > I slightly prefer parentheses when both & and && are involved. We could also > > > have a helper moved into an anonymous namespace at the top: > > > > > > bool IsScrollYDisallowed(action); > > > bool IsScrollXDisallowed(action); > > > > Gotcha, I slightly prefer the more explicit model as it's a bit easier to > read/reason about. Dave would you mind making that change? Done.. I used Is(X|Y)AxisActionDisallowed methods so it makes sense calling them in the fling code path; hope that is ok.
content/browser/renderer_host/input and content/common/input lgtm.
On 2015/05/08 17:47:30, jdduke wrote: > content/browser/renderer_host/input and content/common/input lgtm. still LGTM
https://codereview.chromium.org/1131093002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/input/touch_action_filter.cc:195: } else if > (gesture_event.data.scrollBegin.deltaXHint > 0 && > On 2015/05/07 19:06:30, Dave Tapuska wrote: > > Please check this logic.. > > > > I originally wrote it as < 0 for this line (and the converse for the > pan_right). > > And similar changes on the pan-down/pan-up. > > > > But it seemed to move in the incorrect direction as to what I thought it would > > be. I can show you the implementation if you like. > > Yes, this is correct but I've got a bug in the spec in this regard (confusion > motion of the finger and motion of the content). Will update and cc you on > public-pointer-events thread. This is now corrected in the spec. Discussion thread here: https://lists.w3.org/Archives/Public/public-pointer-events/2015AprJun/0100.html. Also, Dave - you said you modified my touch-action test page, right? Can you submit a pull request on github so I can update the official copy with these new features? Ideally you'd use feature detection (test the CSS parsing) to show the new options only when the browser supports them.
lgtm
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1131093002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a98ac8d7b972c2ca598391e550bef58e2d537684 Cr-Commit-Position: refs/heads/master@{#329011} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
