Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10790019:
Add gesture target fuzzing to views (Closed)
|
Created:
8 years, 5 months ago by tdanderson Modified:
7 years ago CC:
chromium-reviews, tfarina, ben+watch_chromium.org, sky, kevers Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd gesture target fuzzing to views
This CL appears to make gesture targeting in views significantly easier.
Currently RootView::OnGestureEvent selects a tap target by using only the center point of the touch, which can lead to annoying unsuccessful tap gestures when the user attempts to
target small views (e.g. closing a tab, bookmark star, opening a tab, etc.) This CL uses the bounding box information stored in a gesture event which is used for more accurate targeting in the following way:
- Traverse the views tree looking for views that are overlapped by the touch region by at least 70%.
- If there is at least one overlapped view: Among all such overlapped views, pick the one that is closest to the center of
the touch region using a distance-to-center-line metric. This metric removes
the bias of wide/tall rectangles to give a better notion of which view is closest
to the center of the touch region. Set |gesture_handler_| to be this view.
- Otherwise: Set |gesture_handler_| to be the return value of GetEventHandlerForPoint.
Note that the bookmark star still seems slightly hard to touch at certain times. This is because some taps will fuzz to the 3px-wide resizer view located to the right of the bookmark star. To fix this, I can adjust the algorithm to try targeting all overlapped views in descending order of their distance to the touch center until the event is handled. I will address this in a separate CL if we feel that it is needed.
BUG=129794
TEST=GestureEventWithFuzzing and GetEventHandlerForRect in view_unittest.cc
Patch Set 1 #
Total comments: 35
Patch Set 2 : New approach using GetEventHandlerForRect #
Total comments: 19
Patch Set 3 : Fuzzing performed on ET_GESTURE_BEGIN #
Total comments: 6
Patch Set 4 : sadrul's comments addressed #Patch Set 5 : Patch with unit tests #
Total comments: 2
Patch Set 6 : sadrul's comments about testing addressed #
Total comments: 16
Patch Set 7 : sky's comments addressed, some further feedback needed #Patch Set 8 : No longer using screen coordinates #
Total comments: 11
Messages
Total messages: 38 (0 generated)
http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc File ui/views/view.cc (right): http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc#newcode90 ui/views/view.cc:90: } I think this is fine ... but perhaps you could use rect.size().GetArea() http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc#newcode100 ui/views/view.cc:100: int DistanceSquaredFromCenterLineToPoint(const gfx::Point& point, document http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc#newcode106 ui/views/view.cc:106: if (targetRect.width() > targetRect.height()) braces http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc#newcode1316 ui/views/view.cc:1316: gfx::Rect childRect(child->GetScreenBounds()); child_rect etc. Why ScreenBounds? http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc#newcode1337 ui/views/view.cc:1337: } indent is off. http://codereview.chromium.org/10790019/diff/1/ui/views/view.h File ui/views/view.h (right): http://codereview.chromium.org/10790019/diff/1/ui/views/view.h#newcode1025 ui/views/view.h:1025: // empty if there are no such candidates. I can't say I understand this comment very well ... perhaps could use some rewording. http://codereview.chromium.org/10790019/diff/1/ui/views/view.h#newcode1026 ui/views/view.h:1026: virtual void FindClosestOverlappedRect(const gfx::Rect& touchRect, touch_rect etc. (chrome doesn't use camelCase). You want closest_overlapped_rect to either be a Rect* or return gfx::Rect from the function. Chrome doesn't use non-const reference. http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc File ui/views/widget/root_view.cc (right): http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc#n... ui/views/widget/root_view.cc:464: ConvertPointToScreen(this, &adjustedLoc); Why is it necessary to convert to screen coordinate? Explain in comment. http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc#n... ui/views/widget/root_view.cc:477: if (gesture_handler_ && !useFuzzing) { Hm... I think with this change here, we can end up sending some gesture events (e.g. GESTURE_BEGIN, GESTURE_TAP_DOWN) to a view, and then retarget the gesture events based on the fuzzing, and send subsequent gestures (GESTURE_TAP, GESTURE_END) to a different view. That can lead to incorrect behaviour (some views, for example buttons, do process TAP_DOWN event). So we need to be very clear and careful on how we change the target, e.g. perhaps we need to send a GESTURE_END to an existing gesture_handler_, and then a sequence of GESTURE_BEGIN, GESTURE_TAP_DOWN and GESTURE_TAP to the new target (the target may very well expect these events before a TAP).
And I think the targeting logic should have unit tests. http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc File ui/views/view.cc (right): http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc#newcode106 ui/views/view.cc:106: if (targetRect.width() > targetRect.height()) On 2012/07/16 22:16:14, sadrul wrote: > braces Note how WebKit != Chrome. :-) http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc#newcode1308 ui/views/view.cc:1308: void View::FindClosestOverlappedRect(const gfx::Rect& touchRect, This routine could conceivably explore many views. Should this be a concern? If not noted elsewhere, return the best closestOverlappedRect, no non-const refs in Chrome. http://codereview.chromium.org/10790019/diff/1/ui/views/view_constants.h File ui/views/view_constants.h (right): http://codereview.chromium.org/10790019/diff/1/ui/views/view_constants.h#newc... ui/views/view_constants.h:28: VIEWS_EXPORT extern const float kFuzzingOverlapPercentage; So... If we have a really huge button, can we fail to hit it? http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc File ui/views/widget/root_view.cc (right): http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc#n... ui/views/widget/root_view.cc:464: ConvertPointToScreen(this, &adjustedLoc); On 2012/07/16 22:16:14, sadrul wrote: > Why is it necessary to convert to screen coordinate? Explain in comment. Aside: are screen coords in DIP or physical?
https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.cc#newco... ui/views/view.cc:90: } On 2012/07/16 22:16:14, sadrul wrote: > I think this is fine ... but perhaps you could use rect.size().GetArea() Done. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.cc#newco... ui/views/view.cc:100: int DistanceSquaredFromCenterLineToPoint(const gfx::Point& point, On 2012/07/16 22:16:14, sadrul wrote: > document Done. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.cc#newco... ui/views/view.cc:106: if (targetRect.width() > targetRect.height()) On 2012/07/16 23:35:48, rjkroege wrote: > On 2012/07/16 22:16:14, sadrul wrote: > > braces > > Note how WebKit != Chrome. :-) Done. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.cc#newco... ui/views/view.cc:106: if (targetRect.width() > targetRect.height()) On 2012/07/16 22:16:14, sadrul wrote: > braces Done. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.cc#newco... ui/views/view.cc:1308: void View::FindClosestOverlappedRect(const gfx::Rect& touchRect, On 2012/07/16 23:35:48, rjkroege wrote: > This routine could conceivably explore many views. Should this be a concern? I tried to cut down the number of views explored as much as possible by only making a recursive call on the child if the touch rect intersects the child (otherwise this is avoided at line 1317). > If not noted elsewhere, return the best closestOverlappedRect, no non-const refs > in Chrome. Done. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.cc#newco... ui/views/view.cc:1316: gfx::Rect childRect(child->GetScreenBounds()); On 2012/07/16 22:16:14, sadrul wrote: > child_rect etc. > > Why ScreenBounds? It seems easiest to have everything in a common coordinate system. If closest_overlapped_rect is a pointer, then using screen bounds to describe this rect would be necessary since we could be in different coordinate systems at different places in the call stack. If I changed this to a function that returned a rect instead of passing in a pointer, it seems there would be a larger amount of overhead involved with converting to/from child/parent coordinate systems at each recursive call as compared to keeping everything in screen coordinates. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.cc#newco... ui/views/view.cc:1337: } On 2012/07/16 22:16:14, sadrul wrote: > indent is off. Done. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.h File ui/views/view.h (right): https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.h#newcod... ui/views/view.h:1025: // empty if there are no such candidates. On 2012/07/16 22:16:14, sadrul wrote: > I can't say I understand this comment very well ... perhaps could use some > rewording. Done. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.h#newcod... ui/views/view.h:1026: virtual void FindClosestOverlappedRect(const gfx::Rect& touchRect, On 2012/07/16 22:16:14, sadrul wrote: > touch_rect etc. (chrome doesn't use camelCase). > > You want closest_overlapped_rect to either be a Rect* or return gfx::Rect from > the function. Chrome doesn't use non-const reference. Done. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view_constants.h File ui/views/view_constants.h (right): https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view_constant... ui/views/view_constants.h:28: VIEWS_EXPORT extern const float kFuzzingOverlapPercentage; On 2012/07/16 23:35:48, rjkroege wrote: > So... If we have a really huge button, can we fail to hit it? This is a good question, and I should have mentioned this in the CL description. The benefit of fuzzing is for small targets only, and by "small targets" I mean targets that your finger can cover by at least 70%. The algorithm first tries to dispatch the tap event to the closest small target; if there are none or if the target doesn't handle the tap, then the event is dispatched to the center of the touch region (which is what the code did originally). So in the case of a large view such as a tab, assuming that the user's finger did not sufficiently cover the tab close button, there should be no problem hitting it because it is large enough for the user to touch without the help of fuzzing (i.e., it is easy to touch a tab so that the center of the touch region is contained within the tab). https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... File ui/views/widget/root_view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... ui/views/widget/root_view.cc:464: ConvertPointToScreen(this, &adjustedLoc); On 2012/07/16 22:16:14, sadrul wrote: > Why is it necessary to convert to screen coordinate? Explain in comment. Please see my reply in view.cc and let me know if you agree. If we end up using screen coordinates, I will add a comment in the code. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... ui/views/widget/root_view.cc:477: if (gesture_handler_ && !useFuzzing) { On 2012/07/16 22:16:14, sadrul wrote: > Hm... > > I think with this change here, we can end up sending some gesture events (e.g. > GESTURE_BEGIN, GESTURE_TAP_DOWN) to a view, and then retarget the gesture events > based on the fuzzing, and send subsequent gestures (GESTURE_TAP, GESTURE_END) to > a different view. That can lead to incorrect behaviour (some views, for example > buttons, do process TAP_DOWN event). So we need to be very clear and careful on > how we change the target, e.g. perhaps we need to send a GESTURE_END to an > existing gesture_handler_, and then a sequence of GESTURE_BEGIN, > GESTURE_TAP_DOWN and GESTURE_TAP to the new target (the target may very well > expect these events before a TAP). Yes, I see the problem here. But if the fuzzing logic re-targets and we send a GESTURE_END to the original target and GESTURE_BEGIN and GESTURE_TAP_DOWN, wouldn't this incorrectly process two gesture sequences when we only wanted one? A GESTURE_TAP_DOWN would be dispatched on the original target and a GESTURE_TAP would be dispatched on the new target. There are probably several problems with another approach that comes to mind, but what if we always applied the fuzzing logic for a GESTURE_TAP_DOWN? This way we're not processing extra gesture sequences and more gestures would automatically get the benefit of fuzzing.
http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc File ui/views/view.cc (right): http://codereview.chromium.org/10790019/diff/1/ui/views/view.cc#newcode1316 ui/views/view.cc:1316: gfx::Rect childRect(child->GetScreenBounds()); On 2012/07/17 19:07:07, tdanderson wrote: > On 2012/07/16 22:16:14, sadrul wrote: > > child_rect etc. > > > > Why ScreenBounds? > > It seems easiest to have everything in a common coordinate system. > > If closest_overlapped_rect is a pointer, then using screen bounds to describe > this rect would be necessary since we could be in different coordinate systems > at different places in the call stack. > > If I changed this to a function that returned a rect instead of passing in a > pointer, it seems there would be a larger amount of overhead involved with > converting to/from child/parent coordinate systems at each recursive call as > compared to keeping everything in screen coordinates. This sounds reasonable. Make sure the doc for the function in view.h mentions that the rects are in screen-coord. http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc File ui/views/widget/root_view.cc (right): http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc#n... ui/views/widget/root_view.cc:477: if (gesture_handler_ && !useFuzzing) { On 2012/07/17 19:07:07, tdanderson wrote: > On 2012/07/16 22:16:14, sadrul wrote: > > Hm... > > > > I think with this change here, we can end up sending some gesture events (e.g. > > GESTURE_BEGIN, GESTURE_TAP_DOWN) to a view, and then retarget the gesture > events > > based on the fuzzing, and send subsequent gestures (GESTURE_TAP, GESTURE_END) > to > > a different view. That can lead to incorrect behaviour (some views, for > example > > buttons, do process TAP_DOWN event). So we need to be very clear and careful > on > > how we change the target, e.g. perhaps we need to send a GESTURE_END to an > > existing gesture_handler_, and then a sequence of GESTURE_BEGIN, > > GESTURE_TAP_DOWN and GESTURE_TAP to the new target (the target may very well > > expect these events before a TAP). > > Yes, I see the problem here. But if the fuzzing logic re-targets and we send a > GESTURE_END to the original target and GESTURE_BEGIN and GESTURE_TAP_DOWN, > wouldn't this incorrectly process two gesture sequences when we only wanted one? Yes. But I do not think we can avoid this (apart from delaying sending BEGIN/TAP_DOWN until TAP happens so that we know the target for sure, but that's a big no-no). Once we send a view a BEGIN and TAP_DOWN, we need to also send it END. And before we send a TAP to another target, we need to send it a BEGIN and TAP_DOWN to make sure it is receiving an expected and well-defined sequence of events. > A GESTURE_TAP_DOWN would be dispatched on the original target and a GESTURE_TAP > would be dispatched on the new target. This would be unexpected. We should not do this. > There are probably several problems with another approach that comes to mind, > but what if we always applied the fuzzing logic for a GESTURE_TAP_DOWN? TAP_DOWN happens in response to a single TOUCH_DOWN event, i.e. it will have the radius from the touch-event, but the fuzzing here won't be as effective. > This way > we're not processing extra gesture sequences and more gestures would > automatically get the benefit of fuzzing.
I'm fine with doing something like this as it makes a lot of sense. That said, I don't think I get the implementation as it currently stands. I think we want something like a GetEventHandlerForRect() that is used in picking the gesture_handler_ and walking from there. This patch doesn't seem to do that though. http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc File ui/views/widget/root_view.cc (right): http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc#n... ui/views/widget/root_view.cc:459: bool useFuzzing = event.type() == ui::ET_GESTURE_TAP; Why do this here? Shouldn't we fuzz only if there is no gesture_handler_? http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc#n... ui/views/widget/root_view.cc:473: e.location_ = newLoc; What is |e|?
Also, we may need to further scrutinize event handlers. With this patch it's more likely to get a tap outside the bounds of the view. -Scott On Tue, Jul 17, 2012 at 12:54 PM, <sky@chromium.org> wrote: > I'm fine with doing something like this as it makes a lot of sense. > > That said, I don't think I get the implementation as it currently stands. I > think we want something like a GetEventHandlerForRect() that is used in > picking > the gesture_handler_ and walking from there. This patch doesn't seem to do > that > though. > > > > http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc > File ui/views/widget/root_view.cc (right): > > http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc#n... > ui/views/widget/root_view.cc:459: bool useFuzzing = event.type() == > ui::ET_GESTURE_TAP; > Why do this here? Shouldn't we fuzz only if there is no > gesture_handler_? > > http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc#n... > ui/views/widget/root_view.cc:473: e.location_ = newLoc; > What is |e|? > > http://codereview.chromium.org/10790019/
I have uploaded a new CL for your comments. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/view.cc#newco... ui/views/view.cc:1316: gfx::Rect childRect(child->GetScreenBounds()); On 2012/07/17 19:26:45, sadrul wrote: > On 2012/07/17 19:07:07, tdanderson wrote: > > On 2012/07/16 22:16:14, sadrul wrote: > > > child_rect etc. > > > > > > Why ScreenBounds? > > > > It seems easiest to have everything in a common coordinate system. > > > > If closest_overlapped_rect is a pointer, then using screen bounds to describe > > this rect would be necessary since we could be in different coordinate systems > > at different places in the call stack. > > > > If I changed this to a function that returned a rect instead of passing in a > > pointer, it seems there would be a larger amount of overhead involved with > > converting to/from child/parent coordinate systems at each recursive call as > > compared to keeping everything in screen coordinates. > > This sounds reasonable. Make sure the doc for the function in view.h mentions > that the rects are in screen-coord. Done. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... File ui/views/widget/root_view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... ui/views/widget/root_view.cc:459: bool useFuzzing = event.type() == ui::ET_GESTURE_TAP; On 2012/07/17 19:54:32, sky wrote: > Why do this here? Shouldn't we fuzz only if there is no gesture_handler_? There could be a |gesture_handler_| from either GESTURE_BEGIN or GESTURE_TAP_DOWN but this would be based only on the center of the touch region. We want to fuzz to possibly update |gesture_handler_| to something better. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... ui/views/widget/root_view.cc:464: ConvertPointToScreen(this, &adjustedLoc); On 2012/07/17 19:07:07, tdanderson wrote: > On 2012/07/16 22:16:14, sadrul wrote: > > Why is it necessary to convert to screen coordinate? Explain in comment. > > Please see my reply in view.cc and let me know if you agree. If we end up using > screen coordinates, I will add a comment in the code. Done. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... ui/views/widget/root_view.cc:473: e.location_ = newLoc; On 2012/07/17 19:54:32, sky wrote: > What is |e|? This was present before I touched the code. It is a non-const copy of |event| and does not appear to serve any purpose (replacing |e| with |event| does not seem to change the behavior of the code in any way). I'll remove it. https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... ui/views/widget/root_view.cc:477: if (gesture_handler_ && !useFuzzing) { On 2012/07/17 19:26:45, sadrul wrote: > On 2012/07/17 19:07:07, tdanderson wrote: > > On 2012/07/16 22:16:14, sadrul wrote: > > > Hm... > > > > > > I think with this change here, we can end up sending some gesture events > (e.g. > > > GESTURE_BEGIN, GESTURE_TAP_DOWN) to a view, and then retarget the gesture > > events > > > based on the fuzzing, and send subsequent gestures (GESTURE_TAP, > GESTURE_END) > > to > > > a different view. That can lead to incorrect behaviour (some views, for > > example > > > buttons, do process TAP_DOWN event). So we need to be very clear and careful > > on > > > how we change the target, e.g. perhaps we need to send a GESTURE_END to an > > > existing gesture_handler_, and then a sequence of GESTURE_BEGIN, > > > GESTURE_TAP_DOWN and GESTURE_TAP to the new target (the target may very well > > > expect these events before a TAP). > > > > Yes, I see the problem here. But if the fuzzing logic re-targets and we send a > > GESTURE_END to the original target and GESTURE_BEGIN and GESTURE_TAP_DOWN, > > wouldn't this incorrectly process two gesture sequences when we only wanted > one? > > Yes. But I do not think we can avoid this (apart from delaying sending > BEGIN/TAP_DOWN until TAP happens so that we know the target for sure, but that's > a big no-no). Once we send a view a BEGIN and TAP_DOWN, we need to also send it > END. And before we send a TAP to another target, we need to send it a BEGIN and > TAP_DOWN to make sure it is receiving an expected and well-defined sequence of > events. > This is the approach I have taken in my latest CL. > > A GESTURE_TAP_DOWN would be dispatched on the original target and a > GESTURE_TAP > > would be dispatched on the new target. > > This would be unexpected. We should not do this. > > > There are probably several problems with another approach that comes to mind, > > but what if we always applied the fuzzing logic for a GESTURE_TAP_DOWN? > > TAP_DOWN happens in response to a single TOUCH_DOWN event, i.e. it will have the > radius from the touch-event, but the fuzzing here won't be as effective. > > > This way > > we're not processing extra gesture sequences and more gestures would > > automatically get the benefit of fuzzing. https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/widget/roo... File ui/views/widget/root_view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/widget/roo... ui/views/widget/root_view.cc:488: GestureEvent begin_event(ui::ET_GESTURE_BEGIN, sadrul@: you mentioned that I should make sure the details of |begin_event| and |end_event| both have one touch point. Are there any other changes that are needed?
On 2012/07/17 19:54:32, sky wrote: > I'm fine with doing something like this as it makes a lot of sense. > > That said, I don't think I get the implementation as it currently stands. I > think we want something like a GetEventHandlerForRect() that is used in picking > the gesture_handler_ and walking from there. This patch doesn't seem to do that > though. I have tried to address this in Patch set 2. > > http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc > File ui/views/widget/root_view.cc (right): > > http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc#n... > ui/views/widget/root_view.cc:459: bool useFuzzing = event.type() == > ui::ET_GESTURE_TAP; > Why do this here? Shouldn't we fuzz only if there is no gesture_handler_? > > http://codereview.chromium.org/10790019/diff/1/ui/views/widget/root_view.cc#n... > ui/views/widget/root_view.cc:473: e.location_ = newLoc; > What is |e|?
http://codereview.chromium.org/10790019/diff/7001/ui/views/view.cc File ui/views/view.cc (right): http://codereview.chromium.org/10790019/diff/7001/ui/views/view.cc#newcode811 ui/views/view.cc:811: return NULL; I think as long as the intersection is non-empty, this should return |this|. Otherwise, if the View is big without any child views, it will end up returning NULL http://codereview.chromium.org/10790019/diff/7001/ui/views/view.cc#newcode827 ui/views/view.cc:827: if (!closest_view) braces (since else has them) http://codereview.chromium.org/10790019/diff/7001/ui/views/view.cc#newcode841 ui/views/view.cc:841: closest_view = cur_view; Can you remember best_dist_so_far instead of computing it every time? http://codereview.chromium.org/10790019/diff/7001/ui/views/widget/root_view.cc File ui/views/widget/root_view.cc (left): http://codereview.chromium.org/10790019/diff/7001/ui/views/widget/root_view.c... ui/views/widget/root_view.cc:455: GestureEvent e(event, this); This creates an event to take RootView's transforms (if any) into account. This may not be needed anymore, but if so, then let's remove this (and similar cases for mouse and touch events) in a separate CL. http://codereview.chromium.org/10790019/diff/7001/ui/views/widget/root_view.cc File ui/views/widget/root_view.cc (right): http://codereview.chromium.org/10790019/diff/7001/ui/views/widget/root_view.c... ui/views/widget/root_view.cc:473: if (!gesture_handler_) brace http://codereview.chromium.org/10790019/diff/7001/ui/views/widget/root_view.c... ui/views/widget/root_view.cc:488: GestureEvent begin_event(ui::ET_GESTURE_BEGIN, On 2012/07/18 22:35:42, tdanderson wrote: > sadrul@: you mentioned that I should make sure the details of |begin_event| and > |end_event| both have one touch point. Are there any other changes that are > needed? Yes. You need to update GestureEvent::details_ to make sure touch_points() returns 1 http://codereview.chromium.org/10790019/diff/7001/ui/views/widget/root_view.c... ui/views/widget/root_view.cc:543: -
https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... File ui/views/widget/root_view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... ui/views/widget/root_view.cc:459: bool useFuzzing = event.type() == ui::ET_GESTURE_TAP; On 2012/07/18 22:35:42, tdanderson wrote: > On 2012/07/17 19:54:32, sky wrote: > > Why do this here? Shouldn't we fuzz only if there is no gesture_handler_? > > There could be a |gesture_handler_| from either GESTURE_BEGIN or > GESTURE_TAP_DOWN but this would be based only on the center of the touch region. > We want to fuzz to possibly update |gesture_handler_| to something better. Why? It seems wrong to me to change the target view midstream. I could see us targeting multiple views for different touch points (as we do in the aura side), but that isn't what you're doing here.
http://codereview.chromium.org/10790019/diff/7001/ui/views/view.cc File ui/views/view.cc (right): http://codereview.chromium.org/10790019/diff/7001/ui/views/view.cc#newcode100 ui/views/view.cc:100: // Returns the distance from |point| to the center line of |target_rect|. square of the distance -- comment not consistent with code. http://codereview.chromium.org/10790019/diff/7001/ui/views/view.h File ui/views/view.h (right): http://codereview.chromium.org/10790019/diff/7001/ui/views/view.h#newcode509 ui/views/view.h:509: // |touch_rect| is in screen coordinates. "screen" coordinates is imprecise. Do you mean physical coordinates? None of this code should be (afaik) operating in physical pixels, only high DIP.
On 2012/07/19 00:06:38, sky wrote: > https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... > File ui/views/widget/root_view.cc (right): > > https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... > ui/views/widget/root_view.cc:459: bool useFuzzing = event.type() == > ui::ET_GESTURE_TAP; > On 2012/07/18 22:35:42, tdanderson wrote: > > On 2012/07/17 19:54:32, sky wrote: > > > Why do this here? Shouldn't we fuzz only if there is no gesture_handler_? > > > > There could be a |gesture_handler_| from either GESTURE_BEGIN or > > GESTURE_TAP_DOWN but this would be based only on the center of the touch > region. > > We want to fuzz to possibly update |gesture_handler_| to something better. > > Why? It seems wrong to me to change the target view midstream. I could see us > targeting multiple views for different touch points (as we do in the aura side), > but that isn't what you're doing here. Indeed. The problem here is, the target for a single touch-point could change depending on the state of the touch. For example, when a user starts to touch the screen, on the first event, i.e. touch-start, the radius for the touch-point could be really small. Subsequent touch-move events, as the user presses on the screen firmly, the position and the radius of the touch-point will change (usually by a very small amount, but it does change). So, deciding on the target on touch-start, and sticking with it during the subsequent touch and gesture events, may not be good enough, and it may be necessary to adjust the target in the middle of the gesture-sequence. So that is the problem. The solution here is to not worry about touch-events, and only do fuzzing for specific gesture events. This CL does the fuzzing for TAP, and this will probably need to happen for LONG_PRESS too. It makes sure the gesture-events a view receives are predictable (by sending extra END, BEGIN/TAP_DOWN when necessary). However, if a view expects a touch-event to always precede a gesture-event, then this will break that. So if we go this way, then we have to be explicit that this is an incorrect assumption, and fix any code that currently depends on this assumption. However, it is a bit weird that when I tap on something, there could be two buttons that receive some gesture events. Another approach could be to do the fuzzing only for touch-start and gesture-begin (or tap-down?) (since touch-start always corresponds to a gesture-begin, the computed active-region for the event will be the same). This way, we can probably avoid retargetting in the middle of a gesture. (since the changes after touch-start are usually small, this will probably be acceptable). Thoughts?
On 2012/07/19 17:53:29, sadrul wrote: > On 2012/07/19 00:06:38, sky wrote: > > > https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... > > File ui/views/widget/root_view.cc (right): > > > > > https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... > > ui/views/widget/root_view.cc:459: bool useFuzzing = event.type() == > > ui::ET_GESTURE_TAP; > > On 2012/07/18 22:35:42, tdanderson wrote: > > > On 2012/07/17 19:54:32, sky wrote: > > > > Why do this here? Shouldn't we fuzz only if there is no gesture_handler_? > > > > > > There could be a |gesture_handler_| from either GESTURE_BEGIN or > > > GESTURE_TAP_DOWN but this would be based only on the center of the touch > > region. > > > We want to fuzz to possibly update |gesture_handler_| to something better. > > > > Why? It seems wrong to me to change the target view midstream. I could see us > > targeting multiple views for different touch points (as we do in the aura > side), > > but that isn't what you're doing here. > > Indeed. The problem here is, the target for a single touch-point could change > depending on the state of the touch. For example, when a user starts to touch > the screen, on the first event, i.e. touch-start, the radius for the touch-point > could be really small. Subsequent touch-move events, as the user presses on the > screen firmly, the position and the radius of the touch-point will change > (usually by a very small amount, but it does change). So, deciding on the target > on touch-start, and sticking with it during the subsequent touch and gesture > events, may not be good enough, and it may be necessary to adjust the target in > the middle of the gesture-sequence. > > So that is the problem. > > The solution here is to not worry about touch-events, and only do fuzzing for > specific gesture events. This CL does the fuzzing for TAP, and this will > probably need to happen for LONG_PRESS too. It makes sure the gesture-events a > view receives are predictable (by sending extra END, BEGIN/TAP_DOWN when > necessary). However, if a view expects a touch-event to always precede a > gesture-event, then this will break that. So if we go this way, then we have to > be explicit that this is an incorrect assumption, and fix any code that > currently depends on this assumption. However, it is a bit weird that when I tap > on something, there could be two buttons that receive some gesture events. I agree with Sadrul. Gestures should get fuzzed results. Touches should not. And we should try to implement the UI in terms of gestures and not use touches. Rob. > > Another approach could be to do the fuzzing only for touch-start and > gesture-begin (or tap-down?) (since touch-start always corresponds to a > gesture-begin, the computed active-region for the event will be the same). This > way, we can probably avoid retargetting in the middle of a gesture. (since the > changes after touch-start are usually small, this will probably be acceptable). > > Thoughts?
On Thu, Jul 19, 2012 at 10:53 AM, <sadrul@chromium.org> wrote: > On 2012/07/19 00:06:38, sky wrote: > > https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... >> >> File ui/views/widget/root_view.cc (right): > > > > https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... >> >> ui/views/widget/root_view.cc:459: bool useFuzzing = event.type() == >> ui::ET_GESTURE_TAP; >> On 2012/07/18 22:35:42, tdanderson wrote: >> > On 2012/07/17 19:54:32, sky wrote: >> > > Why do this here? Shouldn't we fuzz only if there is no >> > > gesture_handler_? >> > >> > There could be a |gesture_handler_| from either GESTURE_BEGIN or >> > GESTURE_TAP_DOWN but this would be based only on the center of the touch >> region. >> > We want to fuzz to possibly update |gesture_handler_| to something >> > better. > > >> Why? It seems wrong to me to change the target view midstream. I could see >> us >> targeting multiple views for different touch points (as we do in the aura > > side), >> >> but that isn't what you're doing here. > > > Indeed. The problem here is, the target for a single touch-point could > change > depending on the state of the touch. For example, when a user starts to > touch > the screen, on the first event, i.e. touch-start, the radius for the > touch-point > could be really small. Subsequent touch-move events, as the user presses on > the > screen firmly, the position and the radius of the touch-point will change > (usually by a very small amount, but it does change). So, deciding on the > target > on touch-start, and sticking with it during the subsequent touch and gesture > events, may not be good enough, and it may be necessary to adjust the target > in > the middle of the gesture-sequence. > > So that is the problem. > > The solution here is to not worry about touch-events, and only do fuzzing > for > specific gesture events. This CL does the fuzzing for TAP, and this will > probably need to happen for LONG_PRESS too. It makes sure the gesture-events > a > view receives are predictable (by sending extra END, BEGIN/TAP_DOWN when > necessary). Consider two buttons close to each other, A and B. The touch point targets A so we send A the initial sequence of events. This makes A look depressed. The point is held long enough so that a tap is triggered. We fuzz and think the user really wanted B. This results in A cancelling and B looking depressed. Isn't this going to be weird? That we're going through these hoops implies to me our UI does not have a good touch design. Most likely are buttons are too small and poorly positioned for touch. Is it possible this is the core issue we're running into? -Scott > However, if a view expects a touch-event to always precede a > gesture-event, then this will break that. So if we go this way, then we have > to > be explicit that this is an incorrect assumption, and fix any code that > currently depends on this assumption. However, it is a bit weird that when I > tap > on something, there could be two buttons that receive some gesture events. > > Another approach could be to do the fuzzing only for touch-start and > gesture-begin (or tap-down?) (since touch-start always corresponds to a > gesture-begin, the computed active-region for the event will be the same). > This > way, we can probably avoid retargetting in the middle of a gesture. (since > the > changes after touch-start are usually small, this will probably be > acceptable). > > Thoughts?
FWIW I spoke with one of the guys on the Android framework team and he said they don't do anything like this. -Scott
On 2012/07/19 20:15:18, sky wrote: > On Thu, Jul 19, 2012 at 10:53 AM, <mailto:sadrul@chromium.org> wrote: > > On 2012/07/19 00:06:38, sky wrote: > > > > > https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... > >> > >> File ui/views/widget/root_view.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10790019/diff/1/ui/views/widget/root_v... > >> > >> ui/views/widget/root_view.cc:459: bool useFuzzing = event.type() == > >> ui::ET_GESTURE_TAP; > >> On 2012/07/18 22:35:42, tdanderson wrote: > >> > On 2012/07/17 19:54:32, sky wrote: > >> > > Why do this here? Shouldn't we fuzz only if there is no > >> > > gesture_handler_? > >> > > >> > There could be a |gesture_handler_| from either GESTURE_BEGIN or > >> > GESTURE_TAP_DOWN but this would be based only on the center of the touch > >> region. > >> > We want to fuzz to possibly update |gesture_handler_| to something > >> > better. > > > > > >> Why? It seems wrong to me to change the target view midstream. I could see > >> us > >> targeting multiple views for different touch points (as we do in the aura > > > > side), > >> > >> but that isn't what you're doing here. > > > > > > Indeed. The problem here is, the target for a single touch-point could > > change > > depending on the state of the touch. For example, when a user starts to > > touch > > the screen, on the first event, i.e. touch-start, the radius for the > > touch-point > > could be really small. Subsequent touch-move events, as the user presses on > > the > > screen firmly, the position and the radius of the touch-point will change > > (usually by a very small amount, but it does change). So, deciding on the > > target > > on touch-start, and sticking with it during the subsequent touch and gesture > > events, may not be good enough, and it may be necessary to adjust the target > > in > > the middle of the gesture-sequence. > > > > So that is the problem. > > > > The solution here is to not worry about touch-events, and only do fuzzing > > for > > specific gesture events. This CL does the fuzzing for TAP, and this will > > probably need to happen for LONG_PRESS too. It makes sure the gesture-events > > a > > view receives are predictable (by sending extra END, BEGIN/TAP_DOWN when > > necessary). > > Consider two buttons close to each other, A and B. The touch point > targets A so we send A the initial sequence of events. This makes A > look depressed. The point is held long enough so that a tap is > triggered. We fuzz and think the user really wanted B. This results in > A cancelling and B looking depressed. Isn't this going to be weird? > > That we're going through these hoops implies to me our UI does not > have a good touch design. Most likely are buttons are too small and > poorly positioned for touch. Is it possible this is the core issue > we're running into? Yes. There would be no need for this feature if the buttons were big enough and sufficiently spaced. I think of this feature as a helper for UX: it shouldn't be necessary in views (as opposed to say web contents where we must do our best with content known to not be well designed) but it lets us more successfully operate a "less touchy" visual design. That being said: I think the situation that you describe should be equivalent to what would happen if we had narrowly spaced buttons and tried to use them with the mouse: * mouse down targets a specific target A * hand motion jiggles the mouse and it leaves the target A and enters B * mouse up fires B. In a fuzzing context (where even quite big items are "close"), we should have: * tapdown gesture on A -> shows A's depressed state * finger motion changes the best target to B. * the gesture end event to A releases A's depressed state * tapdown on B -> shows' B's depressed state * tap on B -> fires B Rob. > > -Scott > > > However, if a view expects a touch-event to always precede a > > gesture-event, then this will break that. So if we go this way, then we have > > to > > be explicit that this is an incorrect assumption, and fix any code that > > currently depends on this assumption. However, it is a bit weird that when I > > tap > > on something, there could be two buttons that receive some gesture events. > > > > Another approach could be to do the fuzzing only for touch-start and > > gesture-begin (or tap-down?) (since touch-start always corresponds to a > > gesture-begin, the computed active-region for the event will be the same). > > This > > way, we can probably avoid retargetting in the middle of a gesture. (since > > the > > changes after touch-start are usually small, this will probably be > > acceptable). > > > > Thoughts?
https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/view.cc File ui/views/view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/view.cc#ne... ui/views/view.cc:100: // Returns the distance from |point| to the center line of |target_rect|. On 2012/07/19 00:51:57, rjkroege wrote: > square of the distance -- comment not consistent with code. Done. https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/view.cc#ne... ui/views/view.cc:811: return NULL; On 2012/07/18 23:42:31, sadrul wrote: > I think as long as the intersection is non-empty, this should return |this|. > Otherwise, if the View is big without any child views, it will end up returning > NULL This is the behavior that I want since only small targets get the benefit of fuzzing. If GetEventHandlerForRect returns NULL, it means that there are no small targets sufficiently near the center of the touch region and so you probably meant to target something larger. In this case the for loop at the end of the function is entered which performs targeting based only on the center of the touch region (and you should have no problem making the center of the touch region fall within the bounds of a big target). https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/view.cc#ne... ui/views/view.cc:827: if (!closest_view) On 2012/07/18 23:42:31, sadrul wrote: > braces (since else has them) Done. https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/view.cc#ne... ui/views/view.cc:841: closest_view = cur_view; On 2012/07/18 23:42:31, sadrul wrote: > Can you remember best_dist_so_far instead of computing it every time? Done. https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/view.h File ui/views/view.h (right): https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/view.h#new... ui/views/view.h:509: // |touch_rect| is in screen coordinates. On 2012/07/19 00:51:57, rjkroege wrote: > "screen" coordinates is imprecise. Do you mean physical coordinates? None of > this code should be (afaik) operating in physical pixels, only high DIP. I used the term "screen" to be consistent with the function names ConvertPointToScreen and GetBoundsInScreen. https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/widget/roo... File ui/views/widget/root_view.cc (left): https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/widget/roo... ui/views/widget/root_view.cc:455: GestureEvent e(event, this); On 2012/07/18 23:42:31, sadrul wrote: > This creates an event to take RootView's transforms (if any) into account. This > may not be needed anymore, but if so, then let's remove this (and similar cases > for mouse and touch events) in a separate CL. Okay, I will put |e| back in for this CL. https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/widget/roo... File ui/views/widget/root_view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/widget/roo... ui/views/widget/root_view.cc:473: if (!gesture_handler_) On 2012/07/18 23:42:31, sadrul wrote: > brace Done. https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/widget/roo... ui/views/widget/root_view.cc:488: GestureEvent begin_event(ui::ET_GESTURE_BEGIN, On 2012/07/18 23:42:31, sadrul wrote: > On 2012/07/18 22:35:42, tdanderson wrote: > > sadrul@: you mentioned that I should make sure the details of |begin_event| > and > > |end_event| both have one touch point. Are there any other changes that are > > needed? > > Yes. You need to update GestureEvent::details_ to make sure touch_points() > returns 1 This will no longer be relevant in the next CL. https://chromiumcodereview.appspot.com/10790019/diff/7001/ui/views/widget/roo... ui/views/widget/root_view.cc:543: On 2012/07/18 23:42:31, sadrul wrote: > - Done.
I have uploaded a third patch set for your comments.
I think this is looking good. (and there should be tests before it lands) http://codereview.chromium.org/10790019/diff/16001/ui/views/view.cc File ui/views/view.cc (right): http://codereview.chromium.org/10790019/diff/16001/ui/views/view.cc#newcode811 ui/views/view.cc:811: return NULL; I still think as long as the touch_rect intersection with bounds is non-empty, it should return |this| here. http://codereview.chromium.org/10790019/diff/16001/ui/views/widget/root_view.cc File ui/views/widget/root_view.cc (right): http://codereview.chromium.org/10790019/diff/16001/ui/views/widget/root_view.... ui/views/widget/root_view.cc:458: if (!gesture_handler_) { This is not the right place. The event will not bubble up if you do this here. This code should be after line 509. Instead of using GetEventHandlerForPoint in the loop, it will use GetEventHandlerForRect http://codereview.chromium.org/10790019/diff/16001/ui/views/widget/root_view.... ui/views/widget/root_view.cc:466: gesture_handler_ = GetEventHandlerForRect(touch_rect); You could perhaps use bounding_box.set_origin here instead of creating touch_rect
https://chromiumcodereview.appspot.com/10790019/diff/16001/ui/views/view.cc File ui/views/view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/16001/ui/views/view.cc#n... ui/views/view.cc:811: return NULL; On 2012/07/26 18:41:28, sadrul wrote: > I still think as long as the touch_rect intersection with bounds is non-empty, > it should return |this| here. We talked about this offline, but for the benefit of the other reviewers I will summarize the outcome of our discussion. This function only considers small targets (targets that can be covered by your finger by at least ~70%), whereas GetEventHandlerForPoint considers all targets containing a single point regardless of size. The approach (which is clearer to see in the next CL) to finding a gesture handler in RootView::OnGestureEvent is to first consider small targets using GetEventHandlerForRect since these are hardest to touch. If this call returns something, then it's likely what the user wanted to touch. If the call to GetEventHandlerForRect in RootView::OnGestureEvent returns NULL, we then try to find a gesture handler by calling GetEventHandlerForPoint (which is what this code did originally). This will handle the case where the user is trying to touch a large target such as a tab. Large targets don't need fuzzing since it should be easy to precisely target a view that has a larger area than your finger. https://chromiumcodereview.appspot.com/10790019/diff/16001/ui/views/widget/ro... File ui/views/widget/root_view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/16001/ui/views/widget/ro... ui/views/widget/root_view.cc:458: if (!gesture_handler_) { On 2012/07/26 18:41:28, sadrul wrote: > This is not the right place. The event will not bubble up if you do this here. > This code should be after line 509. Instead of using GetEventHandlerForPoint in > the loop, it will use GetEventHandlerForRect Done. https://chromiumcodereview.appspot.com/10790019/diff/16001/ui/views/widget/ro... ui/views/widget/root_view.cc:466: gesture_handler_ = GetEventHandlerForRect(touch_rect); On 2012/07/26 18:41:28, sadrul wrote: > You could perhaps use bounding_box.set_origin here instead of creating > touch_rect Done.
Patch set 4 up for your comments. In the meanwhile I will create unit tests.
This change looks good to me. I think this can land once you add the tests.
Patch set 5 is up now and has unit tests. Please let me know if you see any other issues, otherwise this should be good to land.
The test looks good. One comment: http://codereview.chromium.org/10790019/diff/26001/ui/views/events/event.h File ui/views/events/event.h (right): http://codereview.chromium.org/10790019/diff/26001/ui/views/events/event.h#ne... ui/views/events/event.h:444: private: Instead of adding the constructors, you can add a protected GestureEvent::set_details() and a corresponding GestureEventForTest::set_details
New patch up: sadrul's comments about testing have been addressed. Please let me know if this gets an LGTM or if you have additional comments. https://chromiumcodereview.appspot.com/10790019/diff/26001/ui/views/events/ev... File ui/views/events/event.h (right): https://chromiumcodereview.appspot.com/10790019/diff/26001/ui/views/events/ev... ui/views/events/event.h:444: private: On 2012/08/01 16:05:33, sadrul wrote: > Instead of adding the constructors, you can add a protected > GestureEvent::set_details() and a corresponding GestureEventForTest::set_details Done.
lgtm
LGTM We might have to do additional work for views with hit-test masks, but let's do that after this.
I'll look at this first thing tomorrow. I still strongly believe this functionality is an indication that our UI is not designed for touch and this is just a stopgap. I would like to see a switch to enable this. The default is off, but for chromeos we pass in the right switch to enable it. -Scott On Wed, Aug 1, 2012 at 11:33 AM, <sadrul@chromium.org> wrote: > LGTM > > We might have to do additional work for views with hit-test masks, but let's > do > that after this. > > https://chromiumcodereview.appspot.com/10790019/
Ben convinced me no switch, and this is just a result of having a ui that we're trying to make work for both touch and mouse. I'll make time tomorrow morning to review this. -Scott On Wed, Aug 1, 2012 at 1:00 PM, Scott Violet <sky@chromium.org> wrote: > I'll look at this first thing tomorrow. > > I still strongly believe this functionality is an indication that our > UI is not designed for touch and this is just a stopgap. I would like > to see a switch to enable this. The default is off, but for chromeos > we pass in the right switch to enable it. > > -Scott > > On Wed, Aug 1, 2012 at 11:33 AM, <sadrul@chromium.org> wrote: >> LGTM >> >> We might have to do additional work for views with hit-test masks, but let's >> do >> that after this. >> >> https://chromiumcodereview.appspot.com/10790019/
Please update the subject/description of this cl.
On 2012/08/02 15:23:44, sky wrote: > Please update the subject/description of this cl. Done.
You're going to need to update all views that override GetEventhandleForPoint to override GetEventHandlerForRect too. http://codereview.chromium.org/10790019/diff/25003/ui/views/events/event.h File ui/views/events/event.h (right): http://codereview.chromium.org/10790019/diff/25003/ui/views/events/event.h#ne... ui/views/events/event.h:440: void set_details(ui::GestureEventDetails details) { details_ = details; } const ui::GestureEventDetails& here and 458. Can you nuke this and instead make GestureEventForTest a friend? http://codereview.chromium.org/10790019/diff/25003/ui/views/view.cc File ui/views/view.cc (right): http://codereview.chromium.org/10790019/diff/25003/ui/views/view.cc#newcode89 ui/views/view.cc:89: return intersection_area / rect_1_area; Is it possible for rect_a_area to be 0? http://codereview.chromium.org/10790019/diff/25003/ui/views/view.cc#newcode92 ui/views/view.cc:92: int DistanceToInterval(int pos, int start, int end) { Add description. http://codereview.chromium.org/10790019/diff/25003/ui/views/view.cc#newcode806 ui/views/view.cc:806: if (!child_count()) { The structure of this should roughly match that of GetEventHandlerForPoint. This block should be moved near the end and happen as long as closest_view is NULL and it doesn't matter what child_count is. http://codereview.chromium.org/10790019/diff/25003/ui/views/view.h File ui/views/view.h (right): http://codereview.chromium.org/10790019/diff/25003/ui/views/view.h#newcode503 ui/views/view.h:503: virtual View* GetEventHandlerForPoint(const gfx::Point& point); Add a comment to this that if overriding GetEventHandlerForPoint you'll need to override this GetEventHandlerForRect. http://codereview.chromium.org/10790019/diff/25003/ui/views/view.h#newcode507 ui/views/view.h:507: // |touch_rect|. Among these rectangles, choose one that is closest to the touch_rect -> rect Why is the rect in screen coordinates? It should be location coordinates as is GetEventHandleForPoint and most other methods. http://codereview.chromium.org/10790019/diff/25003/ui/views/view_constants.h File ui/views/view_constants.h (right): http://codereview.chromium.org/10790019/diff/25003/ui/views/view_constants.h#... ui/views/view_constants.h:27: // the view's bounding rectangle. Note that the views tests named Remove last sentence. http://codereview.chromium.org/10790019/diff/25003/ui/views/widget/root_view.cc File ui/views/widget/root_view.cc (right): http://codereview.chromium.org/10790019/diff/25003/ui/views/widget/root_view.... ui/views/widget/root_view.cc:506: for (gesture_handler_ = v ? v : GetEventHandlerForPoint(e.location()); Seems wierd to fallback to GetEventHandlerForPoint. Can't we make GetEventHandlerForRect always return a non-NULL?
sky@'s comments addressed, some still needing additional feedback. I am also uploading patch set 7 now. https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/events/ev... File ui/views/events/event.h (right): https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/events/ev... ui/views/events/event.h:440: void set_details(ui::GestureEventDetails details) { details_ = details; } On 2012/08/02 15:52:08, sky wrote: > const ui::GestureEventDetails& here and 458. > > Can you nuke this and instead make GestureEventForTest a friend? Done. https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc File ui/views/view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc#n... ui/views/view.cc:89: return intersection_area / rect_1_area; On 2012/08/02 15:52:08, sky wrote: > Is it possible for rect_a_area to be 0? I don't believe this is possible given our implementation. But it is possible others may wish to call this function in the future, so I have added in a check for this. https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc#n... ui/views/view.cc:92: int DistanceToInterval(int pos, int start, int end) { On 2012/08/02 15:52:08, sky wrote: > Add description. Done. https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc#n... ui/views/view.cc:806: if (!child_count()) { On 2012/08/02 15:52:08, sky wrote: > The structure of this should roughly match that of GetEventHandlerForPoint. This > block should be moved near the end and happen as long as closest_view is NULL > and it doesn't matter what child_count is. Done. https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.h File ui/views/view.h (right): https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.h#ne... ui/views/view.h:503: virtual View* GetEventHandlerForPoint(const gfx::Point& point); On 2012/08/02 15:52:08, sky wrote: > Add a comment to this that if overriding GetEventHandlerForPoint you'll need to > override this GetEventHandlerForRect. Done. https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.h#ne... ui/views/view.h:507: // |touch_rect|. Among these rectangles, choose one that is closest to the On 2012/08/02 15:52:08, sky wrote: > touch_rect -> rect Done. > Why is the rect in screen coordinates? It should be location coordinates as is > GetEventHandleForPoint and most other methods. I decided to keep everything in screen coordinates because GetEventHandlerForRect makes calls to PercentCoveredBy and DistanceSquaredFromCenterLineToPoint, which both require a common coordinate system for their parameters. This way seems to require less work in switching back and forth between different coordinate systems. https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view_cons... File ui/views/view_constants.h (right): https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view_cons... ui/views/view_constants.h:27: // the view's bounding rectangle. Note that the views tests named On 2012/08/02 15:52:08, sky wrote: > Remove last sentence. Done. https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/widget/ro... File ui/views/widget/root_view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/widget/ro... ui/views/widget/root_view.cc:506: for (gesture_handler_ = v ? v : GetEventHandlerForPoint(e.location()); On 2012/08/02 15:52:08, sky wrote: > Seems wierd to fallback to GetEventHandlerForPoint. Can't we make > GetEventHandlerForRect always return a non-NULL? The fallback is the intended behavior since GetEventHandlerForRect only considers candidates with the >= 70% overlap. If there are none, then we try to dispatch to the center point of the touch region (switching tabs would fall under this case). How would you feel about the following change (suggested by sadrul to me in person last week)? Rename GetEventHandlerForRect to GetEventHandlerForRectInternal and make this private. Then add a GetEventHandlerForRect that takes in a rect and a point |p|. It calls GetEventHandlerForRectInternal and if this returns NULL, we return GetEventHandlerForPoint(p) instead? This will have the same behavior but guarantees a non-NULL return value for GetEventHandlerForRect.
On Thu, Aug 2, 2012 at 11:11 AM, <tdanderson@chromium.org> wrote: > sky@'s comments addressed, some still needing additional feedback. I am also > uploading patch set 7 now. > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/events/ev... > File ui/views/events/event.h (right): > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/events/ev... > > ui/views/events/event.h:440: void set_details(ui::GestureEventDetails > details) { details_ = details; } > On 2012/08/02 15:52:08, sky wrote: >> >> const ui::GestureEventDetails& here and 458. > > >> Can you nuke this and instead make GestureEventForTest a friend? > > > Done. > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc > File ui/views/view.cc (right): > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc#n... > > ui/views/view.cc:89: return intersection_area / rect_1_area; > On 2012/08/02 15:52:08, sky wrote: >> >> Is it possible for rect_a_area to be 0? > > > I don't believe this is possible given our implementation. But it is > possible others may wish to call this function in the future, so I have > added in a check for this. > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc#n... > > ui/views/view.cc:92: int DistanceToInterval(int pos, int start, int end) > { > On 2012/08/02 15:52:08, sky wrote: >> >> Add description. > > > Done. > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc#n... > ui/views/view.cc:806: if (!child_count()) { > > On 2012/08/02 15:52:08, sky wrote: >> >> The structure of this should roughly match that of > > GetEventHandlerForPoint. This >> >> block should be moved near the end and happen as long as closest_view > > is NULL >> >> and it doesn't matter what child_count is. > > > Done. > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.h > File ui/views/view.h (right): > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.h#ne... > > ui/views/view.h:503: virtual View* GetEventHandlerForPoint(const > gfx::Point& point); > On 2012/08/02 15:52:08, sky wrote: >> >> Add a comment to this that if overriding GetEventHandlerForPoint > > you'll need to >> >> override this GetEventHandlerForRect. > > > Done. > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.h#ne... > > ui/views/view.h:507: // |touch_rect|. Among these rectangles, choose one > that is closest to the > On 2012/08/02 15:52:08, sky wrote: >> >> touch_rect -> rect > > > Done. > > >> Why is the rect in screen coordinates? It should be location > > coordinates as is >> >> GetEventHandleForPoint and most other methods. > > > I decided to keep everything in screen coordinates because > GetEventHandlerForRect makes calls to PercentCoveredBy and > DistanceSquaredFromCenterLineToPoint, which both require a common > coordinate system for their parameters. This way seems to require less > work in switching back and forth between different coordinate systems. I would rather have consistency. Make it in the coordinates of the view like nearly all other methods in view. > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view_cons... > File ui/views/view_constants.h (right): > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view_cons... > > ui/views/view_constants.h:27: // the view's bounding rectangle. Note > that the views tests named > On 2012/08/02 15:52:08, sky wrote: >> >> Remove last sentence. > > > Done. > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/widget/ro... > File ui/views/widget/root_view.cc (right): > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/widget/ro... > > ui/views/widget/root_view.cc:506: for (gesture_handler_ = v ? v : > GetEventHandlerForPoint(e.location()); > On 2012/08/02 15:52:08, sky wrote: >> >> Seems wierd to fallback to GetEventHandlerForPoint. Can't we make >> GetEventHandlerForRect always return a non-NULL? > > > The fallback is the intended behavior since GetEventHandlerForRect only > considers candidates with the >= 70% overlap. If there are none, then we > try to dispatch to the center point of the touch region (switching tabs > would fall under this case). > > How would you feel about the following change (suggested by sadrul to me > in person last week)? Rename GetEventHandlerForRect to > GetEventHandlerForRectInternal and make this private. Then add a > GetEventHandlerForRect that takes in a rect and a point |p|. It calls > GetEventHandlerForRectInternal and if this returns NULL, we return > GetEventHandlerForPoint(p) instead? This will have the same behavior but > guarantees a non-NULL return value for GetEventHandlerForRect. > > https://chromiumcodereview.appspot.com/10790019/ Have you looked at the places that subclass GetEventHandleForPoint? They are going to need to override GetEventHandlerForRect and I suspect in a completely different way than you have here. Here's a problem with the patch. This doesn't deal with visible bounds in anyway. It assumes the bounds of the view are entirely visible, which isn't necessarily the case. -Scott
On 2012/08/02 20:02:22, sky wrote: > On Thu, Aug 2, 2012 at 11:11 AM, <mailto:tdanderson@chromium.org> wrote: > > sky@'s comments addressed, some still needing additional feedback. I am also > > uploading patch set 7 now. > > > > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/events/ev... > > File ui/views/events/event.h (right): > > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/events/ev... > > > > ui/views/events/event.h:440: void set_details(ui::GestureEventDetails > > details) { details_ = details; } > > On 2012/08/02 15:52:08, sky wrote: > >> > >> const ui::GestureEventDetails& here and 458. > > > > > >> Can you nuke this and instead make GestureEventForTest a friend? > > > > > > Done. > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc > > File ui/views/view.cc (right): > > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc#n... > > > > ui/views/view.cc:89: return intersection_area / rect_1_area; > > On 2012/08/02 15:52:08, sky wrote: > >> > >> Is it possible for rect_a_area to be 0? > > > > > > I don't believe this is possible given our implementation. But it is > > possible others may wish to call this function in the future, so I have > > added in a check for this. > > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc#n... > > > > ui/views/view.cc:92: int DistanceToInterval(int pos, int start, int end) > > { > > On 2012/08/02 15:52:08, sky wrote: > >> > >> Add description. > > > > > > Done. > > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.cc#n... > > ui/views/view.cc:806: if (!child_count()) { > > > > On 2012/08/02 15:52:08, sky wrote: > >> > >> The structure of this should roughly match that of > > > > GetEventHandlerForPoint. This > >> > >> block should be moved near the end and happen as long as closest_view > > > > is NULL > >> > >> and it doesn't matter what child_count is. > > > > > > Done. > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.h > > File ui/views/view.h (right): > > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.h#ne... > > > > ui/views/view.h:503: virtual View* GetEventHandlerForPoint(const > > gfx::Point& point); > > On 2012/08/02 15:52:08, sky wrote: > >> > >> Add a comment to this that if overriding GetEventHandlerForPoint > > > > you'll need to > >> > >> override this GetEventHandlerForRect. > > > > > > Done. > > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view.h#ne... > > > > ui/views/view.h:507: // |touch_rect|. Among these rectangles, choose one > > that is closest to the > > On 2012/08/02 15:52:08, sky wrote: > >> > >> touch_rect -> rect > > > > > > Done. > > > > > >> Why is the rect in screen coordinates? It should be location > > > > coordinates as is > >> > >> GetEventHandleForPoint and most other methods. > > > > > > I decided to keep everything in screen coordinates because > > GetEventHandlerForRect makes calls to PercentCoveredBy and > > DistanceSquaredFromCenterLineToPoint, which both require a common > > coordinate system for their parameters. This way seems to require less > > work in switching back and forth between different coordinate systems. > > I would rather have consistency. Make it in the coordinates of the > view like nearly all other methods in view. This change will be present in my next patch. > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view_cons... > > File ui/views/view_constants.h (right): > > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/view_cons... > > > > ui/views/view_constants.h:27: // the view's bounding rectangle. Note > > that the views tests named > > On 2012/08/02 15:52:08, sky wrote: > >> > >> Remove last sentence. > > > > > > Done. > > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/widget/ro... > > File ui/views/widget/root_view.cc (right): > > > > > https://chromiumcodereview.appspot.com/10790019/diff/25003/ui/views/widget/ro... > > > > ui/views/widget/root_view.cc:506: for (gesture_handler_ = v ? v : > > GetEventHandlerForPoint(e.location()); > > On 2012/08/02 15:52:08, sky wrote: > >> > >> Seems wierd to fallback to GetEventHandlerForPoint. Can't we make > >> GetEventHandlerForRect always return a non-NULL? > > > > > > The fallback is the intended behavior since GetEventHandlerForRect only > > considers candidates with the >= 70% overlap. If there are none, then we > > try to dispatch to the center point of the touch region (switching tabs > > would fall under this case). > > > > How would you feel about the following change (suggested by sadrul to me > > in person last week)? Rename GetEventHandlerForRect to > > GetEventHandlerForRectInternal and make this private. Then add a > > GetEventHandlerForRect that takes in a rect and a point |p|. It calls > > GetEventHandlerForRectInternal and if this returns NULL, we return > > GetEventHandlerForPoint(p) instead? This will have the same behavior but > > guarantees a non-NULL return value for GetEventHandlerForRect. > > > > https://chromiumcodereview.appspot.com/10790019/ > > Have you looked at the places that subclass GetEventHandleForPoint? > They are going to need to override GetEventHandlerForRect and I > suspect in a completely different way than you have here. I have filed https://code.google.com/p/chromium/issues/detail?id=140582. As discussed today, we could possibly land this patch with a flag to have it off by default and then turn this flag on once the overridden View::GetEventHandlerForRect functions have been implemented. > > Here's a problem with the patch. This doesn't deal with visible bounds > in anyway. It assumes the bounds of the view are entirely visible, > which isn't necessarily the case. > > -Scott
My latest changes are uploaded now. sky, could you please clarify the changes you suggested over IM regarding the return value of GetEventHandlerForRect?
Here's what I was thinking. But, after writing this I think it's harder to understand than the two methods. I have one other thought. I'm going to nuke these comments and start over but am sending them out as is for posterity. https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc File ui/views/view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:816: View* View::GetEventHandlerForRect(const gfx::Rect& rect) { I'm hoping its possible to combine this and GetEventHandlerForPoint. Less error prone that way. I'm thinking something like this: struct EventHandlerResult { View* view_at_center; View* best_view; }; bool GetEventHandlerForRect(..., EventHandlerResult* result) { } https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:831: if (!cur_view) between 830-831 something like: if (rect.IsEmpty() && result->view_at_center) return true; cur_view becomes result->best_view. https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:837: int cur_dist = DistanceSquaredFromCenterLineToPoint(touch_center, If GetEventHandlerForRect returns a view, don't we end up calculating this n -1 times for the same view where n is the depth to the view that returned itself? https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:840: closest_view = cur_view; result->best_view = closest_view; https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:844: if (!result->view_at_center && bounds().contains(center_point)) result->view_at_center = this; https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:846: return closest_view; This would become return true. https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:850: return this; result->best_view = this; return true; https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:851: return NULL; return false;
There would need to be some more changes, but you get the idea. I like this in that we only have one method for figuring out the best candidate. That said, there is a problem with how all of this is set up. It doesn't invoke HitTest at all. That'll cause problems. I suspect this means we need to change HitTest to take a rect, but I don't have time to investigate that. https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc File ui/views/view.cc (right): https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:816: View* View::GetEventHandlerForRect(const gfx::Rect& rect) { On 2012/08/03 23:46:36, sky wrote: > I'm hoping its possible to combine this and GetEventHandlerForPoint. Less error > prone that way. I'm thinking something like this: > > struct EventHandlerResult { > View* view_at_center; > View* best_view; > }; > > bool GetEventHandlerForRect(..., EventHandlerResult* result) { > } Gah, I guess I can't nuke my comments. Here's my second thought. This requires two passes, but combines the two methods into a single one: enum EventType { MOUSE, TOUCH }; name this GetEventHandler(const gfx::Rect& rect, EventType type); https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:831: if (!cur_view) On 2012/08/03 23:46:36, sky wrote: > between 830-831 something like: > > if (rect.IsEmpty() && result->view_at_center) > return true; > cur_view becomes result->best_view. if (type == MOUSE) return cur_view; https://chromiumcodereview.appspot.com/10790019/diff/25006/ui/views/view.cc#n... ui/views/view.cc:851: return NULL; On 2012/08/03 23:46:36, sky wrote: > return false; return type == MOUSE ? this : NULL; |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
