1190541 - PositionedEventTargeting doesn’t take into account the zoom in/out actions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dominique Vincent [:domivinc]]

AppUnitsFromMM method in PositionedEventTargeting.cpp doesn’t take into account the presshell resolution when converting from MM to app units.
The resolution factor has been removed in bug 981651 because the code didn’t work correctly.
The AppUnits value was multiplied by the resolution factor.

Making several tests for the implementation of the zoomed view, it looks like that this calculation without taking into account the resolution doesn’t work well when the user zoom in or zoom out. In this case, the zoomed view is displayed when it should not.

Comment 1

[Dominique Vincent [:domivinc]]

Attachment: patch-03082015 1-Bug_1190541____PositionedEventTargeting_doesn___t_take_into_account_the_zoom_in_out_actions__r_kats.patch

Kats, on Android, this patch seems to fix the issue. But it's really strange that this issue is not visible on the b2g side.
The difference with the original code prior to bug 981651 is the following: I replaced the multiplication by a division.
When you zoom out, the resolution is higher (for instance: getResolution() = 1 before zoom out, = 2 after zoom out). 5 mm converted in application units after the zoom out should be a lower value (divided by 2 in my example).

Comment 2

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

(In reply to Dominique Vincent [:domivinc] from comment #1)
> When you zoom out, the resolution is higher (for instance: getResolution() =
> 1 before zoom out, = 2 after zoom out). 5 mm converted in application units
> after the zoom out should be a lower value (divided by 2 in my example).

This sounds wrong; after zooming out the resolution should go down, if anything.

Comment 3

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

Comment on attachment 8642606 [details] [diff] [review]
patch-03082015 1-Bug_1190541____PositionedEventTargeting_doesn___t_take_into_account_the_zoom_in_out_actions__r_kats.patch

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

I think dividing by the resolution makes sense here, but only if the presShell->ScaleToResolution() function returns true. If ScaleToResolution() is not true, then a higher resolution doesn't necessarily mean things are actually bigger on the screen, they're just drawn at a higher density. r+ if you update the code so that the extra division only happens if ScaleToResolution() returns true.

Comment 4

[Dominique Vincent [:domivinc]]

Attachment: patch-04082015 1-Bug_1190541____PositionedEventTargeting_doesn___t_take_into_account_the_zoom_in_out_actions__r_kats.patch

ScaleToResolution() is now tested.

Comment 5

[Dominique Vincent [:domivinc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80c36c103249

Comment 6

[Dominique Vincent [:domivinc]]

Kats, some B2G tests failed, specifically 8 9 and 22 could be linked to the change.
Do you know if the tests are executed with a specific value for the zoom (value of getResolution) ?  

B2G ICS Emulator opt
3 dom/contacts/tests/test_contacts_basics.html 
8 layout/base/tests/test_event_target_radius.html

B2G ICS Emulator debug
9 dom/events/test/test_bug741666.html
22 layout/base/tests/test_event_target_radius.html

Comment 7

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

Most of the tests don't have a specific zoom value, but because they don't have meta-viewport tags I believe they will get scaled so that 980 CSS pixels are visible on initial page load. The resolution will therefore depend on the width of the emulator screen.

Comment 8

[Dominique Vincent [:domivinc]]

Do you think that we can add in the 3 html test files the following meta tag:
<meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=0">

Comment 9

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

In test_event_target_radius that might make sense. I think test_contacts_basics.html is only an intermittent failure, it shows up in 1 of the 3 M-3 failures on your try push. So the only real failure is test_bug741666.html and that might be a bug in the test which needs fixing.

Comment 10

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

(But to answer your question - no, I don't think we should add that meta-viewport tag to test_bug741666.html without investigating it first and understanding why the test is failing)

Comment 11

[Dominique Vincent [:domivinc]]

Attachment: log_mochitest.txt

Kats, I’m trying to work on the test issue and I need your help (if you have time). 
I never run tests on my machine. I started to configure it using different guides [1] and [2] in order to be able to run mochitests on a b2g simulator.

After the different installations and configurations, I can run the b2g simulator using the command “./b2g -profile /home/vincent/gaia/profile” without any issue.

But I have many issues when I try to run mochitests using the command “./mach mochitest --profile /home/vincent/gaia/profile dom/events/test/”. See the attached log file for details.

I don’t understand different points in the messages (“TEST-SKIP”, “Failed to init NSS: libnssdbm3.so:”, “addons.xpi	WARN	Exception running bootstrap method”, …) and I didn’t find a solution online. I found the following bugs 1176638 and 1146713, but I’m not sure that there is a link with my issue. 

Any feedback about the error messages is welcome.

How do you investigate issues in “test_bug741666.html” for instance? Are you using a simulator or a b2g device to run the mochitest?

[1] https://developer.mozilla.org/en-US/Firefox_OS/Building_the_Firefox_OS_simulator
[2] https://developer.mozilla.org/en-US/Firefox_OS/Automated_testing/Mochitests

Comment 12

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

So unfortunately I think you're going down the wrong path here. The B2G "simulator" is different from the B2G "emulator". The former is a build of mozilla-central with some special flags that cause it to open a window that's mobile-device-sized. This is also known as "B2G desktop" and is labelled as such on TreeHerder.

The failure in your try push was on B2G emulator, which is actually a full ARM build of the B2G system running on the QEMU emulator that comes with Android SDK. The set of prefs and configurations are quite different (for example, B2G desktop doesn't have APZ or zooming behaviour, nor does it support touch events - but B2G emulator does).

If you want I can help you get that building and running although in practice it tends to be quite difficult (and on Windows you need a VM to do it). Even if you do get it up and running debugging it is usually a process of adding lots of printfs anyway, so my recommendation is to just debug it by adding printfs as needed and doing a try push. You can run just the specific test that's failing in your try push for faster turnaround. If you want to build the emulator locally the instructions start at [1].

[1] https://developer.mozilla.org/en-US/Firefox_OS/Firefox_OS_build_prerequisites

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Hey Dominique, any progress here? I think a fix might really improve the cluster detection, and get us a lot closer to shipping this feature.

Comment 14

[Dominique Vincent [:domivinc]]

I tried to build the B2G emulator but it's not an easy task with bug 1146713. I manually fixed the ssltunnel issue using the different comments found in the bug. I was happy! My B2G emulator works, I can simulate the reception of a SMS sending it using telnet... 
But then, trying to run the browser, I realized that the internet connection works randomly in the B2G browser.
I can run an Android emulator without any connection issue on the same configuration (Virtual Box ubuntu). 
So I'm waiting for the resolution of bug 1146713 to build a new B2G emulator and see if the data connection works better.

I also tried to understand the test that is failing but without running it (and probably a lot of break points), I didn't succeed to find the reason of the issue. 

So feel free to take this bug and fix the failing test (or my patch).

Comment 15

[Dominique Vincent [:domivinc]]

This bug has a negative impact on the zoomed view behavior but also in other cases like a single click on a page. For instance in the page “mydealz.de”, after a zoom, you get the same issues related in bug 1233770. The arrows don’t display the vertical menus. The back out of bug 1222234 won’t fix this case (click after a zoom in the page). 
I can reproduce the issue in firefox 45.0a2.

It could also explain the problem (link targeting just seems "off" ...) reported in bug 1233272 comment 3.

Comment 16

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

I agree that this is a thing that needs fixing. It sounds like you got stalled with the B2G emulator. We should probably do another try push with the patch to see what's failing now (the test failures have probably changed since we last tried) and if there are still B2G emulator failures I can look into them.

Comment 17

[Dominique Vincent [:domivinc]]

Attachment: patch-29122015B 1-Bug_1190541____PositionedEventTargeting_doesn___t_take_into_account_the_zoom_in_out_actions__r_kats.patch

Same patch rebased.

Comment 18

[Dominique Vincent [:domivinc]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

(In reply to Dominique Vincent [:domivinc] from comment #15)
> It could also explain the problem (link targeting just seems "off" ...)
> reported in bug 1233272 comment 3.

What I don't understand about this is that my issues with reddit only started when Aurora updated from 44 to 45 on my phone. But this goes back to 42?

Comment 20

[Dominique Vincent [:domivinc]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> (In reply to Dominique Vincent [:domivinc] from comment #15)
> > It could also explain the problem (link targeting just seems "off" ...)
> > reported in bug 1233272 comment 3.
> 
> What I don't understand about this is that my issues with reddit only
> started when Aurora updated from 44 to 45 on my phone. But this goes back to
> 42?

I don't know. I just found the issue working on the implementation of the zoomed view because the issue is more visible.

Comment 21

[Dominique Vincent [:domivinc]]

Kats, there are still the same B2G test failures (3 8 9 and 22). If you have time, could you help to find a solution?

Comment 22

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

Yeah, I'm reproducing it locally and will debug. Leaving needinfo on me for now.

Comment 23

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

So the dom/events/test/test_bug741666.html failure is because the test is trying to click on a <span> element and it ends up clicking on a nearby <a> element instead. This can be fixed in a number of ways - we can set a meta-viewport tag on it, we can move the <span> so that it's farther away, or we can disable fluffing for touch events (bug 1091889). That last option is harder to do because of test failures but the first two should be fine. I stuck a margin-left:100px on the span in the test and that fixed the problem for me locally. I suspect setting a meta-viewport on it might not actually work (see next paragraph).

For the test_event_target_radius.html failure, I tried setting a meta-viewport but for some reason it didn't have the desired effect. I think that the test is running inside an iframe and so the meta-viewport doesn't have any effect. I'm not entirely sure what to do about that. I'll try to update the test so that it works even when zoomed out (when the fluff area legitimately corresponds to more app units).

Comment 24

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

Ok, I think I managed to update the tests so that they work with the patch. Try push with just the test fixes:

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

and try push with the test fixes plus your patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5beefa97408b

Comment 25

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

That didn't work quite so well. Here's another attempt:

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

Comment 26

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

FYI I'm still working on this. The event-radius test is quite finicky but I'm getting close to a fix.

Comment 27

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

Attachment: Update tests

Ok, here we go. This updates the tests so that they work with this change. Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=00510d0e148a&group_state=expanded&selectedJob=15084152 looking green.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd0f47ee2f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0e4f31589e

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6bd0f47ee2f2
https://hg.mozilla.org/mozilla-central/rev/7f0e4f31589e

Comment 30

[Dominique Vincent [:domivinc]]

Kats, should we uplift to Aurora? The issue is visible in Aurora, no?

Comment 31

[Dominique Vincent [:domivinc]]

I just tested using Beta (44.0b2) and the issue is also visible with this version on my device. So it could be nice to uplift to Beta too.

Comment 32

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

Yes, please request uplift to Aurora and beta. It's a pretty low risk patch.

Comment 33

[Dominique Vincent [:domivinc]]

Comment on attachment 8704301 [details] [diff] [review]
Update tests

Approval Request Comment
[Feature/regressing bug #]:Closest link and zoomed view features
[User impact if declined]:After a zoom, wrong touch position is used for the closest link and zoomed view features.
[Describe test coverage new/current, TreeHerder]:Test changes are in the patch
[Risks and why]: pretty low risk patch (see comment 32)
[String/UUID change made/needed]:None

Comment 34

[Dominique Vincent [:domivinc]]

Comment on attachment 8702719 [details] [diff] [review]
patch-29122015B 1-Bug_1190541____PositionedEventTargeting_doesn___t_take_into_account_the_zoom_in_out_actions__r_kats.patch

Approval Request Comment
[Feature/regressing bug #]:Closest link and zoomed view features
[User impact if declined]:After a zoom, wrong touch position is used for the closest link and zoomed view features.
[Describe test coverage new/current, TreeHerder]:Test changes are in the patch
[Risks and why]: pretty low risk patch (see comment 32)
[String/UUID change made/needed]:None

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8704301 [details] [diff] [review]
Update tests

Taking it in aurora as it is early in the cycle.
However, I don't think we want to take in beta as it is late in the cycle and we shipped several releases with this bug.

Comment 36

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8702719 [details] [diff] [review]
patch-29122015B 1-Bug_1190541____PositionedEventTargeting_doesn___t_take_into_account_the_zoom_in_out_actions__r_kats.patch

I am denying this based on the more stringent Beta44 uplift criteria that allows only for fixing critical regressions, sec and stability issues. To me this fix, does not meet that bar. Thanks!

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7960c4badee
https://hg.mozilla.org/releases/mozilla-aurora/rev/3b8540eb98d1