1250024 - Trying to tap on forum links is maddening

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Matt Cosentino]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160221030340

Steps to reproduce:

A good example is here:

http://community.pearljam.com/categories/the-porch


Actual results:

It's maddening trying to actually open the topic that I tap on, it will open one up to two topics down or open the user profile. It seems to me that it is heavily weighting the links for topics with long titles, so trying to tap on one such as "New album?" seems to be impossible.

Comment 1

[Matt Cosentino]

I've tested in 44.0.2 and do not see the issue, so it seems to be a regression.

Comment 2

[Michelle Funches - QA]

Confirmed:
OK in 44.0.2
Not OK in 47.0a1
Access link above in Nightly - it is not as smooth as clicking link in 44.0.2
1) http://community.pearljam.com/categories/the-porch
2) click a link on page i.e. Pearl Jam's Music or pearlgirl52... (sometimes the link was not clickable, mouse pointer should have updated and it didnt)
MozRegression displays:  8:53.16 LOG: MainThread main INFO Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1147673

Comment 3

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

[Tracking Requested - why for this release]:
Based on the previous comment this is a regression in FF46.
(but someone, please verify that FF45 doesn't have this.)

Comment 4

[Markus Stange [:mstange]]

I don't understand this bug, or I can't reproduce it on OS X. Can you make a screen recording?
I've looked at the links in the blue boxes in the sidebar. It looks like the boxes around the links are not clickable, only the text itself is. And not all of the boxes have clickable text. I can see the mouse curser change to a hand whenever the mouse is over the text of a clickable link, and to the regular text cursor if it's over a non-clickable box.
All of these things work the same in Firefox 44.0.2 and in Nightly 47.0a1 (2016-02-22) on my machine (Mac OS 10.11.4 Beta).

From the initial report it sounds like something goes wrong when we do hit testing of the click, and sometimes targeting the wrong link. I don't see anything like that.

Comment 5

[Andrew Overholt [:overholt]]

Markus, feel free to re-assign if you're the wrong person here.

Comment 6

[Michelle Funches - QA]

Not seen in:
Version 	45.0b9
Build ID 	20160223142613
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0
---yesterday I would click a link on the right hand column "Most Recent" and could not update to display that page. I will continue to try and capture video of what can occur.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this sounds like a regression in 46.

Comment 8

[Matt Cosentino]

I think we're confusing different issues here. My original report was for tapping on the forum topic links, not clicking on the sidebar links. As in with my finger, not the mouse.

Comment 9

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

If you set the dom.w3c_touch_events.enabled pref to 0 (browser restart required), does that make the problem go away?

Comment 10

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

(My question in comment 9 was directed to the reporter)

Comment 11

[Matt Cosentino]

Yes, that did the trick.

Comment 12

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

Thanks. This is a regression from bug 1180706 (nightly-only). We need to fix it before we let touch on windows ride the trains (bug 1244402). Marking 46 as unaffected since comment 2 is about a different issue.

Comment 13

[Andrew Overholt [:overholt]]

I'm making the component match that of bug 1180706.

Comment 14

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

Updating flags since Nightly is now 48, and 47 (on Aurora) is unaffected.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a recent regression and we want it before shipping a feature (touch on windows)

Comment 16

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

This doesn't need to track 49, we don't intend to ship touch on windows in 49.

Comment 17

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

I'm not able to reproduce this in a recent Nightly build with touch support enabled. Matt, do you still see this issue (note that you need to have e10s as well as touch events enabled to actually get APZ touch support).

If you can reproduce it, a video would be helpful, or the specific link you're having trouble with. Thanks!

Comment 18

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

Closing as WFM for now. Please reopen if you are still seeing this.

Comment 19

[Matt Cosentino]

I'm still experiencing the issue. Here's a video:

https://goo.gl/photos/m19DZH6AawYVxiZc7

As you can see, I'm tapping on one of the links but it isn't doing anything. I tap a little to the right and it opens the user profile link from the post below it, nowhere near where I was tapping.

Comment 20

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

Thanks for the video! I tried again to reproduce it and while I couldn't at first, I was able to reproduce the behavior after changing the zoom to 90% (or 110%). Can you check if you have set your page zoom level to something other than 100%? If so, does changing the zoom to 100% fix the issue for you? I just want to make sure the issue I'm seeing is the same as the one you are running into.

Comment 21

[Matt Cosentino]

Yeah, that was it. I must have changed the zoom by accident and didn't realize it.

Comment 22

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

Great, thanks for confirming! I'll take a look at fixing this - we are probably just not accounting for that zoom factor somewhere.

Comment 23

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

I made an APZ mochitest that reproduces this problem in order to debug it. What's happening here is that at [1] we convert from LD to CSS pixels, but the LD value we're converting is relative to the window top-left, so it includes some chrome area. When we divide to get CSS pixels, we're using the CSSToLD scale from the content and applying it to some of the chrome which is incorrect. The subsequent transformation at [2] is supposed to move the point into the child process' coordinate space, but turns the value into even more garbage.

I think what we need to do is to keep the HandleTap API in LD pixels, and move the conversion to CSS pixels into the child process. The ChromeProcessController path may need to be updated accordingly as well. It should be a fairly straightforward fix - I think until now we haven't really exercised the touch + full zoom + e10s platform combination, which is what this affects.

[1] http://searchfox.org/mozilla-central/rev/336105a0de49f41a2cc203db7b7ee7266d0d558b/gfx/layers/apz/src/AsyncPanZoomController.cpp#1486
[2] http://searchfox.org/mozilla-central/rev/336105a0de49f41a2cc203db7b7ee7266d0d558b/dom/ipc/TabParent.cpp#1303

Comment 24

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

I did what I described in comment 23. There was another problem where we were using widget->GetDefaultScale() as the CSSToLayoutDeviceScale but it doesn't account for the full zoom. I had to modify some of the use sites of that in order to get it working.

We should probably introduce a new coordinate space that is between CSS and LD pixel space, that includes the widget->GetDefaultScale() but not the full zoom. And more importantly, change widget->GetDefaultScale() to not return a CSSToLayoutDeviceScale, because that's not correct.

Comment 25

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d4fff3060da

Comment 26

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

Whoops, android bustage. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2f90e277bd6

Comment 27

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

Attachment: Bug 1250024 - Fix touch-tap event coordinate transformations when a fullzoom is applied.

There were a couple of problems when delivering tap gestures to content with
full zoom applied. One was that the ConverToGecko function converted the coords
into "CSS pixel" space by using the web content's CSS-to-LD scale, but also
applied that on the translation from the chrome area. Moving that conversion
to later in the process (after the coords got passed through TabParent::
AdjustTapToChildWidget) corrected that issue.

The other problem was that bits of code in APZEventState and APZCCallbackHelper
were using the widget->GetDefaultScale() value as the CSS-to-LD scale, but that
omitted the full zoom value. Getting the CSS-to-LD scale from the presShell and
propagating that through corrected that issue.

Review commit: https://reviewboard.mozilla.org/r/67528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67528/

Comment 28

[Botond Ballo [:botond]]

Comment on attachment 8775360 [details]
Bug 1250024 - Fix touch-tap event coordinate transformations when a fullzoom is applied.

https://reviewboard.mozilla.org/r/67528/#review65112

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
(Diff revision 1)
>        return false;
>      }
>  
> -    { // scoped lock to access mFrameMetrics
> -      ReentrantMonitorAutoEnter lock(mMonitor);
> -      // NOTE: This isn't *quite* LayoutDevicePoint, we just don't have a name

Isn't this comment still accurate?

Comment 29

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

(In reply to Botond Ballo [:botond] from comment #28)
> > -    { // scoped lock to access mFrameMetrics
> > -      ReentrantMonitorAutoEnter lock(mMonitor);
> > -      // NOTE: This isn't *quite* LayoutDevicePoint, we just don't have a name
> 
> Isn't this comment still accurate?

Is it - I'm not sure in what way the return value is different from the LayoutDevice coordinate space anymore.

Comment 30

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #29)
> (In reply to Botond Ballo [:botond] from comment #28)
> > > -    { // scoped lock to access mFrameMetrics
> > > -      ReentrantMonitorAutoEnter lock(mMonitor);
> > > -      // NOTE: This isn't *quite* LayoutDevicePoint, we just don't have a name
> > 
> > Isn't this comment still accurate?
> 
> Is it - I'm not sure in what way the return value is different from the
> LayoutDevice coordinate space anymore.

Thinking about it some more, I guess it is the LayoutDevice coordinates of the root content (but not necessarily of the content associated with this APZC). Since we don't have a way of differentiating LayoutDevice coordinates of different content elements in the type system, this is fine.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b0313fd4ce6
Fix touch-tap event coordinate transformations when a fullzoom is applied. r=botond

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b0313fd4ce6