Closed Bug 1172488 Opened 5 years ago Closed 5 years ago
Small elements are wrongly detected by the cluster detection process and trigger the zoomed view
Steps to reproduce: 1. Go to http://www.worldclass.ro/en/health-academy-cluj/schedule 2. Tap "Log in your world class account" Expected results: - The drop down menu "Login your account" should appear Actual results: - The zoomed view is displayed Note: - please see the following video: https://www.youtube.com/watch?v=crnYPdq0R58&feature=youtu.be
The html page uses a jscript library: cufon.js. This library transforms text elements in canvas elements. The visible part is the canvas element. The text element is invisible: font size set to 1px, line height of 1px, width and height set to 0. The library keeps the text element just in case of text selection, the user can still copy the text. A text with a font size set to 1px is considered unreadable by the cluster detection. It’s the reason why the zoomed view is displayed. The issue is in the test of the font size, it should be done only for clickable text element. The test on the font size should not be done for other clickable elements (images, canvas, …). I will provide a fix just after bug 1171731 (the issue is on the same line).
Assignee: nobody → domivinc
The zoomed view is no more displayed when the clicked link is composed by different elements (canvas, text, ...) and not only text. The top menu of the reddit site is still correctly handle: the links in the menu are composed by text only elements. When the user clicks in the top menu, if the font size is too small to be readable, the zoomed view is displayed. The fix of bug 1171731 is also included in this patch. Michael, do you want to make the functional review of this change?
Dominique, can you update the title of the bug so it better describes the issue? (In reply to Dominique Vincent [:domivinc] from comment #2) > The fix of bug 1171731 is also included in this patch. Once this lands, you can dupe bug 1171731 to this one and give a comment saying the issue was fixed in this bug. (Not sure if we've had a situation like this before so sorry if this is redundant) > Michael, do you want to make the functional review of this change? It's ideal if the reporter of the bug tries your build and sees if it fixes the issue because they're confident in the repro steps. Can you post a link to your build and NI Teodora?
Flags: needinfo?(michael.l.comella) → needinfo?(domivinc)
Teodora, could you test the fix using the following APK: http://rusez.com/apk/bug1172488/fennec-41.0a1.en-US.android-arm.apk
Flags: needinfo?(domivinc) → needinfo?(teodora.vermesan)
Summary: Zoomed View activates on drop up/down menu → Small elements are wrongly detected by the cluster detection process and trigger the zoomed view
Comment on attachment 8617567 [details] [diff] [review] patch-09062015TER 2-Bug_1172488___Zoomed_View_activates_on_drop_up_down_menu__r_kats.patch Review of attachment 8617567 [details] [diff] [review]: ----------------------------------------------------------------- I don't think this is right; you are checking the frame tree instead of the content tree which is what I would expect. A single text node in the DOM can get broken up into multiple frames, I think. But I am not really that familiar with the frame tree structure. Still based on the comments I think it would be better to do the check in nsIContent-land rather than nsIFrame-land.
Thanks Kats, I'm going to explore the nsIContent-land ...
I made the search of the text node in the content tree. It should fix the issue regarding the split of one text node in multiple frames.
Comment on attachment 8621633 [details] [diff] [review] patch-12062015 2-Bug_1172488___Small_clickable_text_nodes_are_wrongly_detected_in_cluster_detection_process__r_kats.patch Review of attachment 8621633 [details] [diff] [review]: ----------------------------------------------------------------- This looks ok to me, thanks! ::: layout/base/PositionedEventTargeting.cpp @@ +391,5 @@ > + * if the text inside the frame is readable (by human eyes) > + * or > + * if the structure is too complex to determine the size. > + * In both cases, the frame is considered as clickable. > + * nit: trailing whitespace
Attachment #8621633 - Flags: review?(bugmail.mozilla) → review+
Kats, trailing whitespace removed. Teodora, I just updated the following APK with the last version of code: http://rusez.com/apk/bug1172488/fennec-41.0a1.en-US.android-arm.apk Could you make the functional review of the fix using this APK?
Attachment #8621633 - Attachment is obsolete: true
I've installed the apk: http://rusez.com/apk/bug1172488/fennec-41.0a1.en-US.android-arm.apk and set up sync. After "Synced Tabs" panel is populated, "Unfortunately, Fennec vincent has stopped" message appears: http://i.imgur.com/f4BYfWU.png On a short exploratory, trying the steps from the description, on a Nexus 4 (Android 5.1.1), "Unfortunately, Fennec vincent has stopped" message appeared several times and firefox closed. Also, tapping the drop down menu, the zoomed view is still displayed: http://imgur.com/ErTRR8G
Teodora, thanks for your time testing the APK. Regarding the issue using sync, there is no change linked to sync in my code to fix your issue. It works without any issue using my HTC with an older version of Android (2.3.3). One possible way to test the fix: try to install the APK without configuring sync. The build should be probably different targeting your device. There are different versions of nightly build. I guess my version is not compatible with your version of Android (regarding Sync functionality). Regarding the zoomed view, the fix doesn't remove the display of the zoomed view. The zoomed view will still be displayed in the following cases: 1- if the touch occurs on a cluster of links, 2- if the elements in the page are considered too small to be readable (html text element using a small font unreadable by human eyes). Your initial description was on the bottom left drop down menu. On this element whatever the size of the drop down menu, the zoomed view was displayed. Now with the fix, it won't be displayed due to the fact that this drop down menu is not a text element but a canvas element. But, for instance, if you open the login popup, you can see a close button (top left of the popup). This text “Close” is a normal text link (not converted in a canvas by the cufon.js library), if this text is considered too small, the zoomed view will be displayed. From a more general point of view, this site is not designed to work on a mobile device. There are many issues: A) With the horizontal menu, it doesn’t work correctly in version 34.0.1 of Firefox for Android (but also with other browsers on my device). When you click on an item of the main horizontal menu (for instance “SPA”), the vertical menu is displayed, but sometimes the page is also reloaded. So you don’t have time to select an item from the vertical menu. B) You can also to try to select the text in a menu, it doesn’t work well (tested with different browsers). C) You can also try to zoom out / zoom in the page in version 34.0.1 of Firefox, the bottom bar won’t be always displayed correctly (using my device). The bar is sometimes transparent. All this issues are linked to the jscript library “cufon.js” used by the page (see my first comment). It looks like that the “cufon.js” library isn’t fully compatible with mobile devices. There is also a very old core issue linked to cufon (Bug 638002). This issue (not fixed) is also due to the transformation done by this cufon library. I’m going to push the code changes, so you will be able to get a nightly version compatible with your device. You will be able to test the initial issue on the drop down menu. The zoomed view should not always be displayed (except in the normal cases).
rc5 test is now green (done with bug 1165127) here: https://tbpl.mozilla.org/?tree=Try&rev=aead71040921
You need to log in before you can comment on or make changes to this bug.