1191041 - Increase the likelihood of "zoomed view" triggering for small elements but decreased the likelihood for large elements

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dominique Vincent [:domivinc]]

This bug is a follow up of bug 1135369 (see comment 26 [1])

Anthony Lam:
Would it be better if we also increase the likelihood of "zoomed view" triggering for text links but decreased the likelihood for <button> / <div> ?  
This would be in addition to increasing it for when "request desktop site" is triggered as well.


Comment from Dominique:
I would prefer to take the size of the elements for the criteria of this heuristic and not the type. 
A click between 2 small buttons should have a likelihood higher than a click between 2 large text links?

Comment 1

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

(In reply to Dominique Vincent [:domivinc] from comment #0)
> Comment from Dominique:
> I would prefer to take the size of the elements for the criteria of this
> heuristic and not the type. 
> A click between 2 small buttons should have a likelihood higher than a click
> between 2 large text links?

Seems reasonable to me.

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

This seems like another one that would help a lot with the issue in bug 1135369 (not appearing at the right times).

Comment 3

[Dominique Vincent [:domivinc]]

Some technical discussions are available here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1191034#c4

The conclusion is to wait before implementing this kind of change. If there is no other way to get a correct behavior from the user point of view, we will work on it.

The idea is the following:
- do not add complexity in the current cluster detection behavior with complex rules. 
- fix the potential bugs we can have in the current code.

Anthony, the current focus should be on testing. And when an issue is detected, try to fix the current process to handle correctly all the different html/xul structures.
You have a good example of this process in bug 1188769 for instance.

Comment 4

[Dominique Vincent [:domivinc]]

Attachment: patch-28082015 2-Bug_1191041___Increase_the_likelihood_of_zoomed_view_triggering_for_small_elements_but_decreased_the_likelihood_for_large_elements__r_kats.patch

Kats, this change introduce a new preference for the zoomed view: "ui.zoomedview.keepLimitSize".
All the elements with a size bigger than this value (x and y) are ignored in the cluster counter. It could "hurt" your rigorous point of view regarding the cluster detection process and how it should work (I think about bug 1188769 for instance). But it seems to give good result...

The idea is the following: when the clickable element detected is a big element, the user probably doesn't want to click on this element. When a user wants to click on a large button, most of the time, the user clicks in the middle of the button. With large button, the touch area is completely inside the button. So if this assumption is correct, this large element should not interfere in the cluster detection process.

The main cases should now work like that:

A) The zoomed view is still displayed with
- 2 small elements in the touch area, counter=2, no change with the current code
- 1 big element and 2 small elements, counter=2, counter=3 before

B) The zoomed view is no more visible with
- 1 small element and 1 big element, counter=1, counter=2 before this patch
- 2 big elements, counter=0, counter=2 before
- 2 big elements and 1 small element, counter=1, counter=3 before
- 1 small element inside a large clickable element, counter=1, counter=2 before

For instance, in the case of bug 1188769, the zoomed view is no more displayed (the size of the buttons is larger than the limit). 

I started with a limit set to the double of the preference "ui.zoomedview.limitReadableSize". If you think that we are now ignoring too many elements, we can change the "ui.zoomedview.keepLimitSize" to a higher value.

Comment 5

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

Comment on attachment 8654107 [details] [diff] [review]
patch-28082015 2-Bug_1191041___Increase_the_likelihood_of_zoomed_view_triggering_for_small_elements_but_decreased_the_likelihood_for_large_elements__r_kats.patch

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

I'd prefer not having to do this but it's not the end of the world. r+ with comments addressed.

