1019735 - Undo close tab super toast blocks access to the address bar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

Attachment: Screeenshot of the issue

The undo close tab super toast blocks access to the address bar.

STR
* Open two or more tabs
* Swipe one closed
* try to tap on the address bar

Comment 1

[:Margaret Leibovic]

In bug 1018417, I brought up the idea of dismissing the toast as soon as the user touches elsewhere on the screen. I think that would be an appropriate solution to this bug.

Comment 2

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

I've noticed this too. I tried to grab it and throw it like a growl notification once or twice...

Comment 3

[:Margaret Leibovic]

I think we should handle this separately from bug 1018417.

I think we should dismiss the toast when the user taps elsewhere on the screen.

Comment 4

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

That seems like a reasonable solution to me. Let's give it a try.

Comment 5

[:Margaret Leibovic]

Attachment: quick and dirty patch

Not sure how gross this is, but it works well. Maybe we can go with this, but then rename HideTabsTouchListener to something more generic about hiding all sorts of stuff when the user touches the screen.

Comment 6

[Richard Newman [:rnewman]]

Can we just put the toast in a better position?

Comment 7

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #6)
> Can we just put the toast in a better position?

That's not a perfect solution. As long as the toast takes up non-zero space, it's going to block *something*.

However, I think we're planning to address the positioning in bug 1019318.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8435324 [details] [diff] [review]
quick and dirty patch

I also want to know what Lucas thinks of this.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8435324 [details] [diff] [review]
quick and dirty patch

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

Looks reasonable. As you said, you should probably rename the listener's class name to something more generic.

