[AccessFu] Explore by Touch uses wrong coordinates on HiDPI Android devices

VERIFIED FIXED in Firefox 27

Status

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: maxli, Assigned: maxli)

Tracking

({regression})

unspecified
mozilla29
All
Android
regression
Points:
---

Firefox Tracking Flags

(firefox26 unaffected, firefox27+ fixed, firefox28+ fixed, firefox29+ verified)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 8362394 [details] [diff] [review]
patch

Turns out one of the regressions that I was reacting to in Bug 890940 was caused by an actual bug (bug 924791), and not just an intentional change like the other ones. So when it was fixed, explore by touch coordinates are now incorrectly multiplied by the device pixel ratio, causing EBT on HiDPI devices to be pretty unusable.
Attachment #8362394 - Flags: review?(eitan)

Comment 1

5 years ago
Thanks, maxLi! Marking this bug as a regression anyway since bug 924791 caused us to regress. And this also explains why I could never reproduce it, since I don't yet have high DPI devices here. We might be too late for the initial 27 release, or maybe not...But we should get this reviewed and landed ASAP so we can uplift.

Have you tested that this also always activates correct items still when double-tapping?
Blocks: 924791
Keywords: regression

Updated

5 years ago
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
tracking-firefox27: --- → ?
tracking-firefox28: --- → ?
tracking-firefox29: --- → ?
(Assignee)

Comment 2

5 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #1)
> Have you tested that this also always activates correct items still when
> double-tapping?

Yes it does (which is unsurprising, since bug 924791 is about mouse/touch event coordinates, whereas the coordinates when double tapping are generated based upon the focused item)

Comment 3

5 years ago
Comment on attachment 8362394 [details] [diff] [review]
patch

r=me. Stealing review so we can get this in at the earliest possible time, given the urgency of the release 27 being very close.

I tested this on a Nexus 7 2012 edition and a newly aquired Nexus 7 2013 edition, and it indeed fixes the problem of Explore By Touch, which I can reproduce on the 2013 Nexus 7 in Nightly builds.
Attachment #8362394 - Flags: review?(eitan) → review+

Comment 4

5 years ago
Landed on Inbound on MaxLi's behalf: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3e8f10d4d8
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/ce3e8f10d4d8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Patch uplift on beta depends on how risky the patch is and the user reward here given we only have a final beta left. If this is absolutely low risk and there is less scope of fallout's and benefits users in a big way, please request uplift on beta with analysis. We would have to land it soon though so this can get in tomorrow on preparation for our Thursday's beta.

Else, we can wontfix for Fx27 but take an aurora uplift instead and resolve this in Fx28.

Updated

5 years ago
status-firefox26: --- → wontfix
tracking-firefox27: ? → +
tracking-firefox28: ? → +
tracking-firefox29: ? → +

Comment 7

5 years ago
Verified fixed in the 2nd nightly build made on 2014-01-21. Note that Firefox26 is not affected by this bug, so changing its flag accordingly.
Status: RESOLVED → VERIFIED
status-firefox26: wontfix → unaffected
status-firefox29: affected → verified

Comment 8

5 years ago
Comment on attachment 8362394 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 924791
User impact if declined: Blind users will no longer be able to explore web content with their finger on devices such as Nexus 5, Nexus 7 2013 edition, Samsung Galaxy S IV, and other High DPI Android devices, effectively breaking accessibility except for the very time consuming purely sequential swiping gesture.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): None. This just reverts a small part of bug 892940, which originally contained this workaround before bug 924791 was fixed. See comment #0 also.
String or IDL/UUID changes made by this patch: None.
Attachment #8362394 - Flags: approval-mozilla-beta?
Attachment #8362394 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #8362394 - Flags: approval-mozilla-beta?
Attachment #8362394 - Flags: approval-mozilla-beta+
Attachment #8362394 - Flags: approval-mozilla-aurora?
Attachment #8362394 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.