::: layout/base/PositionedEventTargeting.cpp
@@ +362,5 @@
>  
> +static bool
> +IsLargeElement(nsIFrame* aFrame, const EventRadiusPrefs* aPrefs)
> +{
> +  uint32_t limitReadableSize = aPrefs->mLimitReadableSize;

Don't need this local var

@@ +433,5 @@
>      // and "for" attribute is present in label element, search the frame list for the "for" element
>      // If this element is present in the current list, do not count the frame in
>      // the cluster elements counter
> +    if ((labelTargetId.IsEmpty() || !IsElementPresent(aCandidates, labelTargetId)) &&
> +        !IsLargeElement(f,aPrefs)) {

nit: space before "aPrefs"

Comment 6

[Dominique Vincent [:domivinc]]

Attachment: patch-28082015 BIS 1-Bug_1191041___Increase_the_likelihood_of_zoomed_view_triggering_for_small_elements_but_decreased_the_likelihood_for_large_elements__r_kats.patch

All changes done.

Comment 7

[Dominique Vincent [:domivinc]]

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

Comment 8

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

Anthony, when this lands in Nightly, can you take a look at the zoomed view behavior and see if it matches your expectations? If not, let's discuss and decide if we want to try out bug 1189973, or another bug?

Dominique, we'll keep the discussion on Bugzilla so you can be looped in as well.

Comment 9

[Dominique Vincent [:domivinc]]

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

Comment 10

[Pulsebot]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Haven't seen the zoomed view since this landed

Comment 13

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

(In reply to Anthony Lam (:antlam) from comment #12)
> Haven't seen the zoomed view since this landed

Have you explicitly tried to activate it? If you can't get it to activate, even when you're trying, perhaps we should adjust the metrics.

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Under the same use-cases of:

Go to CNN.com > Request desktop site > pinch-to-zoom all the way out > scroll down to tap on a specific footer item

and

Go to reddit.com > pinch-to-zoom all the way out > tap on a specific item along the top

I could not trigger it. But, for CNN.com, I did find that I didn't really _need_ it. For reddit header nav though, I did. 

What values are we tweaking here? Perhaps I could be of more assistance if you let me know what values we are measuring and therefore tweaking?

Comment 15

[Dominique Vincent [:domivinc]]

Hi Anthony,

1) Please, verify that you didn’t disable the zoomed view using the menu: Settings -> Display -> “Magnify small areas”. The check box must be checked.

2) Then, could you give us more details about your device (screen size)? I guess you aren’t on a large device screen, because I have exactly the same feeling on my HTC (3.7 inches) : CNN.COM I don’t really need the zoomed view, REDDIT.COM I do.
On my device, it works nice: no zoomed view on cnn, and zoomed view on reddit pages.

3) Could you give us the values available in about:config for:
ui.zoomedview.keepLimitSize
ui.zoomedview.limitReadableSize
ui.mouse.radius.*
ui.touch.radius.*

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Dominique Vincent [:domivinc] from comment #15)
> 2) Then, could you give us more details about your device (screen size)? I
> guess you aren’t on a large device screen, because I have exactly the same
> feeling on my HTC (3.7 inches) : CNN.COM I don’t really need the zoomed
> view, REDDIT.COM I do.
> On my device, it works nice: no zoomed view on cnn, and zoomed view on
> reddit pages.

Nexus 6

> 3) Could you give us the values available in about:config for:
> ui.zoomedview.keepLimitSize

16

> ui.zoomedview.limitReadableSize

8

> ui.mouse.radius.*

true

> ui.touch.radius.*

2

Comment 17

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

Dominique, any thoughts here?

Comment 18

[Dominique Vincent [:domivinc]]

