1078029 - The closest link is not always correctly detected using Nightly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dominique Vincent [:domivinc]]

When a touch occurs 5 mm near a link, the click on the link is triggered by Android firefox. This functionality works on Beta but it seems degraded on Nightly.
Bug 788073 moves the code from browser.js into the platform touch handling code (PositionedEventTargeting.cpp). The issue could come from this recent change.

I detected the issue using this page: http://office.microsoft.com/fr-fr/ (many links in the bottom area of the page). I tried to simplify the page to understand where is the issue. Now I’m using the following page to test the differences between Beta and Nightly: http://rusez.com/bb.html

In this last page, using Beta, when a touch occurs on the red area, 5 mm to the left of the yellow links, the click on the closest link is done (in this case you can see a green flash).
With the same page, using Nightly, it doesn’t work. It’s really hard to get the green flash. I have to move/zoom the page to different values in order to get the correct behavior. As soon as you move or zoom in/out from this position, the closest link is never detected.

I started to look into the new code. When the issue occurs, the function ClipToFrame returns an empty rect (width=0) here :
http://dxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp#254

The point  aPointRelativeToRootFrame is outside the frame. It looks like this point doesn’t take into account the zoom and/or the left offset of the page. Or the frame used is not the correct one.

My device: HTC Desire Z, Android 2.3.3

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Dominique, thanks for the detailed analysis and for digging into the code! That's very helpful.

Kan-ru, it looks like the ClipToFrame call was added as part of bug 950225, do you know if it is working as intended here? It certainly sounds like a bug. Note that Fennec switched to using this PositionedEventTargeting code as of bug 788073 so it's going to help flush out existing bugs in the code.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

It sounds like a bug. However the test page works in Flame so I suspect the bug is at somewhere else. A dump of the frame tree and the value of aPointRelativeToRootFrame would be helpful.

Comment 3

[Dominique Vincent [:domivinc]]

I traced the calculation of aPointRelativeToRootFrame .
In my test case, the calculation is done in nsLayoutUtils::GetEventCoordinatesRelativeTo (http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1907).
There is a view attached to the frame, so the calculation is done here:
return pt - view->ViewToWidgetOffset();
The offset of the view is not used in the calculation, it should probably be something like that:
return pt - view->ViewToWidgetOffset() - view->GetOffsetTo(nullptr);

In addition to this calculation issue, I noticed another difference between Beta and Nightly behavior. When the calculation is correct using Nightly, the closest link is found, the position of the touch event is set to the correct value (5 mm before the link) but the position of the click event (clientX) is set to the same value. In Beta, the clientX of the click event is always set to the first pixel of the link. It's the reason why we never see the green flash in my test case using Nightly. This second bug is linked to the new implementation done in Bug 788073.

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]:

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Dominique Vincent [:domivinc] from comment #3)
> In addition to this calculation issue, I noticed another difference between
> Beta and Nightly behavior. When the calculation is correct using Nightly,
> the closest link is found, the position of the touch event is set to the
> correct value (5 mm before the link) but the position of the click event
> (clientX) is set to the same value. In Beta, the clientX of the click event
> is always set to the first pixel of the link. It's the reason why we never
> see the green flash in my test case using Nightly. This second bug is linked
> to the new implementation done in Bug 788073.

I suspect what's going on here is that the PositionedEventTargeting code finds a new target element for the touch events but doesn't actually move the coordinates of the touch point. The click event dispatched by Gecko happens at the same coordinates as the touch event, so it's going to be where the user actually tapped (i.e. 5mm away). The target-detection for the click for some reason isn't going through PositionedEventTargeting and so the click event gets dispatched with a different target element than the touch event.

I think we do want the target-detection for the click to go through the PositionedEventTargeting code, because that click is based on touch-events and needs to be fuzzed the same way as the touch events. In theory it should detect the exact same target that the touch events did.

Can you try copying the two lines at [1] which set the ignoreRootScrollFrame and MOZ_SOURCE_TOUCH stuff into [2] and see if that solves the problem? My theory is that if you do that, the mousedown/mouseup events that are generated from the touch events (at [3]) should be routed through the PositionedEventTargeting code and the click should end up on the right target.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=f784b7a2d718#1047
[2] http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidJavaWrappers.cpp?rev=4bd1f068e83e#872
[3] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=f784b7a2d718#864

Comment 6

[Dominique Vincent [:domivinc]]

> Can you try copying the two lines at [1] which set the ignoreRootScrollFrame
> and MOZ_SOURCE_TOUCH stuff into [2] and see if that solves the problem? My
> theory is that if you do that, the mousedown/mouseup events that are
> generated from the touch events (at [3]) should be routed through the
> PositionedEventTargeting code and the click should end up on the right
> target.

