1126989 - Click on a very small clickable area, doesn't pop up the ZoomedView

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dominique Vincent [:domivinc]]

Followup of bug 663803 (from comment 59 here https://bugzilla.mozilla.org/show_bug.cgi?id=663803#c59):

:mcomella 
I find I still often click a link when I think the zoomed in view will pop up to disambiguate - the "hard to determine" tap metric can be a bit more sensitive (e.g. happens often with the "hot", "new", "rising", etc. tabs on reddit)

:domivinc
The cluster detection is not done because the click is on a clickable element. In this case, the method "GetClosest" in (layout/base/PositionedEventTargeting.cpp) is never called.
One possible fix could be:
- when a clickable element is detected (before the call to GetClosest), test if the link could be considered as a small clickable area or not (using font sizes, area sizes ???),
- in this case, continue the process using the GetClosest method to determine if we are in a cluster of links.

The difficulty here is to define some rules to decide if we are on a small clickable area or not.

Comment 1

[Dominique Vincent [:domivinc]]

Attachment: patch-07022015 1-Bug_1126989___Detect_not_readable_links_to_pop_up_the_zoomed_view_r_kats.patch

Michael, could you test this change on reddit pages and give me your feedback?

Kats, could you have a look at the code (method IsElementClickableAndReadable)? I'm trying to detect very small frames, but I'm not sure that the way I used is the best ( pc->AppUnitsToGfxUnits(clikableSize.width) * cumulativeResolution.width ).

Comment 2

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

Comment on attachment 8560859 [details] [diff] [review]
patch-07022015 1-Bug_1126989___Detect_not_readable_links_to_pop_up_the_zoomed_view_r_kats.patch

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

Using AppUnitsToGfxUnits and cumulative resolution seems correct to me, yeah.

Comment 3

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

Comment on attachment 8560859 [details] [diff] [review]
patch-07022015 1-Bug_1126989___Detect_not_readable_links_to_pop_up_the_zoomed_view_r_kats.patch

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

(In reply to Dominique Vincent [:domivinc] from comment #1)
> Michael, could you test this change on reddit pages and give me your
> feedback?

Nice, Dominique! It is a definite improvement that I would be happy shipping with.

However, I think it can be a bit better - the second smallest row ("hot", "new", etc.) works well but I can make an unambiguous click on a very long word in the smallest row at the top (e.g. "getmotivated") and the zoomed view will still appear.

Comment 4

[Dominique Vincent [:domivinc]]

Attachment: a-12022015 1-Bug_1126989___Detect_not_readable_links_to_pop_up_the_zoomed_view_r_kats.patch

Michael, the way I implemented this fix is based on a minimum size value of the clickable elements (height or width). If "hot" is ambiguous, "getmotivated" is also ambiguous (if both elements have the same height).
On my small device screen, all the top menu options are unreadable in the "reddit" page. Short or long menu options are unreadable due to the very small height of the menu. If you can read the long menu options and not the short options, it's probably because you already know in your mind this part of the "reddit" menu by heart.

In the last patch, I moved the minimum size value in a Pref value. The user will be able to change the value based on his sight accuracy.

Comment 5

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

Comment on attachment 8563585 [details] [diff] [review]
a-12022015 1-Bug_1126989___Detect_not_readable_links_to_pop_up_the_zoomed_view_r_kats.patch

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

The changes in ZoomedView.java and browser.js look unrelated, it might be better to split those into a separate patch. I didn't understand what was going on there so I'll mcomella review that bit.

Minusing for now because the code as-is will look for a closest clickable element even if the target is already clickable (see comment below).

::: layout/base/PositionedEventTargeting.cpp
@@ +395,5 @@
> + * Frames with a too small size will return false:
> + *   in this case, the frame is considered not clickable.
> + */
> +static bool
> +IsElementClickableAndReadable(nsIFrame* aFrame, WidgetGUIEvent* aEvent,bool aTouchClusterDetection)

nit: add a space before "bool"

@@ +407,5 @@
> +  }
> +
> +  const EventRadiusPrefs* prefs = GetPrefsFor(aEvent->mClass);
> +  uint32_t limitReadableSize = prefs->mLimitReadableSize;
> +  nsSize clikableSize = aFrame->GetSize();

Rename this to frameSize

@@ +444,5 @@
>      mozilla::layers::Stringify(aPointRelativeToRootFrame).c_str(), aRootFrame);
>  
>    const EventRadiusPrefs* prefs = GetPrefsFor(aEvent->mClass);
> +  if (!prefs || !prefs->mEnabled) {
> +    PET_LOG("Retargeting disabled\n", target);

Remove the ", target" here

@@ +448,5 @@
> +    PET_LOG("Retargeting disabled\n", target);
> +    return target;
> +  }
> +  if (target && IsElementClickable(target, nsGkAtoms::body) &&
> +      IsElementClickableAndReadable(target, aEvent, prefs->mTouchClusterDetection)) {

Just pass in "prefs" rather than prefs->mTouchClusterDetection. That will save you having to call GetPrefsFor again inside the function. Also I think you should probably structure this like so:

if (target && IsElementClickable(target, nsGkAtoms::body)) {
  if (!IsElementClickableAndReadable(target, aEvent, prefs)) {
    aEvent->AsMouseEventBase()->hitCluster = true;
  }
  PET_LOG(...);
  return target;
}

The reason for this is that in your version, if the target is clickable, we will still walk through the code to find the "closest clickable" element, which is incorrect. If the target is already clickable then we shouldn't be doing that, but you still want the hitCluster stuff to be true if the target is too small. Does that sound reasonable?

::: mobile/android/app/mobile.js
@@ +399,5 @@
>  // When true, zooming will be enabled on all sites, even ones that declare user-scalable=no.
>  pref("browser.ui.zoom.force-user-scalable", false);
>  
>  pref("ui.zoomedview.enabled", false);
> +pref("ui.zoomedview.limitReadableSize", 8);

Document that this pref is in layer pixels.

Comment 6

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

Dominique, just a heads up that I'll be gone until Wednesday - I'll be sure to take a look at your remaining patches when I get back!

Comment 7

[Dominique Vincent [:domivinc]]

Attachment: patch-14022015 1-Bug_1126989___Detect_not_readable_links_to_pop_up_the_zoomed_view_r_kats.patch

Thanks Kats, I made the changes based on your first code review.

Concerning the changes in ZoomedView.java and browser.js, the patch won't work without them. The cluster detection is always done when the user clicks on a link, from the zoomed view or not. When the user clicks in the zoomed view, we don't want to ignore the click. To handle this case, the cluster is detected but not applied using "Gesture:ClickInZoomedView" .

Comment 8

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

Comment on attachment 8564566 [details] [diff] [review]
patch-14022015 1-Bug_1126989___Detect_not_readable_links_to_pop_up_the_zoomed_view_r_kats.patch

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

::: mobile/android/chrome/content/browser.js
@@ +5108,5 @@
>  
>          let data = JSON.parse(aData);
>          let {x, y} = data;
>  
> +        if (this._inCluster && this._clickInZoomedView != true) {

You should probably initialize _clickInZoomedView to something, because the first time this code runs it will be undefined.

Comment 9

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

(In reply to Dominique Vincent [:domivinc] from comment #4)
> Michael, the way I implemented this fix is based on a minimum size value of
> the clickable elements (height or width).

Is it possible to consider a click ambiguous as opposed to the element clicked on? I think this would meet my expectations.

Though if we continue on the "this element is ambiguous" route, we should probably take into account a combination of both width and height because...

> "getmotivated" is also ambiguous (if both elements have the same height).
> On my small device screen, all the top menu options are unreadable in the
> "reddit" page. Short or long menu options are unreadable due to the very
> small height of the menu. If you can read the long menu options and not the
> short options, it's probably because you already know in your mind this part
> of the "reddit" menu by heart.

On my N4, if I put the device close to my face, I can read it. I imagine on higher DPI devices this text is more readable too. The issue is less that the text is not readable, but the smaller links (in width) are not clickable by my fat thumbs; the larger ones are.

Even if I knew the menu by heart, I should be able to click the link.

This is an improvement so I'm fine landing it, but we should continue working to improve the behavior.

Comment 10

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

Comment on attachment 8564566 [details] [diff] [review]
patch-14022015 1-Bug_1126989___Detect_not_readable_links_to_pop_up_the_zoomed_view_r_kats.patch

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

kats r+ wfm.

Comment 11

[Dominique Vincent [:domivinc]]

Attachment: patch-20022015 1-Bug_1126989___Detect_not_readable_links_to_pop_up_the_zoomed_view_r_kats.patch

I added the initialization of _clickInZoomedView

Comment 12

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

Comment on attachment 8566987 [details] [diff] [review]
patch-20022015 1-Bug_1126989___Detect_not_readable_links_to_pop_up_the_zoomed_view_r_kats.patch

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

initialization lgtm w/ kats r+.

Comment 13

[Dominique Vincent [:domivinc]]

Try tests running with bug 1126866 here:
https://tbpl.mozilla.org/?tree=Try&rev=3230eb9e2219

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ecaac2ab5ef