1172488 - Small elements are wrongly detected by the cluster detection process and trigger the zoomed view

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

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

Comment 1

[Dominique Vincent [:domivinc]]

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).

Comment 2

[Dominique Vincent [:domivinc]]

Attachment: patch-09062015TER 2-Bug_1172488___Zoomed_View_activates_on_drop_up_down_menu__r_kats.patch

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?

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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?

Comment 4

[Dominique Vincent [:domivinc]]

Teodora, could you test the fix using the following APK:

http://rusez.com/apk/bug1172488/fennec-41.0a1.en-US.android-arm.apk

Comment 5

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

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.

Comment 6

[Dominique Vincent [:domivinc]]

Thanks Kats, I'm going to explore the nsIContent-land ...

Comment 7

[Dominique Vincent [:domivinc]]

Attachment: patch-12062015 2-Bug_1172488___Small_clickable_text_nodes_are_wrongly_detected_in_cluster_detection_process__r_kats.patch

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 8

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

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

Comment 9

[Dominique Vincent [:domivinc]]

Attachment: patch-12062015 2-Bug_1172488___Small_clickable_text_nodes_are_wrongly_detected_in_cluster_detection_process__r_kats.patch

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?

Comment 10

[Teodora Vermesan (:TeoVermesan)]

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

Comment 11

[Dominique Vincent [:domivinc]]

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).

Comment 12

[Dominique Vincent [:domivinc]]

https://tbpl.mozilla.org/?tree=Try&rev=ff67cb63f8f5

Comment 13

[Dominique Vincent [:domivinc]]

rc5 test is now green (done with bug 1165127) here:
https://tbpl.mozilla.org/?tree=Try&rev=aead71040921

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a6cf5aa6f5f1

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a6cf5aa6f5f1