1200181 - Zoomed view and Form Assistant should not be displayed at the same time

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dominique Vincent [:domivinc]]

When the form assistant is triggered, if the zoomed view is already open, the zoomed view should be closed.
When the zoomed view is triggered, if a form assistant is already open, the zoomed view should not be displayed.
We cannot have both tools displayed at the same time. Priority is set to the form assistant.

Comment 1

[Dominique Vincent [:domivinc]]

Attachment: patch-31082015 3-Bug_1200181___Zoomed_view_and_Form_Assistant_should_not_be_displayed_at_the_same_time__r_mcomella.patch

Comment 2

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

Comment on attachment 8654828 [details] [diff] [review]
patch-31082015 3-Bug_1200181___Zoomed_view_and_Form_Assistant_should_not_be_displayed_at_the_same_time__r_mcomella.patch

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

Should the zoomed view be closing the form assistant pop-up instead? I feel if I'm editing a form field and ambiguously try to click a link, I'd like the auto-complete to close and the zoomed view to appear – is there a use case for preventing the zoomed view from appearing on these clicks?

::: mobile/android/base/ZoomedView.java
@@ +611,5 @@
>              @Override
>              public void run() {
>                  try {
>                      if (event.equals("Gesture:clusteredLinksClicked")) {
> +                        if (!bLocked) {

It seems safer to put this cancellation in startZoomDisplay, which is the entry point to the zoomed view, rather than on its call sites. Would you agree?

Comment 3

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

Comment on attachment 8654828 [details] [diff] [review]
patch-31082015 3-Bug_1200181___Zoomed_view_and_Form_Assistant_should_not_be_displayed_at_the_same_time__r_mcomella.patch

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

::: mobile/android/base/ZoomedView.java
@@ +77,5 @@
>      private int viewContainerHeight; // Zoomed view height with toolbar and other elements like shadow, ...
>      private int containterSize; // shadow, margin, ...
>      private Point lastPosition;
>      private boolean shouldSetVisibleOnUpdate;
> +    private boolean bLocked; // Prevent the display of the zoomedview while FormAssistantPopup is visible

nit: -> isBlockedFromAppearing, or similar

blocked doesn't really say what is blocked.

Comment 4

[Dominique Vincent [:domivinc]]

(In reply to Michael Comella (:mcomella) from comment #2)
> 
> Should the zoomed view be closing the form assistant pop-up instead? I feel
> if I'm editing a form field and ambiguously try to click a link, I'd like
> the auto-complete to close and the zoomed view to appear – is there a use
> case for preventing the zoomed view from appearing on these clicks?
> 
Yes we can try to close the form assistant pop-up in this case.
I cannot see a user case where it should not work.
The only drawback (on the code side) of your proposal: currently the changes are only located in the zoomed view code. With your proposal, the from assistant code will have to be changed. 

Could we ask the feedback from Barbara (product) and Anthony (UX), and get a final decision regarding this behavior before spending time on the 2 different solutions?

Comment 5

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

(In reply to Dominique Vincent [:domivinc] from comment #4)
> The only drawback (on the code side) of your proposal: currently the changes
> are only located in the zoomed view code. With your proposal, the from
> assistant code will have to be changed. 

This is true and then we can start to have a lot of weird dependencies where one feature relies on another, which relies on another, which...

Given the context I have, I'm not sure I can think of a cleaner way to do this though.

> Could we ask the feedback from Barbara (product) and Anthony (UX), and get a
> final decision regarding this behavior before spending time on the 2
> different solutions?

Sure – feel free to NI them yourself in the future! I think antlam is the most appropriate here.

Anthony, when the form assistant is open and a link is clicked ambiguously, what should happen? I felt the form assistant should be closed and the zoomed view open. It's worth noting that this solution is more complex than preventing the zoomed view from being displayed so if you don't have a preference, we should take the less complex solution.

Comment 6

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

(In reply to Michael Comella (:mcomella) from comment #5)
> Anthony, when the form assistant is open and a link is clicked ambiguously,
> what should happen? I felt the form assistant should be closed and the
> zoomed view open. It's worth noting that this solution is more complex than
> preventing the zoomed view from being displayed so if you don't have a
> preference, we should take the less complex solution.

If the form assistant is open and the user presses outside of it, shouldn't it close the dropdown first? Not unlike what we decided to do for "zoomed view" in bug 1180811.

That would essentially simplify this bug for us.

Comment 7

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

(In reply to Anthony Lam (:antlam) from comment #6)
> If the form assistant is open and the user presses outside of it, shouldn't
> it close the dropdown first?

What about scrolling? I feel the user may want to scroll the page and not have the assistant disappear (e.g. oh, didn't I fill this out already?). Though it may be intuitive to re-click in the input box for the assistant to reappear (if that works). This is compounded by our rather janky form assistant (bug 1198524).

Comment 8

[Dominique Vincent [:domivinc]]

(In reply to Anthony Lam (:antlam) from comment #6)
> (In reply to Michael Comella (:mcomella) from comment #5)
> > Anthony, when the form assistant is open and a link is clicked ambiguously,
> > what should happen? I felt the form assistant should be closed and the
> > zoomed view open. It's worth noting that this solution is more complex than
> > preventing the zoomed view from being displayed so if you don't have a
> > preference, we should take the less complex solution.
> 
> If the form assistant is open and the user presses outside of it, shouldn't
> it close the dropdown first? Not unlike what we decided to do for "zoomed
> view" in bug 1180811.
> 
> That would essentially simplify this bug for us.

Yes, we will have to implement only the first rule of this bug:
"When the form assistant is triggered, if the zoomed view is already open, the zoomed view should be closed."
The second rule should never occur.
A new bug should be open to make the change in the form assistant code.

Comment 9

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

(In reply to Michael Comella (:mcomella) from comment #7)
> (In reply to Anthony Lam (:antlam) from comment #6)
> > If the form assistant is open and the user presses outside of it, shouldn't
> > it close the dropdown first?
> 
> What about scrolling? I feel the user may want to scroll the page and not
> have the assistant disappear (e.g. oh, didn't I fill this out already?).
> Though it may be intuitive to re-click in the input box for the assistant to
> reappear (if that works). This is compounded by our rather janky form
> assistant (bug 1198524).

Yeah, I think that will be fine. 

If scrolling is what the user wants to do, then we shouldn't assume they want to keep something else open unless we have a strong reason to.

Comment 10

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

(In reply to Dominique Vincent [:domivinc] from comment #8)
> Yes, we will have to implement only the first rule of this bug:
> "When the form assistant is triggered, if the zoomed view is already open,
> the zoomed view should be closed."

Ah, this is in the event that the form assistant is triggered from within the zoomed view (because if it's triggered outside, it'd close by itself). Sounds good.

Do you want to post a new patch with my feedback from comment 2 and comment 3? With the feedback, I think it should be a quick r+.

> A new bug should be open to make the change in the form assistant code.

Dominique, can you file?

Comment 11

[Dominique Vincent [:domivinc]]

Attachment: patch-10092015 1-Bug_1200181___Zoomed_view_and_Form_Assistant_should_not_be_displayed_at_the_same_time__r_mcomella.patch

Comment 12

[Dominique Vincent [:domivinc]]

> > A new bug should be open to make the change in the form assistant code.
> 
> Dominique, can you file?

Bug 1203665 will change the form assistant behavior.

Comment 13

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

Comment on attachment 8659425 [details] [diff] [review]
patch-10092015 1-Bug_1200181___Zoomed_view_and_Form_Assistant_should_not_be_displayed_at_the_same_time__r_mcomella.patch

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

If this works, wfm! :)

Comment 14

[Dominique Vincent [:domivinc]]

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

Comment 15

[Dominique Vincent [:domivinc]]

Mickael, testing again in different forms (for instance reddit login form) for Bug 1203665, I have a strange/randomly behavior when the form assistant is displayed: depending of the touch position, the form assistant cannot be closed.
It could be linked to this change. I'm going to figure out where the issue is first, and ask later a checkin of this change.

Comment 16

[Dominique Vincent [:domivinc]]

Attachment: patch-14092015 1-Bug_1200181___Zoomed_view_and_Form_Assistant_should_not_be_displayed_at_the_same_time__r_mcomella.patch

Michael, I found the reason why the form assistant is not always closed clicking outside the form assistant. The display of the form assistant is based on the focus. Without removing the focus from the input element, it’s useless to hide the form assistant because it will be re-displayed.

When a click occurs on a cluster of links, the zoomed view is displayed but there is no mouse down/up events in the page. Without those events in the page, the focus is not removed from the current input element. In this case, the form assistant is not closed. And we cannot close it because the focus is still on the same input element.

I made the following change in order to fix this issue: if the user clicks in a cluster of links and there is a focused element, move the focus away from the current focused element and hide the form assistant. Then the zoomed view can be displayed.

Comment 17

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

Hey Dominique, I'll get to this tomorrow.

Comment 18

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

Comment on attachment 8660664 [details] [diff] [review]
patch-14092015 1-Bug_1200181___Zoomed_view_and_Form_Assistant_should_not_be_displayed_at_the_same_time__r_mcomella.patch

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

Cool.

Comment 19

[Dominique Vincent [:domivinc]]

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

Comment 21

[Pulsebot]

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

Comment 22

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/9a789bc21fb6