1190332 - Zoomed View appears in textareas

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul [pwd]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150728111723

Steps to reproduce:

Zoomed view should never appear in a text area, if someone is trying to move a cursor, you have the big handle for precision, you don't need Zoomed View. It makes the interaction with forms wholly more frustrating.

Comment 1

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

I haven't seen this yet - what do you press here to bring up the zoomed view? e.g. between the letters, on the text selection handle

Comment 2

[Paul [pwd]]

Between the letters.

Comment 3

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

Although I cannot repro, I agree conceptually that this shouldn't happen - we already zoomed into text fields so there's no need to have a magnifying glass there.

Dominique, have you seen this?

Comment 4

[Dominique Vincent [:domivinc]]

If there are other clickable elements near the text area, you can probably click between the different clickable elements and the zoomed view will be displayed.
If you click inside the text area, it should not be possible with the current version of the code.

But after the implementation of bug 1191034, it will be the case. With the heuristic of this bug, if you click inside the text area on a border, and near the text area another clickable element is available, the zoomed view could be displayed (if less than 75% of the click occurs on the text area, and more than 25% on the other clickable element).

Comment 5

[Dominique Vincent [:domivinc]]

Michael, I can reproduce several issues in [1]. I'm not sure that it's exactly the same problem reported in comment 1 because there is no link in the first comment. But in any case, we have to fix them!
By default, this page is displayed with very small font size. 

Issue N°1:
When the user clicks on the small input field, two different actions occur at the same time:
1- the zoomed view is displayed because the input field is too small to be readable,
2- the whole page is zoomed in and centered on the input field,
3- the keyboard is not displayed.

Issue N°2 (after step 3):
When the user clicks inside the input field centered in the screen, the keyboard is displayed but we cannot see anymore the input field on my device (htc desize z). The input field is hidden by the keyboard.

Issue N°3 (after step 3, probably the issue from comment 1):
Sometimes when the user clicks inside the input field, the keyboard is displayed and the big handle to select the text is also displayed.

Issue N°4:
1- The user clicks on the page near the buttons or the input field. In this case the zoomed view is displayed because it's an ambiguous click.
2- Then the user moves the zoomed view to show the input field inside the zoomed view.
3- the user clicks in the zoomed view inside the input field, the whole zoom in of the page occurs, the zoomed view is still visible, the keyboard is visible, the input field is hidden by the keyboard.

I tried without the zoomed view, the issues n°2 and 3 are still visible.

The first issue can be fixed blocking the display of the zoomed view in case of automatic zoom in. I don't know how and where the zoom of the whole page is implemented. But we have to "synchronize" this action with the zoomed view display.

The issue number 4 is more problematic because in this case the user decided to use the zoomed view. Do we have to force the whole zoom in of the page and close the zoomed view? Or do we have to keep the zoomed view visible and do not zoom in the whole page?

[1] http://www.indianrail.gov.in/train_Schedule.html

Comment 6

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

