Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
163933002:
Send early ShowPress on TapDown when page isn't scrollable/pinchable. (Closed)
|
Created:
6 years, 10 months ago by Zeeshan Qureshi Modified:
6 years, 5 months ago CC:
blink-reviews, dglazkov+blink, Nate Chapin, eae+blinkwatch, gavinp+loader_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionSend early ShowPress on TapDown when page isn't scrollable/pinchable.
Blink uses the ShowPress gesture to determine when to display hover and active
states. If the page isn't scrollable or pinchable and if the current TapDown
isn't the start of a scroll then generate an early ShowPress in WebView and
ignore the actual ShowPress received from chromium later.
Additionally, refactor subframe calculation for hit testing into
frameForGestureEvent()
BUG=306581
Patch Set 1 #
Total comments: 9
Patch Set 2 : Move to WebViewImpl #Patch Set 3 : Fix touch adjustment #Patch Set 4 : Add tests #
Total comments: 14
Patch Set 5 : Rebase ToT #Patch Set 6 : Handle special form elements / iframes #
Total comments: 1
Patch Set 7 : Get element coordinates programmatically #
Total comments: 27
Patch Set 8 : Rebase #Patch Set 9 : Address comments #Patch Set 10 : Refactor into hasEarlyShowPress() #Messages
Total messages: 23 (0 generated)
Running through first, tests on the way.
As we discussed offline, we'll probably want a mechanism for triggering tap highlighting in this case as well. So perhaps this could be done most cleanly at the WebViewImpl layer - converting GestureTapDown into GestureShowPress events when WebViewImpl knows scrolling/zooming isn't possible (which would also avoid the need to extend ChromeClient). https://codereview.chromium.org/163933002/diff/1/Source/core/loader/EmptyClie... File Source/core/loader/EmptyClients.h (right): https://codereview.chromium.org/163933002/diff/1/Source/core/loader/EmptyClie... Source/core/loader/EmptyClients.h:176: virtual float maximumPageScaleFactor() const OVERRIDE { return 0; }; perhaps 1 would be a better default maximum (and probably the best default minimum too). Code may be confused if the only valid scale _factor_ is 0. https://codereview.chromium.org/163933002/diff/1/Source/core/page/ChromeClient.h File Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/163933002/diff/1/Source/core/page/ChromeClien... Source/core/page/ChromeClient.h:270: virtual float minimumPageScaleFactor() const = 0; did aelias@ suggest exposing both of these? I was expecting something more specific to pinch being enabled (eg. how does this interact with browser zoom on desktop which I believe uses page scale factor as well?). But I defer to aelias@ here. This is probably OK too. https://codereview.chromium.org/163933002/diff/1/Source/core/page/EventHandle... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/163933002/diff/1/Source/core/page/EventHandle... Source/core/page/EventHandler.cpp:60: #include "core/loader/EmptyClients.h" why do you need this new include? Seems odd... https://codereview.chromium.org/163933002/diff/1/Source/core/page/EventHandle... Source/core/page/EventHandler.cpp:2300: ChromeClient& chromeClient = mainFrame ? mainFrame->chromeClient() : *new EmptyChromeClient(); we should probably avoid creating a fake client (just use a different branch instead). Also you'd want stack allocation not heap allocation to avoid leaking. https://codereview.chromium.org/163933002/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/163933002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1113: hitType |= HitTestRequest::ReadOnly; Does this normally need to be non-readonly so that active gets triggered for the disambiguation pop-up? I had assumed showPress would trigger it in that case too (or if there's a race, perhaps we should be sending a show press explicitly before showing the disambiguation window). Why do you need to make it read only in this case?
https://codereview.chromium.org/163933002/diff/1/Source/core/page/ChromeClient.h File Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/163933002/diff/1/Source/core/page/ChromeClien... Source/core/page/ChromeClient.h:270: virtual float minimumPageScaleFactor() const = 0; On 2014/02/14 21:27:53, Rick Byers wrote: > did aelias@ suggest exposing both of these? I was expecting something more > specific to pinch being enabled (eg. how does this interact with browser zoom on > desktop which I believe uses page scale factor as well?). But I defer to > aelias@ here. This is probably OK too. I think these should instead be isPinchZoomable() (maybe with better name). It's not immediately clear that "pinch zoom disabled iff minimum == maximum" (i.e. pinch code could check the user-scale=no setting without consulting min/max). This would be more explicit. https://codereview.chromium.org/163933002/diff/1/Source/core/page/EventHandle... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/163933002/diff/1/Source/core/page/EventHandle... Source/core/page/EventHandler.cpp:2297: Page* page = m_frame->page(); Nit: I think this is involved enough that you could factor this out into a helper method. https://codereview.chromium.org/163933002/diff/1/Source/core/page/EventHandle... Source/core/page/EventHandler.cpp:2311: for (RenderLayer* parent = renderer ? renderer->enclosingLayer() : 0; parent && shouldTapDownTriggerActive; parent = parent->parent()) { I think this will break if your renderer is contained in an iframe; there may be a scrollable frame or renderer up the hierarchy, but parent() will end at the RenderView for the frame. I think you should walk up ScrollableArea::enclosingScrollableArea, but they're filled with FIXMEs to implement this case I just mentioned. See EventHandler::bubblingScroll for how to do this. If you're looking for brownie points, implement those ScrollableArea FIXMEs and use those. https://codereview.chromium.org/163933002/diff/1/Source/core/page/EventHandle... Source/core/page/EventHandler.cpp:2313: shouldTapDownTriggerActive = false; You can 'break' out of the loop here
Refactored most of it into WebViewImpl, walking up both the Layer and Frame tree as David suggested. Spent quite some time figuring out why viewport wasn't working in content_shell, patch to enable that is in CQ, working on tests now.
Added tests.
This seems like a much cleaner approach, good work! I'd re-word the CL description to focus on the gestures at a high-level. active states is just one possible application of GestureShowPress (I'd list both active and link highlighting as examples). Also you should probably mention the refactoring you've done in the CL description. bokan: please review the scroll-detection logic as well, I suspect some more work is needed there. https://codereview.chromium.org/163933002/diff/290001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-down-pinch-no-scroll.html (right): https://codereview.chromium.org/163933002/diff/290001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-pinch-no-scroll.html:78: /* Verify behaviour when page is pinchable */ does this really need to be a separate test (there's a lot of duplication between the two files)? Can't you just check this case in your main test before adding the viewport tag? https://codereview.chromium.org/163933002/diff/290001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-no-pinch.html (right): https://codereview.chromium.org/163933002/diff/290001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-no-pinch.html:113: you should be sending a tapCancel (or tap) gesture after each gestureShowPress in order to have a valid gesture sequence (tdresser is going to start adding code to check for this). https://codereview.chromium.org/163933002/diff/290001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/163933002/diff/290001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2223: bool EventHandler::hasScrollableAncestor(const Node* node) const some special cases that may require more general logic and test cases: - <select> combo box - <input> text field that has overflowed - is <textarea> the same as other overflow:scroll elements? - a scrollable iframe without any other scrollable layers (I don't think the root layers will have scrollsOverflow set true, will it?) https://codereview.chromium.org/163933002/diff/290001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2227: for (RenderLayer* parent = renderer ? renderer->enclosingLayer() : 0; parent; parent = parent->parent()) { Did bokan@ say that walking the layer tree was sufficient in all cases? I thought a normal <div style=overflow:scroll> wouldn't necessarily create a layer... https://codereview.chromium.org/163933002/diff/290001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/163933002/diff/290001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:768: eventSwallowed = handleGestureEvent(showPressEvent); nit: 2 spaces https://codereview.chromium.org/163933002/diff/290001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:781: // Only activate on TapDown if page isn't scrollable or pinchable and A little more explanation is probably in order around this function. I'd say something like "GestureShowPress is normally delayed to avoid triggering on the start of scroll, but when scrolling and pinching aren't possible we can do the ShowPress early, immediately after TapDown". https://codereview.chromium.org/163933002/diff/290001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/163933002/diff/290001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:618: bool shouldGestureTapDownTriggerActive(const WebCore::PlatformGestureEvent&); Since this is no longer just about 'triggering active' (a concept WebViewImpl doesn't even really know about), you should rename this. Maybe something like 'shouldGestureTapDownTriggerShowPressImmediately', or less verbose 'earlyGestureShowPress'. https://codereview.chromium.org/163933002/diff/290001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:791: bool m_gestureTapDownTriggeredActive; maybe m_sentEarlyShowPress?
Will talk to Bokan about the scroll detection and update handling for specific controls. https://codereview.chromium.org/163933002/diff/290001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-down-pinch-no-scroll.html (right): https://codereview.chromium.org/163933002/diff/290001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-pinch-no-scroll.html:78: /* Verify behaviour when page is pinchable */ On 2014/02/21 03:35:52, Rick Byers wrote: > does this really need to be a separate test (there's a lot of duplication > between the two files)? Can't you just check this case in your main test before > adding the viewport tag? Not really, I was just skeptical that adding too many to a single page might cause things to overflow before we wanted scrolling, but I think we have just enough to cram everything into one. https://codereview.chromium.org/163933002/diff/290001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-no-pinch.html (right): https://codereview.chromium.org/163933002/diff/290001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-no-pinch.html:113: On 2014/02/21 03:35:52, Rick Byers wrote: > you should be sending a tapCancel (or tap) gesture after each gestureShowPress > in order to have a valid gesture sequence (tdresser is going to start adding > code to check for this). Yes, I was planning to do a sweep of the layout tests a bit later to update this, created an issue to track this: http://crbug.com/346128 https://codereview.chromium.org/163933002/diff/290001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/163933002/diff/290001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:768: eventSwallowed = handleGestureEvent(showPressEvent); On 2014/02/21 03:35:52, Rick Byers wrote: > nit: 2 spaces Oh wow, would have never caught this, Thanks. https://codereview.chromium.org/163933002/diff/290001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/163933002/diff/290001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:618: bool shouldGestureTapDownTriggerActive(const WebCore::PlatformGestureEvent&); On 2014/02/21 03:35:52, Rick Byers wrote: > Since this is no longer just about 'triggering active' (a concept WebViewImpl > doesn't even really know about), you should rename this. Maybe something like > 'shouldGestureTapDownTriggerShowPressImmediately', or less verbose > 'earlyGestureShowPress'. earlyGestureShowPress sounds good. I was using shouldGestureTapDownTriggerShowPress earlier but switched to active. Thinking about it, talking about ShowPress in WebView does make more sense, I'll add more comments and update description accordingly.
https://codereview.chromium.org/163933002/diff/290001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/163933002/diff/290001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2223: bool EventHandler::hasScrollableAncestor(const Node* node) const On 2014/02/21 03:35:52, Rick Byers wrote: > some special cases that may require more general logic and test cases: > - <select> combo box > - <input> text field that has overflowed > - is <textarea> the same as other overflow:scroll elements? > - a scrollable iframe without any other scrollable layers (I don't think the > root layers will have scrollsOverflow set true, will it?) > I'll also add list box (<select> with size < 5) to this list. I expect this to be problematic since it handles scrolling in a rather unique way (no RenderLayer, it itself is a ScrollableArea). https://codereview.chromium.org/163933002/diff/290001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2227: for (RenderLayer* parent = renderer ? renderer->enclosingLayer() : 0; parent; parent = parent->parent()) { On 2014/02/21 03:35:52, Rick Byers wrote: > Did bokan@ say that walking the layer tree was sufficient in all cases? I > thought a normal <div style=overflow:scroll> wouldn't necessarily create a > layer... I think (though I could be wrong) it'll always create a RenderLayer, just not a CompositedLayerMapping. That said, I'd still suggest iterating over RenderObjects rather than RenderLayers to avoid any surprises. In fact, it looks like you shouldn't have to do this yourself: have a look at RenderBox::canBeScrolledAndHasScrollableArea and RenderObject::enclosingScrollableBox(), looks like it does exactly this.
Changes: * Handle form elements / iframes correctly * Use m_earlyGestureShowPress * Combine tests for scroll / pinch states
LGTM modulo one note in tests https://codereview.chromium.org/163933002/diff/450001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html (right): https://codereview.chromium.org/163933002/diff/450001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:157: eventSender.gestureTapDown(50, 65); Get the coordinates of the element you're trying to tap down on programatically (i.e. combobox.offsetLeft()/Top()).
https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-pinch.html (right): https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-pinch.html:32: #scroller { nit: "scroller" suggests overflow:scroll to me. Maybe "spacer"? https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-pinch.html:122: eventSender.gestureTapCancel(box.x, box.y); can you test after this that it's no longer active but is still hover? https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-pinch.html:124: // Send a sequence to another element to clear current one without relying on tap This is just to clear hover (active is already clear), right? https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-pinch.html:164: debug("tapdown on element should not activate."); you repeat this basic set of checks often enough that I think you should factor it into a 'hasEarlyShowPress' function with returns true or false (or causes test failure if the result is unexpected). https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html (right): https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:110: function isBoxHoverActive(id) there's a bunch of copy-paste here from the other test (and I recognize this code from other tests too). Can you please factor it into common files (js and css) which the tests that use it reference? Otherwise making fixes/improvements to it is a PITA... https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:162: debug("combobox should always be activated on tapdown"); why? Is this something your CL changed, or has it always been this way? It's not clear to me if this is what we want or not. https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:164: eventSender.gestureShowPress(combobox.x, combobox.y); should you check it's state after showPress too? https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:171: debug("Overflow listbox should remain unchanged"); why? Does it change after showPress? https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:180: debug("Overflow textbox should remain unchanged"); because it supports scrolling? Does it go active on showPress still? https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:200: eventSender.gestureShowPress(frame.x, frame.y); We should have at least one test where doing a gestureTapDown inside an iframe DOES cause something to go active in it. This test would pass if iframe hit testing was completely broken... https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:200: eventSender.gestureShowPress(frame.x, frame.y); It'll be active after the showPress, right? https://codereview.chromium.org/163933002/diff/470001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/163933002/diff/470001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2669: case PlatformEvent::GesturePinchBegin: I know you're just moving code around here, but this division feels odd to me. Ideally we'd have a comment saying how we decided which should be handled in the current frame and which need adjustment down to the subframe. Eg. pinch should probably be handled in the same way as scroll (although I don't think we actually receive pinch events in WebCore). https://codereview.chromium.org/163933002/diff/470001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/163933002/diff/470001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:756: && earlyGestureShowPress(platformEvent)) { might as well move this up into the switch (since you already have a dedicated case for GestureTapDown).
https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-pinch.html (right): https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-pinch.html:32: #scroller { On 2014/03/13 01:40:05, Rick Byers wrote: > nit: "scroller" suggests overflow:scroll to me. Maybe "spacer"? Done. https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-pinch.html:122: eventSender.gestureTapCancel(box.x, box.y); On 2014/03/13 01:40:05, Rick Byers wrote: > can you test after this that it's no longer active but is still hover? Yes, it goes inactive but stays hovered. Already tested here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-scroll-pinch.html:124: // Send a sequence to another element to clear current one without relying on tap On 2014/03/13 01:40:05, Rick Byers wrote: > This is just to clear hover (active is already clear), right? Yep https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html (right): https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:110: function isBoxHoverActive(id) On 2014/03/13 01:40:06, Rick Byers wrote: > there's a bunch of copy-paste here from the other test (and I recognize this > code from other tests too). Can you please factor it into common files (js and > css) which the tests that use it reference? Otherwise making fixes/improvements > to it is a PITA... Done in separate CL: http://crrev.com/197923006 https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:162: debug("combobox should always be activated on tapdown"); On 2014/03/13 01:40:06, Rick Byers wrote: > why? Is this something your CL changed, or has it always been this way? It's > not clear to me if this is what we want or not. A <select> tag with just 1 selectable item will never be srollable so it should always activate on tapDown. The menu pops up after tap has been received and is a separate WebView so we shouldn't need to worry about it. Maybe I should use a different name for it? https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:164: eventSender.gestureShowPress(combobox.x, combobox.y); On 2014/03/13 01:40:06, Rick Byers wrote: > should you check it's state after showPress too? Yes, just to make sure it's maintained. https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:171: debug("Overflow listbox should remain unchanged"); On 2014/03/13 01:40:06, Rick Byers wrote: > why? Does it change after showPress? See below. https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:180: debug("Overflow textbox should remain unchanged"); On 2014/03/13 01:40:06, Rick Byers wrote: > because it supports scrolling? Does it go active on showPress still? Yes, see comment below. https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:200: eventSender.gestureShowPress(frame.x, frame.y); On 2014/03/13 01:40:06, Rick Byers wrote: > It'll be active after the showPress, right? That functionality is tested in gesture-tap-active-state-iframe.html I was thinking that this test should only cover cases where an element requires special handling (figuring out if it's scrollable in case of an iframe). If the rest of the behaviour is like a normal element, it should work as tested in gesture-tap-down-scroll-pinch.html and gesture-tap-active-state.html https://codereview.chromium.org/163933002/diff/470001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/163933002/diff/470001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2669: case PlatformEvent::GesturePinchBegin: On 2014/03/13 01:40:06, Rick Byers wrote: > I know you're just moving code around here, but this division feels odd to me. > Ideally we'd have a comment saying how we decided which should be handled in the > current frame and which need adjustment down to the subframe. Eg. pinch should > probably be handled in the same way as scroll (although I don't think we > actually receive pinch events in WebCore). I'm not quite sure why, I was hoping to dig into it when continuing work on http://crrev.com/130433009 as it will require modifying a good chunk of EventHandler::handleGestureEvent(). https://codereview.chromium.org/163933002/diff/470001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/163933002/diff/470001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:756: && earlyGestureShowPress(platformEvent)) { On 2014/03/13 01:40:06, Rick Byers wrote: > might as well move this up into the switch (since you already have a dedicated > case for GestureTapDown). Done, although I'm not sure if having a return inside the switch is desirable.
This looks good - just one outstanding question... https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html (right): https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:162: debug("combobox should always be activated on tapdown"); On 2014/03/24 21:37:43, Zeeshan Qureshi wrote: > On 2014/03/13 01:40:06, Rick Byers wrote: > > why? Is this something your CL changed, or has it always been this way? It's > > not clear to me if this is what we want or not. > > A <select> tag with just 1 selectable item will never be srollable so it should > always activate on tapDown. Why do you say "1 selectable item"? This combobox has many items, right? > The menu pops up after tap has been received and is > a separate WebView so we shouldn't need to worry about it. Maybe I should use a > different name for it? But if I touch on a <select> tag I can still scroll the page, eg: see http://jsbin.com/jihuz/1/edit https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:200: eventSender.gestureShowPress(frame.x, frame.y); On 2014/03/24 21:37:43, Zeeshan Qureshi wrote: > On 2014/03/13 01:40:06, Rick Byers wrote: > > It'll be active after the showPress, right? > > That functionality is tested in gesture-tap-active-state-iframe.html > > I was thinking that this test should only cover cases where an element requires > special handling (figuring out if it's scrollable in case of an iframe). If the > rest of the behaviour is like a normal element, it should work as tested in > gesture-tap-down-scroll-pinch.html and gesture-tap-active-state.html Makes sense, thanks! https://codereview.chromium.org/163933002/diff/470001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/163933002/diff/470001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2669: case PlatformEvent::GesturePinchBegin: On 2014/03/24 21:37:43, Zeeshan Qureshi wrote: > On 2014/03/13 01:40:06, Rick Byers wrote: > > I know you're just moving code around here, but this division feels odd to me. > > > Ideally we'd have a comment saying how we decided which should be handled in > the > > current frame and which need adjustment down to the subframe. Eg. pinch > should > > probably be handled in the same way as scroll (although I don't think we > > actually receive pinch events in WebCore). > > I'm not quite sure why, I was hoping to dig into it when continuing work on > http://crrev.com/130433009 as it will require modifying a good chunk of > EventHandler::handleGestureEvent(). Ok, deferring this to a future CL is fine. Thanks.
On 2014/03/25 19:46:46, Rick Byers wrote: > This looks good - just one outstanding question... > > https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... > File > LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html > (right): > > https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... > LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:162: > debug("combobox should always be activated on tapdown"); > On 2014/03/24 21:37:43, Zeeshan Qureshi wrote: > > On 2014/03/13 01:40:06, Rick Byers wrote: > > > why? Is this something your CL changed, or has it always been this way? > It's > > > not clear to me if this is what we want or not. > > > > A <select> tag with just 1 selectable item will never be srollable so it > should > > always activate on tapDown. > > Why do you say "1 selectable item"? This combobox has many items, right? There is a list of items but you can only select one of them right? If you can select multiple then you get a different control (see the test case for listbox) > > The menu pops up after tap has been received and is > > a separate WebView so we shouldn't need to worry about it. Maybe I should use > a > > different name for it? > > But if I touch on a <select> tag I can still scroll the page, eg: see > http://jsbin.com/jihuz/1/edit If the page is scrollable / pinchable then this optimization never kicks in. These test cases are under the assumption that scroll / pinch are disabled, in that case you want the list to pop up without the 150ms ShowPress delay.
On 2014/03/25 20:15:38, Zeeshan Qureshi wrote: > On 2014/03/25 19:46:46, Rick Byers wrote: > > This looks good - just one outstanding question... > > > > > https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... > > File > > LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html > > (right): > > > > > https://codereview.chromium.org/163933002/diff/470001/LayoutTests/fast/events... > > > LayoutTests/fast/events/touch/gesture/gesture-tap-down-special-elements.html:162: > > debug("combobox should always be activated on tapdown"); > > On 2014/03/24 21:37:43, Zeeshan Qureshi wrote: > > > On 2014/03/13 01:40:06, Rick Byers wrote: > > > > why? Is this something your CL changed, or has it always been this way? > > It's > > > > not clear to me if this is what we want or not. > > > > > > A <select> tag with just 1 selectable item will never be srollable so it > > should > > > always activate on tapDown. > > > > Why do you say "1 selectable item"? This combobox has many items, right? > > There is a list of items but you can only select one of them right? If you can > select multiple then you get a different control (see the test case for listbox) > > > > > The menu pops up after tap has been received and is > > > a separate WebView so we shouldn't need to worry about it. Maybe I should > use > > a > > > different name for it? > > > > But if I touch on a <select> tag I can still scroll the page, eg: see > > http://jsbin.com/jihuz/1/edit > > If the page is scrollable / pinchable then this optimization never kicks in. > These test cases are under the assumption that scroll / pinch are disabled, in > that case you want the list to pop up without the 150ms ShowPress delay. Ah sorry, I understand now. That makes sense. LGTM
+ojan for OWNERS
Hmm, what I don't like about this is the ignoring of the future ShowPress event. This kind of state machine can get flaky and it's better to have a single decision point. One alternative is to detect scrollability in the browser process, although that would require sending up an array of rectangles for scrollable sublayers and slow-scrolling rects in the CompositorFrameMetadata, which would also be somewhat complex and raise a performance concern. Is this change worth doing at all? Re: similar logic in the Android framework, adamp@ said "In practice, just about everything of note in an Android UI is in a scrolling container" so it didn't provide much value. This also applies to the web. They recently decided to just lower the timeout to 100ms instead.
On 2014/03/25 22:18:42, aelias wrote: > Hmm, what I don't like about this is the ignoring of the future ShowPress event. > This kind of state machine can get flaky and it's better to have a single > decision point. Yeah I agree. I am a little worried about the maintainability here. > One alternative is to detect scrollability in the browser process, although that > would require sending up an array of rectangles for scrollable sublayers and > slow-scrolling rects in the CompositorFrameMetadata, which would also be > somewhat complex and raise a performance concern. Ugh, that seems like a lot of extra complexity just for this. If we decided we wanted it for other reasons then we could extend it for this. > Is this change worth doing at all? Re: similar logic in the Android framework, > adamp@ said "In practice, just about everything of note in an Android UI is in a > scrolling container" so it didn't provide much value. This also applies to the > web. They recently decided to just lower the timeout to 100ms instead. Thanks for that context, that's really helpful. I forwarded you a thread from some internal google developers arguing that this was something Android did better than the web (with claims that appear inconsistent with what you've said above). I agree we should be applying a similar philosophy for both Android and web here. One option may be to remove the ShowPress gesture from the browser gesture detector entirely (or, if we really want it for Aura, at least prevent it from being sent to the renderer). Then rather than attempt to un-delay it here in WebView, we could put all logic for creating it in the first place (delayed or not) here.
On 2014/03/25 22:29:01, Rick Byers wrote: > One option may be to remove the ShowPress gesture from the browser gesture > detector entirely (or, if we really want it for Aura, at least prevent it from > being sent to the renderer). Then rather than attempt to un-delay it here in > WebView, we could put all logic for creating it in the first place (delayed or > not) here. The difficulty with that is that the main thread doesn't always hear about a ScrollBegin. We could add a new message type called CancelShowPressState I suppose.
On 2014/03/25 22:55:49, aelias wrote: > On 2014/03/25 22:29:01, Rick Byers wrote: > > One option may be to remove the ShowPress gesture from the browser gesture > > detector entirely (or, if we really want it for Aura, at least prevent it from > > being sent to the renderer). Then rather than attempt to un-delay it here in > > WebView, we could put all logic for creating it in the first place (delayed or > > not) here. > > The difficulty with that is that the main thread doesn't always hear about a > ScrollBegin. We could add a new message type called CancelShowPressState I > suppose. We'd still send GestureTapCancel on ScrollBegin as we do today (TapCancel is intended to terminate both TapDown and ShowPress).
On 2014/03/25 22:55:49, aelias wrote: > The difficulty with that is that the main thread doesn't always hear about a > ScrollBegin. We could add a new message type called CancelShowPressState I > suppose. The main problem with this is that this would be prone to flashing a link highlight if the Blink main thread is hung (TapDown at 0ms, Blink main thread hangs, CancelShowPress sent at 50ms, Blink frees up at 300ms, then creates the link highlight and then cancels it). I don't know if there's some trick we could use to avoid this issue.
Do we have some real world sites for which we know this will enrich the experience?
On 2014/03/25 23:04:29, aelias wrote: > The main problem with this is that this would be prone to flashing a link > highlight if the Blink main thread is hung (TapDown at 0ms, Blink main thread > hangs, CancelShowPress sent at 50ms, Blink frees up at 300ms, then creates the > link highlight and then cancels it). I don't know if there's some trick we > could use to avoid this issue. Good point. We'd probably not actually see the highlight in most cases because it would be shown and cleared in the same frame (see http://crbug.com/312798) but still the occasional flicker here would be unfortunate. On 2014/03/25 23:13:40, jdduke wrote: > Do we have some real world sites for which we know this will enrich the > experience? There are lots of 'appy' sites that have touch effects on things that aren't scrollable or pinchable (eg. mobile gmail), but those sorts of sites tend to care enough about a fast response to their buttons that they implement them using touch events (that's the only thing that works reliably and consistently across all browsers anyhow). I'm not personally aware of any sites that rely heavily on :active today and are mobile-optimized, but this bug started with a request/complaint from an engineer on Google Maps who felt the web was behind android here. The argument is that we should be providing reliable and performant higher-level APIs that are agnostic to input type so developers can code to them rather than specific types of input (and then future input types and accessibility scenarios just work instead of requiring more input-type-specific code). But perhaps that's a weak enough argument that it's not worth the debate at this point. We could instead tackle this as a larger effort to improve/rationalize/extend our high-level device-agnostic input APIs (eg. I think we should have 'onactive' events for JS too). |