1) It doesn't fix the issue. AndroidGeckoEvent::MakeMouseEvent is called but returns NS_EVENT_NULL. So the 2 new lines have no effect. The method is called with Action = AndroidMotionEvent::ACTION_DOWN and AndroidMotionEvent::ACTION_UP but both actions are not present in the switch ...

2) I added the missing cases like that:
            case AndroidMotionEvent::ACTION_DOWN:
                msg = NS_MOUSE_BUTTON_DOWN;
                break;
            case AndroidMotionEvent::ACTION_UP:
                msg =  NS_MOUSE_BUTTON_UP;
                break;
but it doesn't change anything. The click position is still the same of the touch position.

3) The event.refPoint calculation here:
 event.refPoint = LayoutDeviceIntPoint((Points()[0].x * scale.scale) - offset.x,
                                          (Points()[0].y * scale.scale) - offset.y);

should probably be changed. My guess is that the closest clickable frame position should be used?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Dominique Vincent [:domivinc] from comment #6)
> 1) It doesn't fix the issue. AndroidGeckoEvent::MakeMouseEvent is called but
> returns NS_EVENT_NULL. So the 2 new lines have no effect.

Ah, sorry, my mistake. I didn't look at the code very carefully and thought that's where the mousedown/mouseup events were coming from. I looked again and they are coming from [1]. The comment there agrees with what I expect to be happening.

I also took a closer look at your test page and I noticed that you're only setting the green color flash if the actual x-coordinate is different. Based on the code we have the behaviour I'm seeing on your test page is actually what I would expect. That is, the coordinates of the click reflect (clientX/screenX) the actual touch position that the user hit, and the target element of the click event reflects the closest clickable element.

Note that this means that:
(1) the coordinates of the click event might not actually be inside the bounding rect of the target element and (2) this behaviour is intentionally different from the behaviour we had before 788073. I at least did not expect this change to result in differences in user-visible behaviour, because it should be the target element of the click event that gets used when activating the link, and the coordinates being outside it shouldn't matter. In particular, on your test page, any tap on the red area should activate the nearest link and in fact that's what happens - I see the background of the link go black every time I tap, even if the red area doesn't turn green.

So I'm wondering now what was the original problem you were seeing on the microsoft office page - could you describe that in a bit more detail? Which link were you trying to click on that was behaving differently? If the click coordinates being different is causing a regression then we can look into moving the click coordinates but I hope that isn't necessary.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=597b7da7f843&mark=4903-4911#4909

Comment 8

[Dominique Vincent [:domivinc]]

> what I would expect. That is, the coordinates of the click reflect
> (clientX/screenX) the actual touch position that the user hit, and the
> target element of the click event reflects the closest clickable element.
> 
> Note that this means that:
> (1) the coordinates of the click event might not actually be inside the
> bounding rect of the target element and (2) this behaviour is intentionally
> different from the behaviour we had before 788073. I at least did not expect
> this change to result in differences in user-visible behaviour, because it
> should be the target element of the click event that gets used when
> activating the link, and the coordinates being outside it shouldn't matter.

1) I agree with you if the html page is very basic (links without jscript). We won't see any visual difference when the closest behavior works (see the second point).
But if you have jscript in the page, the change will probably break some web applications. The position returned in the event is outside the html element ... 
The previous behavior (Beta) is perfect from a front-end point of view: touch near links, click triggered on the closest link. All the event data positions are correct. With the Nightly behavior, it's hard to justify the behavior: touch near links, click outside the closest link, it's really strange for me ... You have a click on an element but the event reports a click position outside the element!
The Firefox documentation will have to be updated and the front-end developers must be informed about this change. 
Do you know if the W3C DOM event documentation gives some rules about the clientX/screenX values? 


> In particular, on your test page, any tap on the red area should activate
> the nearest link and in fact that's what happens - I see the background of
> the link go black every time I tap, even if the red area doesn't turn green.

2) Try to change the zoom (zoom out) and move the red area near the right side of your screen (the bug is more visible here): the closest link is never found. And you have the same issue on the Microsoft page with the links in the bottom of the page. Zoom out and move the links near the right side of the screen: the closest link is no more found.

Details: When you load the page (without changing the zoom), on my HTC, you can touch at 492px, the closest link is found (the link starts at 510 px). So it works fine, it's probably near 5 mm (18 px).
Zoom out and move the red area to the right side of the screen: you can touch at 509 px, and the closest link is still not found (just 1px after). You can verify that, you just have to trace the process and you will see that the closest area is empty because the touch position is totally wrong is this case.



Both issues seem for me important, and I'm not sure that they were in the scope of bug 788073. My understanding of 788073 is the following: same behavior with better performances. You should perhaps ask a feedback from members of the Android team using the mailing list to see what they think about point 1. If there is a mailing list with jscript front-end developers it could also be a good idea to get their feedback about point 1. I think that such a change is risky and it's better to have a large consensus before moving the change in a release version of Firefox.

