Trying to tap on forum links is maddening

RESOLVED FIXED in mozilla50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mattcoz, Assigned: kats)

Tracking

({regression})

47 Branch
mozilla50
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, firefox50+ unaffected)

Details

(Whiteboard: [gfx-noted][nightly only])

Attachments

(1 attachment)

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.
I've tested in 44.0.2 and do not see the issue, so it seems to be a regression.
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
Status: UNCONFIRMED → NEW
Component: Untriaged → Event Handling
Ever confirmed: true
Product: Firefox → Core
[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.)
Flags: needinfo?(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.
Flags: needinfo?(mstange) → needinfo?(mfunches)
Markus, feel free to re-assign if you're the wrong person here.
Assignee: nobody → mstange
Whiteboard: btpp-active
Whiteboard: btpp-active → btpp-active btpp-fixnow
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.
Flags: needinfo?(mfunches)
Tracking since this sounds like a regression in 46.
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.
If you set the dom.w3c_touch_events.enabled pref to 0 (browser restart required), does that make the problem go away?
(My question in comment 9 was directed to the reporter)
Flags: needinfo?(mattcoz)
Yes, that did the trick.
Flags: needinfo?(mattcoz)
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.
Assignee: mstange → nobody
Blocks: 1244402, 1180706
No longer blocks: 1147673
OS: Unspecified → Windows 10
I'm making the component match that of bug 1180706.
Component: Event Handling → Panning and Zooming
Whiteboard: btpp-active btpp-fixnow → btpp-fixlater
Updating flags since Nightly is now 48, and 47 (on Aurora) is unaffected.
Whiteboard: btpp-fixlater → [gfx-noted]
Assignee: nobody → bugmail.mozilla
Whiteboard: [gfx-noted] → [gfx-noted][nightly only]
Tracking since this is a recent regression and we want it before shipping a feature (touch on windows)
This doesn't need to track 49, we don't intend to ship touch on windows in 49.
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!
Flags: needinfo?(mattcoz)
Closing as WFM for now. Please reopen if you are still seeing this.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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.
Yeah, that was it. I must have changed the zoom by accident and didn't realize it.
Great, thanks for confirming! I'll take a look at fixing this - we are probably just not accounting for that zoom factor somewhere.
Flags: needinfo?(mattcoz)
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
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.
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/
Attachment #8775360 - Flags: review?(botond)
Attachment #8775360 - Flags: review?(botond) → review+
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?
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b0313fd4ce6
Fix touch-tap event coordinate transformations when a fullzoom is applied. r=botond
https://hg.mozilla.org/mozilla-central/rev/3b0313fd4ce6
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1292002
No longer blocks: 1290823
Depends on: 1290823
No longer depends on: 1292002
You need to log in before you can comment on or make changes to this bug.