1208370 - Disable zoomed view size heuristic (formerly: Magnifying glass destroys form UX)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul [pwd]]

Attachment: tmp_2651-Screenshot_2015-09-25-09-09-37-256921620.png

User Agent: Mozilla/5.0 (Android 5.1.1; Tablet; rv:44.0) Gecko/44.0 Firefox/44.0
Build ID: 20150924030231

Steps to reproduce:

Attempting to post on a forum, I tap the huge post reply button and the magnifying glass appears. No matter how many times I dismiss it, it reappears. The magnifying glass should have very simple rules. If the area tapped plus a little extra (greedy zone) contains more one tappable item, open magnifying glass. My experience these past few months of testing are horrendous. It appears seemingly whenever it wants and this is another example. There's absolutely nothing ambiguous about the post reply button.

Comment 1

[Mark Capella [:capella]]

Does it help that Bug 1189971 added a Settings -> Display -> Magnifying glass on/off toggle (?)

Comment 2

[Paul [pwd]]

Not really because the magnifying glass should work to facilitate the navigation of a user in areas of ambiguous links. The logic is supposed to be simple, if the area of tap plus buffer contains more than two tap-able items, display the magnifying glass. Turning it off doesn't fix the issue but rather sweeps it under the carpet whereby it's likely to just remain a blot on otherwise really good product. It's like the text inflation, rather than inflate _ALL_ text of a hierarchy level and _ALL_ things below, it seems to have additional logic which mars the overall user experience. It's like The Major says, overspecialise and you breed in weakness. That's what we're doing. We're overcomplicating features to the point they're fundamentally broken and then the solution becomes that of  disabling the feature to work around the short comings. We're now filing bugs whereby if the logic was correct, we shouldn't ever need to disable the magnifying glass manually anyway (see bug 1189973).

Comment 3

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

Thanks for the feedback, Paul. We're trying to fix the issue in bug 1135369 – "Zoomed view does not appear when expected". 

I previously suggested your algorithm in bug 1135369 comment 19 and there were responses but I'm not entirely clear why it's more complicated than that – let me talk to the feature owner and see what we can do.

Comment 4

[Dominique Vincent [:domivinc]]

Michael, I think this bug is valid and is different of bug 1135369 comment 19. We should try to find a solution on the technical side (to avoid the "solution": disable the feature using the settings menu).

Based on the attached picture, the issue is visible here [1]. The html code of the page looks like the following:

<div data-theme="a" class="ui-btn ui-btn-up-a ui-btn-corner-all ui-shadow" aria-disabled="false">
<span class="ui-btn-inner ui-btn-corner-all" aria-hidden="true">
<span class="ui-btn-text">Log in</span></span>
<button type="submit" name="sbutton" data-theme="a" class="ui-btn-hidden" aria-disabled="false">Log in</button></div>

The button is duplicated by a span element. It's probably the reason why the cluster is detected: the span and the button are probably both "clickable" elements.

Feel free to re-open this bug and I will trace the issue to try to find a technical solution.

[1] http://forum.supercell.net/mobile.php?do=login

Comment 5

[Dominique Vincent [:domivinc]]

Attachment: patch-27092015 Bis 1-Bug_1208370___Buttons_with_hidden_text_are_incompatible_with_the_zoomed_view__r_cats.patch

Michael, I found the reason of the issue. 

The text of the button is invisible due to the following css rules:
opacity: .1; 
font-size: 1px; 
text-indent: -9999px; 

The visible text comes from the span element (.ui-btn-text). This span element is positioned to be displayed in place of the button text.

When the user clicks on the button, the button is detected like a small unreadable element due to the font size (1px). It's the reason why the zoomed view is displayed.

The bad news is that the library using the previous trick to hide the text of the button is jquery-mobile. The site supercell is using an old version (1.0) but this trick is still used in the last version of the jquery-mobile library [1].

We already had a similar problem with another library (see bug 1172488). The text elements were transformed into Canvas elements.
I reinforced the current rule used to detect unreadable elements. If the element is a button, the font size is no more relevant. 

The tricks used by those libraries are really not perfect. We can see the issue with the zoomed view. But the highlight element behavior is also broken with the html structure used by the libraries.
The purpose of the jquery-mobile library is to facilitate the development of mobile sites compatible with all the devices/browsers. The price is the html structure of the page that is totally outside of the html5 standard (the library hides and replaces the text of the html button by the text of a span ...). There is also a cost in term of performances, one simple html button is surrounded by 2 or 3 elements to get the text correctly positioned. It looks strange that the jquery mobile team promotes such html design.