Comment 9

[Dominique Vincent [:domivinc]]

Attachment: device-2014-10-08-212936.png

Screen copy of the page when the closest link is never found.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Dominique Vincent [:domivinc] from comment #8)
> The Firefox documentation will have to be updated and the front-end
> developers must be informed about this change. 
> Do you know if the W3C DOM event documentation gives some rules about the
> clientX/screenX values? 

From my reading of the spec, what we are doing is not disallowed. In fact, if you use the enter key or spacebar to activate a button and trigger the click event, you'll see that the clientX value is 0 regardless of where the button is placed. So what we are doing is consistent with that if nothing else. But yes, I agree that we should update documentation and ensure that front-end developers are aware of this.

> I think
> that such a change is risky and it's better to have a large consensus before
> moving the change in a release version of Firefox.

I agree that the change carries some risk. It will be some time before this change makes it out to a release build of Firefox, and we'll pay close attention to see if there any issues reported because of this.

Wes, smaug, do you guys have any further thoughts on this? To summarize:

Prior to bug 788073, Fennec would dispatch mousedown/mouseup/click events at coordinates that were different from the corresponding touch events. The mouse/click event coordinates were repositioned to be inside the bounding rect of the event target (which, due to fuzzing, may have been near but not directly under the touch point).

After bug 788073, Fennec dispatches mousedown/mouseup/click events with the same *target* as before, but the event coordinates (clientX etc.) match the coordinates of the touch events. This means any JS code that assumes the event coordinates are inside the bounding rect will fail. On the other hand, content JS will be better able to track the finger moving using mouse events, since the coordinates don't take into account the fuzzing for clickable elements.

I believe B2G's behaviour has pretty much always matched the new Fennec behaviour, because it dispatches the mouse/click events from [1] which use the touch event coordinates, and then PositionedEventTargeting changes the target element in the event.

Do you know if this behaviour is likely to break content in the wild, disagrees with other browsers, or violates specifications?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=40263f6c0fbc#1929

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Dominique Vincent [:domivinc] from comment #9)
> Created attachment 8501972 [details]
> device-2014-10-08-212936.png
> 
> Screen copy of the page when the closest link is never found.

Yeah in this configuration I can reproduce the links being difficult to click in Nightly while they click fine in Beta. For this we should look at adjusting code in PositionedEventTargeting.cpp to find the right target. Does the change you described in comment 3 fix this behaviour? If so then you could submit a patch for review for it.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Matt might have some feedback to this.

I don't know if the new behavior breaks some pages. Might not.

Someone should test what other mobile browsers do (I have access only to b2g browser).

Hit testing isn't spec'ed anywhere, so most of this stuff is undefined.
However the old behavior of clientX/Y sounds like a spec violation.
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#widl-MouseEvent-clientX
and the current Touch Events spec says 
"If the user agent interprets a sequence of touch events as a click, then it should dispatch mousemove, mousedown, mouseup, and click events (in that order) at the location of the touchend event for the corresponding touch input"
...but then, that really doesn't handle repositioning...

So I'd say this case isn't defined properly anywhere, and we need to figure out what other browsers do and
get this spec'ed somewhere.

Comment 13

[Matt Brubeck (:mbrubeck)]

Here's the old Android-specific code (pre bug 788073) to adjust the simulated mouse event coordinates to be inside the target element:
https://hg.mozilla.org/integration/mozilla-inbound/file/9c5e3e980339/mobile/android/chrome/content/browser.js#l3908

I think the best solution for this bug is to port this logic into the new cross-platform implementation (PositionedEventTargeting).  This would fix the test case here, and is generally in line with the goal of simulating mouse events for compatibility with legacy content on touch hardware.  As smaug said, there's no spec dealing with this retargeting; I don't know if that is for IP reasons or just that no one has written one yet.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I cooked up a test page [1] to test this behaviour in Chrome (37.0.2062.117) and Opera (24.0.1565.82529) on Android. Both of them behave similarly to Fennec pre-788073. That is, when I click near a button, the button does NOT get the touch events, but it does get mouse and click events, and the clientX/clientY on the mouse/click events have been adjusted to be inside the bounds of the button.

When I click near a button in Fennec post-788073, the button DOES get touch events, along with the mouse and click events, and for all of them the clientX/clientY have not been adjusted to be inside the bounds of the button.

Conceptually to me the new Fennec behaviour seems like the best because the coordinates reflect what actually happened, so if content wants the actual un-fuzzed location of the tap, they can get it. But at the same time the default behaviours will work as expected. However considering the existing behaviour in these browsers all seem to match up maybe it would be better to stick with that.