(In reply to Michael Comella (:mcomella) from comment #17)
> Dominique, any thoughts here?

I cannot test with a nexus 6. Using the landscape mode on my device (about 4 in), the zoomed view is displayed on the top menu in reddit.com. The second horizontal menu in reddit.com is probably built with a larger font, in this case, the zoomed view is not always displayed.

You can try with a higher value for “ui.zoomedview.limitReadableSize” (12). In this case the second menu is always considered unreadable. The display of the zoomed view is always done.

Comment 19

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

I can also open the zoomed view on reddit's top bar (GS5, portrait).

Anthony, can you? Do you still feel the zoomed view doesn't appear enough?

Comment 20

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

As per my question in comment 14, what values are we tweaking here? It might help me provide better feedback to you guys. Is this the ui.zoomedview.limitReadableSize?

(In reply to Michael Comella (:mcomella) from comment #19)
> I can also open the zoomed view on reddit's top bar (GS5, portrait).
> 
> Anthony, can you? Do you still feel the zoomed view doesn't appear enough?

on my GS6, portrait, I can't actually get the magnifying glass to show (on the same reddit top bar)... Yes, it's enabled as well.

Comment 21

[Dominique Vincent [:domivinc]]

(In reply to Anthony Lam (:antlam) from comment #20)
> As per my question in comment 14, what values are we tweaking here? It might
> help me provide better feedback to you guys. Is this the
> ui.zoomedview.limitReadableSize?
> 

Anthony, the answer is in comment 18: "You can try with a higher value for “ui.zoomedview.limitReadableSize” (12)."

> 
> on my GS6, portrait, I can't actually get the magnifying glass to show (on
> the same reddit top bar)... Yes, it's enabled as well.

It looks like that the zoomed view is displayed with larger screen sizes (for instance using landscape on smaller devices). In this case, it could be a specific issue linked to the GS6 device. 

Michael, any developer or user with a GS6 device on your side, to confirm the issue on GS6?

Comment 22

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

(In reply to Dominique Vincent [:domivinc] from comment #21)
> Anthony, the answer is in comment 18: "You can try with a higher value for
> “ui.zoomedview.limitReadableSize” (12)."

Anthony?

> Michael, any developer or user with a GS6 device on your side, to confirm
> the issue on GS6?

Kevin, do you have a GS6 or an N6? If so, can you try to open the zoomed view on the top navigation bar of reddit? comment 19 for reference but there's no useful STR there.

If not, I think Brad might have a GS6.

Comment 23

[Kevin Brosnan [Ex-Mozilla]]

I don't have either of those devices.

Comment 24

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Michael Comella (:mcomella) from comment #22)
> (In reply to Dominique Vincent [:domivinc] from comment #21)
> > Anthony, the answer is in comment 18: "You can try with a higher value for
> > “ui.zoomedview.limitReadableSize” (12)."
> 
> Anthony?

Sorry, can you give me some more context to this number? What does increasing it or decreasing it do?

Comment 25

[Dominique Vincent [:domivinc]]

Using this preference value, we want to detect small clickable text elements using the size of the element or the size of the font (for text).
If the size is lower than ui.zoomedview.limitReadableSize, the element is considered "unreadable". In this case, the zoomed view will be displayed. If you increase this value (8 to 12), the zoomed view will be displayed more often.

Comment 26

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Dominique Vincent [:domivinc] from comment #25)
> Using this preference value, we want to detect small clickable text elements
> using the size of the element or the size of the font (for text).
> If the size is lower than ui.zoomedview.limitReadableSize, the element is
> considered "unreadable". In this case, the zoomed view will be displayed. If
> you increase this value (8 to 12), the zoomed view will be displayed more
> often.

Thanks Domivinc!

So, since we're using type size to determine "readability" here. How did we arrive at 8 or 12? My suggestion would be, why don't we set that number to 14 sp?

Since, in the Material guidelines provided by Google [1], the smallest body font size (for their apps) is spec'd at 14sp for devices. 

[1] https://www.google.com/design/spec/style/typography.html#typography-styles

Comment 27

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

Leaving this to Dominique.

Comment 28

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

Anthony, do you have another non-huge device to see if you can activate the zoomed view (comment 19)? I wonder how the device size might be affecting it.

Comment 29

[Dominique Vincent [:domivinc]]

(In reply to Anthony Lam (:antlam) from comment #26)
> So, since we're using type size to determine "readability" here. How did we
> arrive at 8 or 12? My suggestion would be, why don't we set that number to
> 14 sp?
> 
> Since, in the Material guidelines provided by Google [1], the smallest body
> font size (for their apps) is spec'd at 14sp for devices. 
> 
The current value (8) is based on different tests using reddit or twitter sites for instance.
If you think that 14 for ui.zoomedview.limitReadableSize is a better value, and gives good results on the different devices, we can change it. 
When you set the value to 14 on your device, the zoomed view is correctly displayed on the top menu of reddit (displayed only when the text in the top menu is unreadable) ?

Comment 30

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

(comment 28 & comment 29)

Comment 31

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

So I've set it at 14. But I'm still not triggering it on my N6, latest Nightly. 

Is there another variable I can change? Because 14 sounds like it should be the right value here. But its unclear to me how this logic works. 

I've also gone to play with this value a bit. If I play with this on my N6 using Reddit's footer as the testing bed, 20 seems to be the right number.

Domivinc, could you explain a bit more about how this affects the current algorithm?

Comment 32

[Dominique Vincent [:domivinc]]

(In reply to Anthony Lam (:antlam) from comment #31)
> So I've set it at 14. But I'm still not triggering it on my N6, latest
> Nightly. 
> 
> Is there another variable I can change? Because 14 sounds like it should be
> the right value here. But its unclear to me how this logic works. 
> 
> I've also gone to play with this value a bit. If I play with this on my N6
> using Reddit's footer as the testing bed, 20 seems to be the right number.
> 
> Domivinc, could you explain a bit more about how this affects the current
> algorithm?

When you click on a very small clickable element ("unreadable" element), the zoomed view is displayed.
The fact it doesn't work on your N6 could be linked to the very hight resolution display on this device.
The value "ui.zoomedview.limitReadableSize" in the preferences is in "layer pixels". I'm not an expert of the different values used in the code: CSSPixel, LayoutDevicePixel, LayerPixel and ScreenPixel. The issue is probably here in the case of the N6.

Another way to "fix" this issue, is to remove the functionality based on the size heuristic [1]. The zoomed view won't be displayed when a click occurs on a very small link of the reddit page for instance. In this case, the user thinks that the zoomed view doesn't work correctly because the link is unreadable (he wants to read it and then click on it). But We can start without the size heuristic on the first version, and keep trying to improve/fix those issues later.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1208370#c11

Comment 33

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Sorry Domivinc,

I'm having trouble understanding here. Could you explain a little bit about how the current algorithm for determining "when to show the zoomed view" works?

I'll have a look in bug 1208370 as well.

Comment 34

[Dominique Vincent [:domivinc]]

(In reply to Anthony Lam (:antlam) from comment #33)
> Sorry Domivinc,
> 
> I'm having trouble understanding here. Could you explain a little bit about
> how the current algorithm for determining "when to show the zoomed view"
> works?
> 
> I'll have a look in bug 1208370 as well.

Added an explanation in bug 1208370 comment 13. 
But please, do not close this bug, if we keep the size heuristic behind a preference value to experiment and improve it, we will have to find a way to get the same "limitReadableSize" value on all the devices (N6 included).