Closed Bug 1190541 Opened 4 years ago Closed 4 years ago
Event Targeting doesn’t take into account the zoom in/out actions
35.18 KB, text/plain
1.15 KB, patch
|Details | Diff | Splinter Review|
6.95 KB, patch
|Details | Diff | Splinter Review|
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.
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).
Attachment #8642606 - Flags: review?(bugmail.mozilla)
(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 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.
Attachment #8642606 - Flags: review?(bugmail.mozilla) → review+
ScaleToResolution() is now tested.
Attachment #8642606 - Attachment is obsolete: true
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
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.
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">
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.
(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)
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  and  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?  https://developer.mozilla.org/en-US/Firefox_OS/Building_the_Firefox_OS_simulator  https://developer.mozilla.org/en-US/Firefox_OS/Automated_testing/Mochitests
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 .  https://developer.mozilla.org/en-US/Firefox_OS/Firefox_OS_build_prerequisites
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.
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).
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.
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.
Same patch rebased.
Attachment #8643391 - Attachment is obsolete: true
(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?
(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.
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?
Yeah, I'm reproducing it locally and will debug. Leaving needinfo on me for now.
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).
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
That didn't work quite so well. Here's another attempt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f488edfc0b5f
FYI I'm still working on this. The event-radius test is quite finicky but I'm getting close to a fix.
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.
Attachment #8704301 - Flags: review?(domivinc)
Attachment #8704301 - Flags: review?(domivinc) → review+
Kats, should we uplift to Aurora? The issue is visible in Aurora, no?
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.
Yes, please request uplift to Aurora and beta. It's a pretty low risk patch.
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 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 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.
Attachment #8704301 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8702719 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Attachment #8702719 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8704301 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.