Also cc'ing Rick to see if he has any thoughts on this.

[1] http://people.mozilla.org/~kgupta/bug/1078029.html

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Brian, do you have cycles to look into this?

Comment 16

[Rick Byers]

Our thinking here in chromium is that touch events are a low level API, and as such we really don't want to mess with what we're getting from the OS/hardware.  From an extensible web manifesto perspective, we should get out of the way between the framework and OS.  The mouse events, however, are really built upon a higher level abstraction (gesture events in chromium) where we imply some meaning/semantics.

This argument would be more defensible if we actually provided all the primitives necessary for a framework to implement it's own touch adjustment algorithm.  We're close (eg. they've got the point, radius and hit-testing via elementFromPoint, but we don't expose a region-based hit-test API yet).

Comment 17

[Rick Byers]

Also, touch events may be used in a scenario where the app really doesn't want any sort of adjustment (eg. a painting app).  I personally wouldn't want to adjust touch events without an API to let applications disable it.

Comment 18

[Dominique Vincent [:domivinc]]

> Does the change you described in comment 3 fix this behaviour? If so then
> you could submit a patch for review for it.

No, it's a partial fix when a view is attached to the frame. If you test with the original Microsoft page links (right bottom of the page) you will see that the closest behavior has still some issues:
- current zoom value and horizontal scroll position is not taken into account in the calculation,
- links in child elements are not detected

Comment 19