(In reply to Dominique Vincent [:domivinc] from comment #5)
> When the user clicks inside the input field centered in the screen, the
> keyboard is displayed but we cannot see anymore the input field on my device
> (htc desize z). The input field is hidden by the keyboard.

I personally find our algorithm to zoom into a text field to be pretty crappy - that's probably the issue.

> Issue N°3 (after step 3, probably the issue from comment 1):
> Sometimes when the user clicks inside the input field, the keyboard is
> displayed and the big handle to select the text is also displayed.

I've noticed this before without the zoomed view.

> The first issue can be fixed blocking the display of the zoomed view in case
> of automatic zoom in. I don't know how and where the zoom of the whole page
> is implemented.

I think that's here: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=2e9102e4b0a6#1679

> The issue number 4 is more problematic because in this case the user decided
> to use the zoomed view. Do we have to force the whole zoom in of the page
> and close the zoomed view? Or do we have to keep the zoomed view visible and
> do not zoom in the whole page?

Anthony?

Comment 7

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

(In reply to Michael Comella (:mcomella) from comment #6)
> (In reply to Dominique Vincent [:domivinc] from comment #5)
> > The issue number 4 is more problematic because in this case the user decided
> > to use the zoomed view. Do we have to force the whole zoom in of the page
> > and close the zoomed view? Or do we have to keep the zoomed view visible and
> > do not zoom in the whole page?
> 
> Anthony?

After a user takes an action inside the zoomed view, it should close. Reason being, the purpose of the zoomed view is to disambiguate links. Once that purpose has been served (i.e. a clear tap/press has been triggered), it should close.

If I'm understanding this correctly, the user has clicked something inside the Zoomed View. So, I think it should close. :) hope that helps!

Comment 8

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

(In reply to Anthony Lam (:antlam) from comment #7)
> After a user takes an action inside the zoomed view, it should close. Reason
> being, the purpose of the zoomed view is to disambiguate links. Once that
> purpose has been served (i.e. a clear tap/press has been triggered), it
> should close.

This was previously WONTFIXED in bug 1170713 - perhaps you didn't get a chance to chime in then, Anthony?

Comment 9

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

Yeah I didn't notice that bug go by. Filed bug 1193633.

Comment 10

[Dominique Vincent [:domivinc]]

Attachment: patch-14082015 TER 3-Bug_1190332____Zoomed_View_appears_in_input_fieds__r_mcomella.patch

The zoomed view is closed when events "ZoomToPageWidth" and "Browser:ZoomToRect" are received.

Comment 11

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

Comment on attachment 8648042 [details] [diff] [review]
patch-14082015 TER 3-Bug_1190332____Zoomed_View_appears_in_input_fieds__r_mcomella.patch

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

::: mobile/android/base/ZoomedView.java
@@ +561,5 @@
>      public void stopZoomDisplay(boolean withAnimation) {
> +        // If startZoomDisplay is running and not completed, the zoomed view will be displayed
> +        // after this call. Force the stop of the zoomed view, changing the shouldSetVisibleOnUpdate flag
> +        // before the test of the visibility.
> +        shouldSetVisibleOnUpdate = false;

So startZoomDisplay can run concurrently with stopZoomDisplay? Hm – I wonder if there's a cleaner way to fix these concurrency issues, e.g. run everything on the UI thread.

Have you played with this approach at all?

In any case, if they can run concurrently, startZoomDisplay may start, stopZoomDisplay sets shouldSetVisible = false, and then startZoomDisplay then calls it's shouldSetVisible = true – so I think you need some synchronized in here.

Comment 12

[Dominique Vincent [:domivinc]]

All is done in the UI thread, they cannot run concurrently. 

The zoomedView is displayed in 2 steps in the UI thread: startZoomDisplay then showZoomedView. Between the 2 previous steps, some tasks are executed into the grecko threads (the build of the bitmap). It's the reason why between the 2 steps, a call to stopZoomDisplay can occur.

So, I see only 2 cases:

1) startZoomDisplay (set shouldSetVisible = true)
   showZoomedView (set shouldSetVisible = false, zoomed view is visible)
   stopZoomDisplay (unnecessary set shouldSetVisible = false)

2) startZoomDisplay (set shouldSetVisible = true)
   stopZoomDisplay (set shouldSetVisible = false, zoomed view is not yet visible, showZoomedView will not be called when Gecko threads will be completed)

The code change in stopZoomDisplay fixed an existing issue but it's probably hard to get it: trigger the zoomed view and go quickly to another tab for instance (do not wait to get the zoomed view visible). You should be in the second case, and it should not work correctly, at the end, the zoomed view will be visible.

I can update the comment like that if you want:

// If "startZoomDisplay" is running and not totally completed (Gecko thread is still 
// running and "showZoomedView" has not yet been called), the zoomed view will be 
// displayed after this call and it should not. 
// Force the stop of the zoomed view, changing the shouldSetVisibleOnUpdate flag
// before the test of the visibility.

Comment 13

[Dominique Vincent [:domivinc]]

Michael, could you have a look to my answer to your code review in comment 12 ?

Comment 14

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

Comment on attachment 8648042 [details] [diff] [review]
patch-14082015 TER 3-Bug_1190332____Zoomed_View_appears_in_input_fieds__r_mcomella.patch

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

Sorry for the delay, Dominique.

With the comment cleanup, looks good.

::: mobile/android/base/ZoomedView.java
@@ +558,5 @@
>          moveUsingGeckoPosition(leftFromGecko, topFromGecko);
>      }
>  
>      public void stopZoomDisplay(boolean withAnimation) {
> +        // If startZoomDisplay is running and not completed, the zoomed view will be displayed

"running and not completed" means to me that we're accessing another method concurrently with startZoomDisplay. Something like comment 12 would be good. Perhaps something like:

"We always set shouldSetVisibleOnUpdate to false, rather than in the visibility check block, because if stopZoomDisplay is called before updateView, the view is not visible so shouldSetVisibleOnUpdate is true and a later call to updateView will set the zoomed view visible even though stopZoomDisplay was called and it shouldn't be."

Comment 15

[Dominique Vincent [:domivinc]]

Attachment: patch-29082015 1-Bug_1190332____Zoomed_View_appears_in_input_fieds__r_mcomella.patch

Comment 16

[Dominique Vincent [:domivinc]]

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

Comment 17

[Dominique Vincent [:domivinc]]

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

Comment 18

[Pulsebot]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/008983d8b30a