::: mobile/android/base/BrowserApp.java
@@ +1978,5 @@
>          private boolean mIsHidingTabs = false;
>  
>          @Override
>          public boolean onInterceptTouchEvent(View view, MotionEvent event) {
> +            getButtonToast().hide(false, ButtonToast.ReasonHidden.TOUCH);

Design question: maybe it should fade instead of simply disappearing?

@@ +1984,5 @@
>              // We need to account for scroll state for the touched view otherwise
>              // tapping on an "empty" part of the view will still be considered a
>              // valid touch event.
>              if (view.getScrollX() != 0 || view.getScrollY() != 0) {
>                  Rect rect = new Rect();

Ugh, we should create an mTempRect member to avoid allocating a new Rect on each touch event. File a mentored bug?

::: mobile/android/base/widget/ButtonToast.java
@@ +41,5 @@
>      private Toast mCurrentToast;
>  
>      public enum ReasonHidden {
>          CLICKED,
> +        TOUCH,

TOUCH_OUTSIDE for extra clarity?

Comment 10

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #9)
> Comment on attachment 8435324 [details] [diff] [review]
> quick and dirty patch
> 
> Review of attachment 8435324 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable. As you said, you should probably rename the listener's
> class name to something more generic.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1978,5 @@
> >          private boolean mIsHidingTabs = false;
> >  
> >          @Override
> >          public boolean onInterceptTouchEvent(View view, MotionEvent event) {
> > +            getButtonToast().hide(false, ButtonToast.ReasonHidden.TOUCH);
> 
> Design question: maybe it should fade instead of simply disappearing?

That's actually what this code currently does:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ButtonToast.java#143

I think the fade is nice, so I think we should just keep it as is :)

> @@ +1984,5 @@
> >              // We need to account for scroll state for the touched view otherwise
> >              // tapping on an "empty" part of the view will still be considered a
> >              // valid touch event.
> >              if (view.getScrollX() != 0 || view.getScrollY() != 0) {
> >                  Rect rect = new Rect();
> 
> Ugh, we should create an mTempRect member to avoid allocating a new Rect on
> each touch event. File a mentored bug?

Filed bug 1024120.

> ::: mobile/android/base/widget/ButtonToast.java
> @@ +41,5 @@
> >      private Toast mCurrentToast;
> >  
> >      public enum ReasonHidden {
> >          CLICKED,
> > +        TOUCH,
> 
> TOUCH_OUTSIDE for extra clarity?

Sounds good.

Comment 11

[:Margaret Leibovic]

Attachment: Hide the button toast if the user touches outside of it

Addressed feedback.

Here's an APK to test: https://dl.dropboxusercontent.com/u/3358452/fennec-hide-toast.apk

Comment 12

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

I like it!

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8438707 [details] [diff] [review]
Hide the button toast if the user touches outside of it

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

Nice. Is this alpha animation prone to that visibility bug on pre-HC?

Comment 14

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #13)
> Comment on attachment 8438707 [details] [diff] [review]
> Hide the button toast if the user touches outside of it
> 
> Review of attachment 8438707 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice. Is this alpha animation prone to that visibility bug on pre-HC?

Already fixed by bug 1017566 :)

/me shakes fist at gingerbread animations!

Comment 15

[:Margaret Leibovic]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/578bf8d6363c

Comment 17

[:Margaret Leibovic]

Shoot, it looks like this caused a robocop pan regression:
http://graphs.mozilla.org/graph.html#tests=[[174,64,29]]&sel=1402512218000,1402685018000

The regression looks pretty bad, so I just backed the patch out:
https://hg.mozilla.org/integration/fx-team/rev/1e1f339edfa8

I'll look into this more closely to see how I can fix it.

Comment 18

[:Margaret Leibovic]

It looks like we were doing 2 dumb things:

1) Inflating the button toast view before trying to hide it if it wasn't already inflated
2) Always creating a new alpha animation, even if the view was already hidden

I made a try push with fixes for these issues to see if this improves things:

https://tbpl.mozilla.org/?tree=Try&rev=72dfe7857e2c

Comment 19

[:Margaret Leibovic]

Attachment: (Part 2) Don't do unnecssary work on every touch

It looks like this patch brings the talos numbers back down to their normal range.

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 8440447 [details] [diff] [review]
(Part 2) Don't do unnecssary work on every touch

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

::: mobile/android/base/BrowserApp.java
@@ +2000,5 @@
>          @Override
>          public boolean onInterceptTouchEvent(View view, MotionEvent event) {
> +            // Only try to hide the button toast if it's already inflated.
> +            if (mToast != null) {
> +                mToast.hide(false, ButtonToast.ReasonHidden.TOUCH_OUTSIDE);

Slightly mixed about this. The whole point about the getButtonToast() method was to encapsulate the toast's lazy inflation. Using mToast directly here kinda breaks this scheme. I don't feel strongly about it though.

Comment 21

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #20)
> Comment on attachment 8440447 [details] [diff] [review]
> (Part 2) Don't do unnecssary work on every touch
> 
> Review of attachment 8440447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +2000,5 @@
> >          @Override
> >          public boolean onInterceptTouchEvent(View view, MotionEvent event) {
> > +            // Only try to hide the button toast if it's already inflated.
> > +            if (mToast != null) {
> > +                mToast.hide(false, ButtonToast.ReasonHidden.TOUCH_OUTSIDE);
> 
> Slightly mixed about this. The whole point about the getButtonToast() method
> was to encapsulate the toast's lazy inflation. Using mToast directly here
> kinda breaks this scheme. I don't feel strongly about it though.

Yeah... I don't love it either, but I feel like this is necessary for performance. I don't really love that the button toast just lives as part of the main layout, but there doesn't seem to be a simple way to change that right now.

Pushed a folded patch:
https://hg.mozilla.org/integration/fx-team/rev/eeec5cc72be7

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Tiecheng Wu]

It looks like there is a potential memory leak. Because GeckoApp.java notify browser.js to release the callback data, only when the Hidden reason is "ReasonHidden.TIMEOUT", and therefore in case the reason is "ReasonHidden.TOUCH_OUTSIDE", callback data will just accumulate all the way.

---------------------
GeckoApp.java#840
                    public void onToastHidden(ButtonToast.ReasonHidden reason) {
                        if (reason == ButtonToast.ReasonHidden.TIMEOUT) {
                            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Toast:Hidden", buttonId));
                        }
                    }
---------------------

---------------------
browser.js#2159
    } else if (aTopic == "Toast:Hidden") {
      if (this.toast._callbacks[aData])
        delete this.toast._callbacks[aData];
---------------------

Comment 24

[:Margaret Leibovic]

(In reply to Tiecheng Wu from comment #23)
> It looks like there is a potential memory leak. Because GeckoApp.java notify
> browser.js to release the callback data, only when the Hidden reason is
> "ReasonHidden.TIMEOUT", and therefore in case the reason is
> "ReasonHidden.TOUCH_OUTSIDE", callback data will just accumulate all the way.
> 
> ---------------------
> GeckoApp.java#840
>                     public void onToastHidden(ButtonToast.ReasonHidden
> reason) {
>                         if (reason == ButtonToast.ReasonHidden.TIMEOUT) {
>                            
> GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Toast:
> Hidden", buttonId));
>                         }
>                     }
> ---------------------
> 
> ---------------------
> browser.js#2159
>     } else if (aTopic == "Toast:Hidden") {
>       if (this.toast._callbacks[aData])
>         delete this.toast._callbacks[aData];
> ---------------------

Thanks for the report! Would you mind filing a new bug for us to investigate and track this issue?

Comment 25

[Tiecheng Wu]

(In reply to :Margaret Leibovic from comment #24)
> (In reply to Tiecheng Wu from comment #23)
> > It looks like there is a potential memory leak. Because GeckoApp.java notify
> > browser.js to release the callback data, only when the Hidden reason is
> > "ReasonHidden.TIMEOUT", and therefore in case the reason is
> > "ReasonHidden.TOUCH_OUTSIDE", callback data will just accumulate all the way.
> > 
> > ---------------------
> > GeckoApp.java#840
> >                     public void onToastHidden(ButtonToast.ReasonHidden
> > reason) {
> >                         if (reason == ButtonToast.ReasonHidden.TIMEOUT) {
> >                            
> > GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Toast:
> > Hidden", buttonId));
> >                         }
> >                     }
> > ---------------------
> > 
> > ---------------------
> > browser.js#2159
> >     } else if (aTopic == "Toast:Hidden") {
> >       if (this.toast._callbacks[aData])
> >         delete this.toast._callbacks[aData];
> > ---------------------
> 
> Thanks for the report! Would you mind filing a new bug for us to investigate
> and track this issue?

Done. Please check out BugID 1099500. Thanks.
https://bugzilla.mozilla.org/show_bug.cgi?id=1099500