[Brian Nicholson (:bnicholson)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> Brian, do you have cycles to look into this?

No, not really. I can take a look when I get some spare time, but I have other things in my queue now. Bug 788073 wasn't a high priority bug to begin with, so unless someone else wants to take this now, I can back out bug 788073 until I have time to look at this.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

I'd rather work through the issues than back it out, primarily because fixing up this code will help B2G. We have a real problem there with not enough bugs getting discovered early and the more we share code across platforms the more that situation will improve.

I'll try to get to this in the next week or two.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> When I click near a button in Fennec post-788073, the button DOES get touch
> events, along with the mouse and click events, and for all of them the
> clientX/clientY have not been adjusted to be inside the bounds of the button.

So for some reason I can't reproduce this behaviour any more, even on nightlies from around the time I made this comment. No idea what's going on, but it seems like the touch redirection just doesn't happen at all. Clicking near the button doesn't dispatch anything to the button.

Comment 22

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #21)
> So for some reason I can't reproduce this behaviour any more, even on
> nightlies from around the time I made this comment. No idea what's going on,
> but it seems like the touch redirection just doesn't happen at all. Clicking
> near the button doesn't dispatch anything to the button.

You are probably in one of the 2 following cases (in layout/base/PositionedEventTargeting.cpp):

1- the button is a child element and in "static nsIFrame* GetClosest" method only the parent is tested here:

if (!IsElementClickable(f)) {
       continue;
}

You button could be a children of f.


2- you are using the zoom and/or scroll the page. The result returned by "static nsRect
 ClipToFrame" is empty. The "aFrame->GetSize()" is not valid and doesn't take into account the scroll/zoom situation.

Take a look into this patch to get more details:
https://bug663803.bugzilla.mozilla.org/attachment.cgi?id=8504792

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Turned out to be neither of those, actually. On my test page the divs around the button were also getting considered "clickable" because I had added touch/mouse/click listeners on the body. Moving the listeners up to the window fixed that problem, and now I can reproduce the behaviour I was seeing before. I don't know why I didn't run into this before, maybe I modified the test page along the way somewhere. Anyway, working on this now to restore the behaviour as it was before bug 788073.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Disable retargeting of touch events on Fennec

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Add repositioning of mouse events on Fennec

Comment 26

[Kartikaya Gupta (email:[e-mail])]

These two patches seem to do the job. That is, they change Fennec's behaviour to match what is described as the "pre-788073 behaviour" in comment 14. Since this code is shared with B2G I added a new pref to control repositioning the event so that the coordinates are within the bounds of the target frame, and only enabled it on Fennec. That way the B2G behaviour should remain exactly as it was before (which is probably wrong, but we should fix that in a separate bug by lining up the B2G prefs with the Fennec ones).

There is one outstanding issue that I'm aware of which is that with these patches applied, clicking outside the button on my test page doesn't set the button as active and so it doesn't flash red. I think the Fennec browser.js code will have to change to account for this.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Ensure the browser.js tap highlighting still works

This fixes the thing I mentioned above

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Try push, will wait for this before requesting reviews.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5e33e2f7cb3

Dominique, feel free to try out these patches and let me know if you see any problems with them. I briefly tested the test page you linked in comment 0 and I see the boxes flash green when I tap on them, so I think this should work.

Comment 29

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> Try push, will wait for this before requesting reviews.
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5e33e2f7cb3
> 
> Dominique, feel free to try out these patches and let me know if you see any
> problems with them. I briefly tested the test page you linked in comment 0
> and I see the boxes flash green when I tap on them, so I think this should
> work.

Kats, I applied your 3 patches but I still see the issue using my HTC (Android 2.3).
Did you test your patches following the point 2 in comment 8 [1]. You just have to zoom out and move the links near the right side of the screen.

There is something I don't understand in your changes (in layout/base/PositionedEventTargeting.cpp).
When the issue occurs, the closest clikable frame is not found by the method GetClosest (target is not changed after the call to this method). Then you are trying to find the nearest widget using target, but the target is probably not the good one. (I could be wrong, I didn't trace your patches but it could be the reason why I still see the issue on my test page)


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1078029#c8

Comment 30

[Kartikaya Gupta (email:[e-mail])]

(In reply to Dominique Vincent [:domivinc] from comment #29)
> Kats, I applied your 3 patches but I still see the issue using my HTC
> (Android 2.3).
> Did you test your patches following the point 2 in comment 8 [1]. You just
> have to zoom out and move the links near the right side of the screen.

Ah, you're right, I can reproduce this with my patches as well. I debugged it and it looks like the target area is coming out empty in this case because we're still clipping the inflated touch rect to the root scroll frame. This is a problem, but it's unrelated to bug 788073 and so I'll file a new bug to fix this.

> There is something I don't understand in your changes (in
> layout/base/PositionedEventTargeting.cpp).
> When the issue occurs, the closest clikable frame is not found by the method
> GetClosest (target is not changed after the call to this method). Then you
> are trying to find the nearest widget using target, but the target is
> probably not the good one. (I could be wrong, I didn't trace your patches
> but it could be the reason why I still see the issue on my test page)

I don't really understand what you're asking here. I agree that the closest clickable frame is not found by GetClosest, but that's because the target area is (incorrectly) empty and so GetFrameForArea returns an empty list.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Add repositioning of mouse events on Fennec

Now with better commit message.

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Bug 1092139 has the fix for the issue described in comment 30.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8514945 [details] [diff] [review]
Part 2 - Add repositioning of mouse events on Fennec

Actually roc is probably a more appropriate reviewer for this patch.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

yes.

Comment 35

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #30)
> 
> I don't really understand what you're asking here. I agree that the closest
> clickable frame is not found by GetClosest, but that's because the target
> area is (incorrectly) empty and so GetFrameForArea returns an empty list.

I made some new tests and you fixed the related issue in Bug 1092139. I made my test using the 3 patches from here and the patch from 1092139. The method GetClosest has still some issues with links that are child elements of the frame tested here: (!IsElementClickable(f)). With the previous process [1] it was not the case, the closest links were correctly detected. With complex html structures, the GetClosest method returns different elements compared to the previous code.

But the main concern is in the way the re-positioning is now done in your patch. The GetClosest process is now done 3 times (for each event: mousemove, mousedown, mouseup). Two issues here:
1- the performances are probably not better compared to the previous process (the closest search was executed only once before sending the 3 events),
2- the 3 calls to the GetClosest method can now return different elements if the html structure is changed between the different events. It should not be and the targeted element should be the same for the 3 events.

Another point in the new process, the 3 events are sent without knowing if we are in cluster area or not (GetClosest is not yet called). To implement bug 663803, the 3 events must not be sent in case of a cluster area. The new design should take into account this point. 

Do you think that it's possible to move the 3 calls sending the events inside the platform code and after the GetClosest call in order to fix the points 1 and 2 ? For me, this change will perfectly implement the design of bug 788073 (platform touch redirection) and it could be the reason of comment 19 [2] made by :bnicholson. 


[1] https://hg.mozilla.org/integration/mozilla-inbound/file/9c5e3e980339/mobile/android/chrome/content/browser.js#l4057
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1078029#c19

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8514945 [details] [diff] [review]
Part 2 - Add repositioning of mouse events on Fennec

Review of attachment 8514945 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/PositionedEventTargeting.cpp
@@ +410,5 @@
> +  }
> +
> +  // We first compute the bounds of |target| relative to the nearest widget.
> +  nsPoint offsetToWidget;
> +  nsIWidget* widget = target->GetNearestWidget(offsetToWidget);

This will fail in the presence of CSS transforms.

I think you should convert the event coordinate from aPointRelativeToRootFrame to be relative to target, then clamp to target's border-box, then convert it back to be relative to the widget. The former step is easy using nsLayoutUtils::TransformPoint. For the latter step, we kinda need to invert what nsLayoutUtils::GetEventCoordinatesRelativeTo does, but that method unnecessarily general... so here's what I think would work:
-- call GetView on aRootFrame. There should be one.
-- call nsLayoutUtils::TranslateWidgetToView to get the offset from the event's widget to aRootFrame->GetView().
-- use nsLayoutUtils::TransformPoint to convert from 'target' to aRootFrame, and then subtract the above offset.

Comment 37

[Kartikaya Gupta (email:[e-mail])]

(In reply to Dominique Vincent [:domivinc] from comment #35)
> I made some new tests and you fixed the related issue in Bug 1092139. I made
> my test using the 3 patches from here and the patch from 1092139. The method
> GetClosest has still some issues with links that are child elements of the
> frame tested here: (!IsElementClickable(f)). With the previous process [1]
> it was not the case, the closest links were correctly detected. With complex
> html structures, the GetClosest method returns different elements compared
> to the previous code.

From my understanding of the code the GetFramesForArea call should include the frames for child elements as well, so when we iterate through the frames they should be found as clickable. If this is not happening do you have a test case to isolate this bug?

> But the main concern is in the way the re-positioning is now done in your
> patch. The GetClosest process is now done 3 times (for each event:
> mousemove, mousedown, mouseup). Two issues here:
> 1- the performances are probably not better compared to the previous process
> (the closest search was executed only once before sending the 3 events),

True, but I'm not convinced that the performance change here is even measurable as these should be very fast operations.

> 2- the 3 calls to the GetClosest method can now return different elements if
> the html structure is changed between the different events. It should not be
> and the targeted element should be the same for the 3 events.

I don't think this has changed with respect to the original code. Even with the original code, when we create and dispatch 3 mouse events from browser.js, it has to run through the hit-testing code in gecko to find the right target. If the html structure changes in between then you end up with different targets for the three events. I don't think you can avoid that, really. If it is the case that the html structure is changing then it's probably more correct to use the updated target than to use the same targeted element always.

> Another point in the new process, the 3 events are sent without knowing if
> we are in cluster area or not (GetClosest is not yet called). To implement
> bug 663803, the 3 events must not be sent in case of a cluster area. The new
> design should take into account this point. 

Ok.

> Do you think that it's possible to move the 3 calls sending the events
> inside the platform code and after the GetClosest call in order to fix the
> points 1 and 2 ? For me, this change will perfectly implement the design of
> bug 788073 (platform touch redirection) and it could be the reason of
> comment 19 [2] made by :bnicholson. 

So here's a suggestion, let me know if this will work for you. In the part 3 patch I posted on this bug, I dispatch a hit-test event from nsWindow.cpp and which is picked up in browser.js. This hit-test event will have both the "GetClosest" element as the target and updated event coordinates that are inside the target's bounds. Right now we just save the target to _highlightElement in browser.js. What we can do is also save the x and y coordinates, and then when we dispatch the mousemove/mousedown/mouseup events in the Gesture:SingleTap handler, we use the these saved x/y coordinates. That should effectively restore the behaviour the way it was originally. If you need additional information about cluster areas you can stick that on the MozMouseHitTest event and then get it in browser.js. Then you can use that to skip sending the mousemove/mousedown/mouseup entirely if needed.

Comment 38

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Add repositioning of mouse events on Fennec (v2)

I think this is what you were saying, but with more helper methods.

Comment 39

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> 
> From my understanding of the code the GetFramesForArea call should include
> the frames for child elements as well, so when we iterate through the frames
> they should be found as clickable. If this is not happening do you have a
> test case to isolate this bug?

In this page:
http://office.microsoft.com/fr-fr/training/cours-de-formation-sur-office-2013-HA104096598.aspx

Try to touch just under 2013 of "Power Point 2013"
Using Firefox 33, you can highlight the link "Power Point 2013" clicking near the link.
With your version, the closest link is never found, the highlight never occurs.

In this page
http://office.microsoft.com/fr-fr/
Try to highlight the bottom right links (for instance touch right of link "Office 365 Famille") 

> 
> I don't think this has changed with respect to the original code. Even with
> the original code, when we create and dispatch 3 mouse events from
> browser.js, it has to run through the hit-testing code in gecko to find the
> right target. If the html structure changes in between then you end up with

If the hit-testing code in gecko was used, we should have had the same behavior (same errors with complex html structure).
I think that the hit testing code was not called in the old process, because the first found target was already clickable.
We should trace the old process to get a definitive answer about that.

> 
> So here's a suggestion, let me know if this will work for you. In the part 3
> patch I posted on this bug, I dispatch a hit-test event from nsWindow.cpp
> and which is picked up in browser.js. This hit-test event will have both the
> "GetClosest" element as the target and updated event coordinates that are
> inside the target's bounds. Right now we just save the target to
> _highlightElement in browser.js. What we can do is also save the x and y
> coordinates, and then when we dispatch the mousemove/mousedown/mouseup
> events in the Gesture:SingleTap handler, we use the these saved x/y
> coordinates. That should effectively restore the behaviour the way it was
> originally. If you need additional information about cluster areas you can
> stick that on the MozMouseHitTest event and then get it in browser.js. Then
> you can use that to skip sending the mousemove/mousedown/mouseup entirely if
> needed.

It should be better. But I don't like the fact that the "touch" event set some variables in browser.js and then the same variables are used by another event ("Gesture:SingleTap"). The reviewer of the browser.js code changes should give us a definitive answer on this approach.

Comment 40

[Kartikaya Gupta (email:[e-mail])]

A more recent try push with the updated version of the patch:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0dc2aa4cf6b5

Comment 41

[Kartikaya Gupta (email:[e-mail])]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c87ea67e88a1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/74c1d5f9020d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/138515da8c06

Comment 42

[Kartikaya Gupta (email:[e-mail])]

(In reply to Dominique Vincent [:domivinc] from comment #39)
> In this page:
> http://office.microsoft.com/fr-fr/training/cours-de-formation-sur-office-
> 2013-HA104096598.aspx
> 
> Try to touch just under 2013 of "Power Point 2013"
> Using Firefox 33, you can highlight the link "Power Point 2013" clicking
> near the link.
> With your version, the closest link is never found, the highlight never
> occurs.

I debugged this and found that when you tap there, the block directly under the finger is also determined to be "clickable" which is why it doesn't use the link instead. The block is clickable because the body element has some listeners on it and the check at [1] doesn't pass the nsGkAtoms::body second parameter like the call at [2] does. Changing that fixes the problem.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp?rev=c3c317bd388f#334
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp?rev=c3c317bd388f#374

> In this page
> http://office.microsoft.com/fr-fr/
> Try to highlight the bottom right links (for instance touch right of link
> "Office 365 Famille") 

I didn't test specifically but my change fixed this one as well so it's probably the same thing.

I'll file a follow-up bug to fix this.

Comment 43

[Kartikaya Gupta (email:[e-mail])]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1093686 with patch.

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c87ea67e88a1
https://hg.mozilla.org/mozilla-central/rev/74c1d5f9020d
https://hg.mozilla.org/mozilla-central/rev/138515da8c06

Comment 45

[u279076]

Dominic, this should be resolved in the latest Nightly build. Can you please confirm?

Comment 46

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8514527 [details] [diff] [review]
Part 1 - Disable retargeting of touch events on Fennec

Approval Request Comment (for all patches)
[Feature/regressing bug #]: bug 788073
[User impact if declined]: Scripts in web content might be suprised because (a) coordinates in touch events are not the "raw" coordinates from the user and (b) for both mouse and touch events, the coordinates of the event may be outside the target element. This second one in particular may break assumptions made by web content. It is also incompatible with behaviour in other browsers. This patch restores these two behavioural elements to how it was before bug 788073. 
[Describe test coverage new/current, TBPL]: tested locally
[Risks and why]: affects Fennec only, pretty safe set of patches I think.
[String/UUID change made/needed]: none

Comment 47

[Dominique Vincent [:domivinc]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #45)
> Dominic, this should be resolved in the latest Nightly build. Can you please
> confirm?

Partial fix in Nightly build, but there is probably somewhere a follow-up bug to fix the remaining issues (see comment 42)

Comment 48

[Kartikaya Gupta (email:[e-mail])]

Bug 1093686 has the rest of the fix, and it's on inbound now. I'll have to fix the test I wrote for it but the fix itself should make it to central on the next merge.

Comment 49

[u279076]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #48)
> Bug 1093686 has the rest of the fix, and it's on inbound now. I'll have to
> fix the test I wrote for it but the fix itself should make it to central on
> the next merge.

Dominic, can you retest this once bug 1093686 lands?

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f76f9426e70
https://hg.mozilla.org/releases/mozilla-aurora/rev/0d0bf4db9dca
https://hg.mozilla.org/releases/mozilla-aurora/rev/f00dc82c5f32

Comment 51

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)

> originally. If you need additional information about cluster areas you can
> stick that on the MozMouseHitTest event and then get it in browser.js. Then

Hi Kats, could you help me to implement this change? Where and how in the MozMouseHitTest event should I add the cluster information? Do you have some links to similar changes?

Comment 52

[Kartikaya Gupta (email:[e-mail])]

(In reply to Dominique Vincent [:domivinc] from comment #51)
> Hi Kats, could you help me to implement this change? Where and how in the
> MozMouseHitTest event should I add the cluster information? Do you have some
> links to similar changes?

Sure. So right now in your implementation (in attachment 8504792 [details] [diff] [review]), the flow looks something like this:

- mouse button down event goes to FindFrameTargetedByInputEvent
- FindFrameTargetedByInputEvent checks if touch-clustering is enabled
- if yes, it counts the number of elements in the cluster as part of the "GetClosest" code
- Then it dispatches a touchcluster event with that information
- code in browser.js picks up the touchcluster event
- browser.js sends a Gesture:clusteredLinksClicked message to Java
- Java displays the zoomed view

Instead, what I'm suggesting is this:

- Add a boolean flag to the WidgetMouseEvent class, let's call it "hitCluster"
- MozMouseHittest event goes to FindFrameTargetedByInputEvent
- FindFrameTargetedByInputEvent checks if touch-clustering is enabled
- if yes, it counts the number of elements in the cluster as part of the "GetClosest" code
- if there is cluster, it sets the hitCluster flag to true on event
- When that event dispatch unwinds back to [1], you read the hitCluster flag from the event
- If the flag is true, you can notify Java and/or browser.js to display the zoomed view.

I didn't look through your patch in a lot of detail so I'm not sure how you're cancelling the existing click event that triggers the zoomed-in view, but it shouldn't be too hard to do that in this flow. Does that sound like a reasonable approach?

[1] http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp?rev=663a6bc26926#1101

Comment 53

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #52)

Thanks for your help. I had already made the changes described in your suggestion (my new flag was named "inCluster" but "hitCluster" is fine).
My problem was not located in the first steps of your suggestion. My issue is here: I don't find the correct way to get the "hitCluster" information inside the function _handleRetargetedTouchStart (in browser.js).
In the function _handleRetargetedTouchStart the new targeted position is available, but the new flag value is not available. 
For testing purpose, I moved the "hitCluster" flag value in the field "pressure" ("pressure" seems to not be used by the event MozMouseHittest). I get the correct value in the event field "mozPressure" inside _handleRetargetedTouchStart. It works.
But I would prefer to keep the "hitCluster" information in a separated field and get it directly inside _handleRetargetedTouchStart. The new field in the jscript event could be named "mozHitCluster". I don't want to mix the cluster and the pressure data ... 
Do you have any idea to achieve that? Is there a better way to get the cluster information inside the function _handleRetargetedTouchStart ?

Comment 54

[Kartikaya Gupta (email:[e-mail])]

You can add a new field similar to mozPressure. If you put it in the nsIDOMMouseEvent.idl file it should become accessible from JS. See how mozPressure is implemented for example: http://mxr.mozilla.org/mozilla-central/ident?i=mozPressure

However from your existing patch out looks like most of the work occurs in Java, which I'd why I suggested the approach in comment 52 where you send a message from nsWindow to java and not bother with exposing this flag to JS.

Btw I think you can mark the field as ChromeOnly in the IDL file so it doesn't get exposed to web content. See for example http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Document.webidl#204

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

s/nsIDOMMouseEvent.idl/MouseEvent.webidl/

Comment 56

[Dominique Vincent [:domivinc]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #49)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #48)
> > Bug 1093686 has the rest of the fix, and it's on inbound now. I'll have to
> > fix the test I wrote for it but the fix itself should make it to central on
> > the next merge.
> 
> Dominic, can you retest this once bug 1093686 lands?

Anthony, I tested again with bug 1093686 landed, I still see the issue in the page:
http://office.microsoft.com/fr-fr/training/cours-de-formation-sur-office-2013-HA104096598.aspx

Bug 1093686 fixed the body listener issue but it looks like there is another problem due to the html structure of the links.

Steps to reproduce the issue:
1- zoom out the page in order to get very small links
2- click on the left of 2 links (for instance "Access 2013" and "Lync 2013"), the closest link is correctly detected.
3- click on the right of the 2 same links, the closest link is never detected.

I tested the same points using Firefox 33 and it works in both cases. Closest link is correctly detected on the right or on the left side.

Kats I found the following list of frames in method "GetClosest". 
When it works (step 2), the list of frames contains the following elements:
- not clickable: <li id="bmkTOClinks"><a href="#_Toc362454444">Lync&nbsp;2013</a></li>
- empty region : <a href="#_Toc362454444">Lync&nbsp;2013</a>
- clickable : Lync&nbsp;2013
- ... same with "Access 2013"

When it doesn't work (step 3), the list of frames is:
- not clickable: <li id="bmkTOClinks"><a href="#_Toc362454444">Lync&nbsp;2013</a></li>
- not clickable: <li id="bmkTOClinks"><a href="#_Toc362454443">Access&nbsp;2013</a></li>
- ...

The text element frames (clickable) are not in the list of frames. It's the reason why it doesn't work.

Comment 57

[Kartikaya Gupta (email:[e-mail])]

Please file a new bug for this.

Comment 58

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #57)
> Please file a new bug for this.

New bug 1096042 created.