[1] http://demos.jquerymobile.com/1.3.2/widgets/buttons/

Comment 6

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

Comment on attachment 8666453 [details] [diff] [review]
patch-27092015 Bis 1-Bug_1208370___Buttons_with_hidden_text_are_incompatible_with_the_zoomed_view__r_cats.patch

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

I'm not a huge fan of this because it's so specific but I can r+ it if you write a test for it, ideally just a simplified version of the page in question. Also s/cats/kats/ in the commit message :)

Dropping flag for now, re-request review with the test please.

Comment 7

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

I feel strange that we're using font readability as a metric for the zoomed view (especially given all of the special fixes that we've needed to come up with). While I think the "magnification" aspect of the zoomed view does help to make non-readable text readable, I think the primary issue here is whether or not a click is ambiguous (which does not relate to text size, but rather, element size, unless I'm overlooking something).

Dominique, Kats – any thoughts?

Comment 8

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

Honestly it feels like we're playing whack-a-mole with this feature now. The initial simple algorithm I thought worked ok but then to deal with all sorts of different scenarios we made the algorithm more and more complex. That's not necessarily bad, but it's getting to the point where any change carries a high risk of undoing previous changes, so we're just going to go around in circles unless we have a good bank of regression tests. Hence my request for tests for any future changes to this algorithm.

As for the text-size heuristic, that was added back in bug 1126989 - I don't have an opinion one way or another on whether we should do it. I reviewed the code but only from a code-correctness point of view; if it's not something we want then we can remove the entire text-size heuristic and just stick with the basic "ambiguous click" algorithm. Who is the person who will be making the final call on whether or not the feature is "good enough to ship"? That person should decide if the zoomed view should pop up on unreadably small links or not.

Comment 9

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Who is the person who will
> be making the final call on whether or not the feature is "good enough to
> ship"? That person should decide if the zoomed view should pop up on
> unreadably small links or not.

I'd say Anthony currently has the say, but Barbara/Kar may also have some input.

Comment 10

[Paul [pwd]]

Attachment: tmp_12808-Screenshot_2015-09-28-21-47-49-256921620.png

While somewhat off topic, can I add that the current implementation is even showing for imgur's navigation drawer. On top of that and just as frustrating, it doesn't disappear upon the ambiguous link being selected either. This feature has fallen so far from its design goal that it simply doesn't even make sense any more. The original bug was very succinct in what we were trying to achieve, what's happened?

Comment 11

[Dominique Vincent [:domivinc]]

(In reply to Michael Comella (:mcomella) from comment #7)
> I feel strange that we're using font readability as a metric for the zoomed
> view (especially given all of the special fixes that we've needed to come up
> with). While I think the "magnification" aspect of the zoomed view does help
> to make non-readable text readable, I think the primary issue here is
> whether or not a click is ambiguous (which does not relate to text size, but
> rather, element size, unless I'm overlooking something).
> 
> Dominique, Kats – any thoughts?

Michael, It's ok to remove the size heuristic (frame size and font size). But in this case, you won't get the zoomed view when the click occurs on a very small link of the reddit page for instance or a small radio button in a form.
I can make this change. You will be able to test the difference.

In any case, it won't help for the highlight element behavior that is also broken with the html structure used by the libraries.

Comment 12

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Honestly it feels like we're playing whack-a-mole with this feature now. The
> initial simple algorithm I thought worked ok but then to deal with all sorts
> of different scenarios we made the algorithm more and more complex. That's
> not necessarily bad, but it's getting to the point where any change carries
> a high risk of undoing previous changes, so we're just going to go around in
> circles unless we have a good bank of regression tests. Hence my request for
> tests for any future changes to this algorithm.

I completely agree, Kats. I've attempted to triage the list with Barbara and provide some guidance for a "V1" in bug 1198463. That's my UX "line in the sand" as of this moment.

> As for the text-size heuristic, that was added back in bug 1126989 - I don't
> have an opinion one way or another on whether we should do it. I reviewed
> the code but only from a code-correctness point of view; if it's not
> something we want then we can remove the entire text-size heuristic and just
> stick with the basic "ambiguous click" algorithm. 

I agree, I'm still having trouble understanding what logic we're using here and what the algorithm takes into account. Domivinc, can you clarify please? 

> Who is the person who will
> be making the final call on whether or not the feature is "good enough to
> ship"? That person should decide if the zoomed view should pop up on
> unreadably small links or not.

I'm not actually running into it most of the time now and that's OK. But I would like to understand the algorithm to make the call. The logic seems really convoluted in the form of many bugs to me

Comment 13

[Dominique Vincent [:domivinc]]

(In reply to Anthony Lam (:antlam) from comment #12)
> > As for the text-size heuristic, that was added back in bug 1126989 - I don't
> > have an opinion one way or another on whether we should do it. I reviewed
> > the code but only from a code-correctness point of view; if it's not
> > something we want then we can remove the entire text-size heuristic and just
> > stick with the basic "ambiguous click" algorithm. 
> 
> I agree, I'm still having trouble understanding what logic we're using here
> and what the algorithm takes into account. Domivinc, can you clarify please? 
> 

I'm not sure we can talk about an algorithm here, it's only 2 tests (the full story is here: bug 1126989): 
- first the size of the clickable element is tested, under "ui.zoomedview.limitReadableSize" the element is considered to be unreadable.
- second for text elements / and elements with one text children, the font size is tested. Under "ui.zoomedview.limitReadableSize" the element is considered to be unreadable.

When a click occurs on an unreadable element, the zoomed view is displayed.

The issues are not in the 2 previous tests. The issues are in the html structures generated by some libraries in the pages. For instance, a jscript library can transform a simple html button into a complex structure where a span is displayed over the button, and the text of the button is hidden to the user but not to the browser using different techniques (opacity, font size, text-indent, ...). In this case, the tests fail because of the trick used to hide the button text. The highlight behavior also fails in this case.
We can add exceptions to detect those libraries (see bug 1172488 for the first one) but we cannot be sure that another library uses another trick to hide element to the user and not to the browser.

Could I suggest that we deactivate the size heuristic for the V1? But keep it in the code like an experimental behavior that the user can activate with the "ui.zoomedview.simplified" set to false. It's already the way we manage the zoom buttons. If the user change "ui.zoomedview.simplified" to false, the zoom buttons are displayed, and with my proposal, the size heuristic will be used to detect small elements.

Comment 14

[Dominique Vincent [:domivinc]]

Attachment: patch-30092015 1-Bug_1208370___Deactivate_the_size_heuristic_in_cluster_detection__r_kats.patch

This is the patch to deactivate the size heuristic when the pref "ui.zoomedview.simplified" is true (default for all the users).

It doesn't fix the issue when the size heuristic is on (jquery-ui button fix in the other patch).

Comment 15

[Dominique Vincent [:domivinc]]

Anthony, Michael, could you give me your feedbacks about my suggestion in comment 13. And if it's ok for you, I will ask for a code review for the patch attached in comment 14.
This suggestion doesn't mean that the bugs of the size heuristic won't be fixed but we will have more time to deal with them.

Comment 16

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

I'll get back to you tomorrow, Dominique.

Comment 17

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

I don't understand why a size heuristic is necessary but after reading through bug 1126989, I think this may be a difference in interpretation of the algorithm. In particular, in bug 1126989 comment 9 I ask:

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

In my head, I've been thinking as this algorithm being about ambiguous clicks and not ambiguous elements, but perhaps you've been thinking about ambiguous elements?

I spoke with Anthony and a few others to come up with a high level algorithm that makes sense to me. We came up with:

When the user clicks a point, we create a circle around that point representing the user's finger. If it's a stylus, we ignore the press. We expand the press using values from MotionEvent.getTouch*. If the circle intersects more than 1 link, we call the click ambiguous and provide the zoomed view.

Caveats:
* MotionEvent.getTouch* gives an ellipse, not a circle – not sure what to do with that
* MotionEvent.getTouch* does not give units in the documentation – it's likely pixels (but why is it a float?), but it can be dp, normalized over 1, etc.
* In kats' experience, most devices' hardware does not give a size for the pressed state so MotionEvent.getTouch* is useless – perhaps talk with him to determine if it's worth exploring this option.
* The ambiguous radius will likely need to be tweaked through experimentation
* If the click would be ambiguous (e.g. it did not land on a view to open) but there is one and only one link in the ambiguous radius, I'd argue we open it. Seems like a natural next step for this algorithm.
---

Dominique, does this seem like a reasonable way to think about the algorithm? Is it implementable? Would you take this approach? Why or why not?

Note: since Kats is closer to the code, he agreed to help out with technical mentorship of the algorithm's implementation. I'm going to NI him in case there's anything else he wants to add though it seems he's PTO until mid/late-October. :\ I'll do my best in the meantime.

Comment 18

[Dominique Vincent [:domivinc]]

> "Is it possible to consider a click ambiguous as opposed to the element
> clicked on? I think this would meet my expectations."
> 
> In my head, I've been thinking as this algorithm being about ambiguous
> clicks and not ambiguous elements, but perhaps you've been thinking about
> ambiguous elements?

Yes, the size heuristic (clicked element size and font size in case of element with text) comes from this point.
And there is no link with the size of the touch area defined by the 5 following preferences: 
ui.mouse.radius.enabled = true
ui.mouse.radius.leftmm = 3
ui.mouse.radius.rightmm = 3
ui.mouse.radius.topmm = 5
ui.mouse.radius.bottommm = 2

You can also use ui.touch.radius values, if you set ui.touch.radius.enabled to true.
 
> I spoke with Anthony and a few others to come up with a high level algorithm
> that makes sense to me. We came up with:
> 
> When the user clicks a point, we create a circle around that point
> representing the user's finger. If it's a stylus, we ignore the press. We
> expand the press using values from MotionEvent.getTouch*. If the circle
> intersects more than 1 link, we call the click ambiguous and provide the
> zoomed view.
> ...

This part is already implemented. This touch area algorithm and the the size heuristic (coming from bug 1126989 comment 9) are two totally different parts of the code.
This touch area algorithm is used to find the closest link near the touch point. This algorithm is not new (not implemented for the cluster detection). The distances between the touch point and the different links inside the touch area are used to determine the closest link. 
The cluster detection is done in the same part of the code (the same loop) because the 2 behaviors are linked (the links are the same). Several bugs have been detected/fixed in this touch area algorithm using the zoomed view because the bugs are more visible... 

From my point of view, I found only two small differences between the current algorithm and your proposal:

A) the stylus case is not detected (I didn't find any line of code linked to this case). 
And I'm not sure that Android is able to detect the difference between a finger touch and a stylus touch.

B) the touch area is not a circle but a rectangle (nsRect). The size of the rectangle is fixed by the preferences (ui.mouse.radius.*) in mm and not dp.
If you want to change the form and the size of the touch area, we can do it. But it will also be done for the search of the closest link in order to be coherent. The main issue with the circle is that it will cost a lot in calculation to get all the elements in the circle. It's probably the reason why a rectangle is currently used. Another point in favor of the rectangle, taking into account the small dimension of the touch area, a rectangle is a good approximation of a circle.

> 
> Dominique, does this seem like a reasonable way to think about the
> algorithm? Is it implementable? Would you take this approach? Why or why not?
> 

To get your design (without points A and B) the only thing to do is the deactivation of the size heuristic (the patch attached in comment 14).
We can open 2 new bugs to investigate in details the points A and B. But those 2 changes are not exclusively linked to the cluster detection (the zoomed view). They will also impact the closest link detection algorithm.

Again, I would suggest to start with the patch in comment 14 and see how it works on the reddit page (bug 1126989 comment 9 comes from the tests in this page).

Comment 19

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

Thanks for the overview, Dominique! That was very helpful.

However, I'm still unclear what role the cluster detection plays in the algorithm – can you fill me in on those details?

(In reply to Dominique Vincent [:domivinc] from comment #18)
> To get your design (without points A and B) the only thing to do is the
> deactivation of the size heuristic (the patch attached in comment 14).

Sure, then let's try it! You suggested it before too and I like that. :)

> We can open 2 new bugs to investigate in details the points A and B. But
> those 2 changes are not exclusively linked to the cluster detection (the
> zoomed view). They will also impact the closest link detection algorithm.

The connection is unfortunate but it is worth investigating, I think – can you file?

> Again, I would suggest to start with the patch in comment 14 and see how it
> works on the reddit page (bug 1126989 comment 9 comes from the tests in this
> page).

Sounds reasonable.

Comment 20

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

Comment on attachment 8667844 [details] [diff] [review]
patch-30092015 1-Bug_1208370___Deactivate_the_size_heuristic_in_cluster_detection__r_kats.patch

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

I don't have the context on the code to say this is exactly correct (if kats was not on PTO, he'd probably be reviewing it), but given the method names and my skimming, this approach looks reasonable.

Comment 21

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

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

Comment 22

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

Does the mm used in the hit area size properly scale with Android screen size as dp does? I remember Anthony having issues activating the view on his larger device (N6?).

Also, I remember us talking about saloon doors previously and the issue of the zoomed view activating when clicking on or between elements. What is the current state? Will the zoomed view appear both when clicking on and between elements?

---
Using this patch on my N4, I always activate the zoomed view on the top navigation bar in Reddit and can never activate it on the navigation one level above that. Unclear if this is desired (I'll have to try it out more).

The build if anyone else wants to try it:
  https://people.mozilla.com/~mcomella/apks/dominique-1208370_01.apk

Comment 23

[Dominique Vincent [:domivinc]]

(In reply to Michael Comella (:mcomella) from comment #22)
> Does the mm used in the hit area size properly scale with Android screen
> size as dp does? I remember Anthony having issues activating the view on his
> larger device (N6?).
> 

It should for 2 reasons:
- the closest link behavior is using the same mm values to define the touch point area,
- the touch area size is not linked to the screen characteristics (size, resolution, ...) but to the estimation of the touch area of a finger. I guess that your finger is not already smart enough to adapt the size of its touch point based on the device used?

If the mm values work for the closest link behavior, it should work the same for the cluster detection. There is only one touch area (same size and position) in both cases. And in both cases, we are looking for all the links inside the touch area.

The issue in your case could come from the bug 1190541. The patch attached in this bug works for me, but there is a pending failing test on the b2g side. It's the reason why the code is not yet used on nightly version. 
You could test this patch and see how it works.


> Also, I remember us talking about saloon doors previously and the issue of
> the zoomed view activating when clicking on or between elements. What is the
> current state? Will the zoomed view appear both when clicking on and between
> elements?
> 

You can test some cases using [1]. You can also add or change the test cases of this html file to see how it works with the "saloon doors" case. This test case was used in bug 1181763 comment 17. 

> ---
> Using this patch on my N4, I always activate the zoomed view on the top
> navigation bar in Reddit and can never activate it on the navigation one
> level above that. Unclear if this is desired (I'll have to try it out more).
> 
> The build if anyone else wants to try it:
>   https://people.mozilla.com/~mcomella/apks/dominique-1208370_01.apk

On my device, using your apk it works great, on top menu, and on the second level top menu too of Reddit.
The zoomed view is displayed when the click doesn't occur on a link but the touch area contains 2 links.
You can see it in this video [2].
At the end of the video, the last touch point is done in the middle of the link "controversial". In this case, the zoomed view is not displayed, the page "controversial" is displayed.


I have the feeling that the behaviors (closest link detection and cluster links detection) works differently on some devices. Could I suggest the following ways to clarify the current situation:

A) Test with the patch in bug 1190541 comment 4

B) One way to understand the issue is to post a video of your failing tests with a short explanation giving the touch point position. 

C) Another way to move on, you can also build a test case of your issue using [2] as a model.

D) Make additional tests on your device with different touch area sizes. you can increase the size of the touch area using bigger values for:
ui.mouse.radius.enabled = true
ui.mouse.radius.leftmm = 3
ui.mouse.radius.rightmm = 3
ui.mouse.radius.topmm = 5
ui.mouse.radius.bottommm = 2


[1] https://bug946731.bmoattachments.org/attachment.cgi?id=8595972

[2]https://youtu.be/NFq0ZSEPLtI

Comment 24

[Dominique Vincent [:domivinc]]

(In reply to Michael Comella (:mcomella) from comment #19)
> Thanks for the overview, Dominique! That was very helpful.
> 
> However, I'm still unclear what role the cluster detection plays in the
> algorithm – can you fill me in on those details?
> 
A simplified view of the current implementation in the code:
1- The closest link algorithm build a list of all the links inside the touch area, and keep the closest link using the following criteria: the distance between the touch point and the link. 
2- The cluster detection code counts the number of links inside the touch area (using the list built in point 1), and at the end, if the counter is higher than 1, returns the information "the click occurs inside a cluster".

The connection between the 2 behaviors is reasonable (from my point of view): the touch area is the same, the list of links is the same, and it will be useless to make twice the costly process to build the list of links.

> > We can open 2 new bugs to investigate in details the points A and B. But
> > those 2 changes are not exclusively linked to the cluster detection (the
> > zoomed view). They will also impact the closest link detection algorithm.
> 
> The connection is unfortunate but it is worth investigating, I think – can
> you file?
> 

I will do it. I keep the NI for this point.

Comment 25

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

https://hg.mozilla.org/integration/fx-team/rev/58b109d03b1fdf05ed6da1930ef632b0b859cc47
Bug 1208370 - Deactivate the size heuristic in cluster detection. r=mcomella

Comment 26

[Dominique Vincent [:domivinc]]

Bug 1213756 added (regarding the stylus case and the form of the touch area).

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58b109d03b1f

Comment 28

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

Anthony, this should be in tomorrow's Nightly – let us know what you think of the zoomed view behavior.

Comment 29

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

Hey guys! great work! this feels like we're almost there.

I've had this enabled on my devices N5 and N6 since comment 28 but I'm still having trouble triggering it when I need it. 

I'm not sure how much of this has to do with us going with mm rather than dp. But if we are going with "mm", what is the size of the "area" we are drawing around the touch point to simulate a finger? 

I'd suggest we go with 14mm in accordance with this MIT touch lab study [1].

[1] http://touchlab.mit.edu/publications/2003_009.pdf

Comment 30

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

^domivinc, can we try updating with this? I noticed from your comments that your "shape" is not symmetrical and I don't see a reason why it shouldn't be.

Let's see how this feels.

Comment 31

[Dominique Vincent [:domivinc]]

(In reply to Anthony Lam (:antlam) from comment #29)
> 
> I'm not sure how much of this has to do with us going with mm rather than
> dp. But if we are going with "mm", what is the size of the "area" we are
> drawing around the touch point to simulate a finger? 
> 
> I'd suggest we go with 14mm in accordance with this MIT touch lab study [1].
> 

The change regarding the touch area in "mm" comes from bug 788073 and is not linked to the zoomed view implementation (see the diff here [1]). Prior this old bug, the touch radius was defined in 1/240-inch pixels:

// Touch radius (area around the touch location to look for target elements),
// in 1/240-inch pixels:
pref("browser.ui.touch.left", 32);
pref("browser.ui.touch.right", 32);
pref("browser.ui.touch.top", 48);
pref("browser.ui.touch.bottom", 16);

I have no idea of the reason of the unit change. But the shape wasn't symmetrical (top:48 <> bottom:16).

We can use a symmetrical shape of 14mm. You can test it on your different devices using the following preferences (in about:config) :
ui.mouse.radius.enabled = true
ui.mouse.radius.leftmm = 7
ui.mouse.radius.rightmm = 7
ui.mouse.radius.topmm = 7
ui.mouse.radius.bottommm = 7

After your different tests, if it looks good, we will define those values by default in the code.


[1] https://hg.mozilla.org/mozilla-central/rev/c8e8e389d84f

Comment 32

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

It's looking better

On my N5, these new settings for ui.mouse.radius works great. A bit on the conservative side but I think its a good starting point. I had no issue triggering it on the Reddit top level nav.

On my N6, the same Reddit nav did not trigger it. Nor did the cnn.com footer on after I "Request desktop site".

What's the best way to ensure a consistent experience across diff device screens and sizes here Domivinc?

Comment 33

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

Anthony, do the elements on the page appear larger on the N6 than the N5? If so, doesn't that mean that the zoomed view could be expected to appear on the N5 on the reddit top nav, but not on the N6? If the page elements and the spaces between them are different sizes, it's unclear what it means to be consistent.

If it's not doing what you expect on the N6, while lacking a bit of context, I think the appropriate course of action would be to increase the size of the hit box.

Comment 34

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

You're right, it is bigger (although ever so slightly).

I did also increase it to 16mm and ultimately 18mm as well but still didn't trigger it on the N6. On the N5, 14 and 16 seem to be the same.

But these new settings do seem to work a lot better for the N5. I'd suggest going with 16mm for now.

Comment 35

[Dominique Vincent [:domivinc]]

Attachment: patch-01112015 1-Bug_11208370___New_size_for_the_touch_area___r_mcomella.patch

Michael, I made the changes in the pref using the values proposed by Anthony.
I cannot test those settings using my device (Android 2.3.3), the nightly code is currently totally broken: firefox vertical menu is unreadable, wrong size for the search pictures in about:config, and the zoomed view is never displayed.

The issue regarding the zoomed view could come from a change on DrawTarget in Gecko code. I can see the following messages in the log just after the call to create the bitmap used by the zoomed view:

11-01 07:47:03.525: I/Gecko(7084): [GFX1]: Attempt to create DrawTarget for invalid surface. Size(240,240) Cairo Status: 24
11-01 07:47:03.525: I/Gecko(7084): [GFX1-]: Failed to create DrawTarget, Type: 4 Size: Size(240,240), Data: 0x, Stride: 32

Comment 36

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

(In reply to Dominique Vincent [:domivinc] from comment #35)
> the nightly code is currently totally broken: firefox vertical menu is unreadable, wrong
> size for the search pictures in about:config, and the zoomed view is never
> displayed.

On my GS4, the zoomed view works correctly – is this a 2.3 problem? Maybe the change got backed out. Let me know if you find out more (or need help finding out more).

Comment 37

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

Comment on attachment 8681674 [details] [diff] [review]
patch-01112015 1-Bug_11208370___New_size_for_the_touch_area___r_mcomella.patch

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

I didn't dig deeply enough to identify the side effects of this but I think kats would be more appropriate here.

::: mobile/android/app/mobile.js
@@ +421,5 @@
>  pref("ui.zoomedview.simplified", true); // Do not display all the zoomed view controls, do not use size heurisistic
>  
>  pref("ui.touch.radius.enabled", false);
> +pref("ui.touch.radius.leftmm", 8);
> +pref("ui.touch.radius.topmm", 8);

I see in the bug this is last implemented in, it's mentioned we should keep the skew towards the top:
  https://bugzilla.mozilla.org/show_bug.cgi?id=788073#c3

kats, do you still think we should keep that skew?

Comment 38

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

Comment on attachment 8681674 [details] [diff] [review]
patch-01112015 1-Bug_11208370___New_size_for_the_touch_area___r_mcomella.patch

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

This is a UX/product decision so I'm deferring to antlam. See also bug 1220770 which is trying to change some of this. Personally I do think we should keep the skew.

Comment 39

[Dominique Vincent [:domivinc]]

Attachment: issues.2.3.png

(In reply to Michael Comella (:mcomella) from comment #36)
> 
> On my GS4, the zoomed view works correctly – is this a 2.3 problem? Maybe
> the change got backed out. Let me know if you find out more (or need help
> finding out more).

It's probably a 2.3 problem. The issues are still visible with the last nightly version of the code. You can see in the attached picture the different visible issues and also the log error when the zoomed view is triggered.

Are there some changes on the Gecko side regarding the display of bitmap using DrawTarget?

Comment 40

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #38)
> This is a UX/product decision so I'm deferring to antlam. See also bug
> 1220770 which is trying to change some of this. Personally I do think we
> should keep the skew.

kats, Anthony and I aren't sure what is the benefit of the skew is. Do you know? Specifically, 1) the skew where the top is larger than the bottom and 2) the skew where the height is larger than the width (maybe a side effect of 1?).

Comment 41

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

The skew is basically to account for the shape of an average user's finger when it makes contact with the touch surface. Generally the contact area is taller than it is wide, and the contact point is lower than the user expects it to be. So in the diagram below if the user is aiming at X they will likely hit somewhere closer to Y and the entire box will be hidden by their finger. So the fluff region expands the hit area from Y to the entire box, skewed in the direction of X. Does that make sense?

+-----+
|     |
|  X  |
|     |
|  Y  |
|     |
+-----+

Comment 42

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

Comment on attachment 8681674 [details] [diff] [review]
patch-01112015 1-Bug_11208370___New_size_for_the_touch_area___r_mcomella.patch

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

It's unclear to me what we're changing here and how it affects UX

Comment 43

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

What else do the "ui.*.radius.*mm" values affect? Anthony is trying to change the behavior only for the zoomed view so we'd like to know about the side effects of this change.

Comment 44

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

Moving the discussion (and NI) about the triggering hit area from comment 29 downward to bug 1222234 to keep that discussion separate from the one this bug was originally about.

Comment 45

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

NI about 2.3 (comment 39) moved to bug 1222237.