Undo close tab super toast blocks access to the address bar

RESOLVED FIXED in Firefox 33

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
8 months ago

People

(Reporter: kbrosnan, Assigned: Margaret)

Tracking

Trunk
Firefox 33
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec32+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8433455 [details]
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
(Reporter)

Updated

4 years ago
Blocks: 701725
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Updated

4 years ago
tracking-fennec: --- → ?
I've noticed this too. I tried to grab it and throw it like a growl notification once or twice...
(Assignee)

Comment 3

4 years ago
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.
Assignee: nobody → margaret.leibovic
That seems like a reasonable solution to me. Let's give it a try.
(Assignee)

Updated

4 years ago
tracking-fennec: ? → 32+
(Assignee)

Comment 5

4 years ago
Created attachment 8435324 [details] [diff] [review]
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.
Attachment #8435324 - Flags: feedback?(wjohnston)
Can we just put the toast in a better position?
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Comment 8

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

I also want to know what Lucas thinks of this.
Attachment #8435324 - Flags: feedback?(lucasr.at.mozilla)
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?
Attachment #8435324 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(Assignee)

Comment 10

4 years ago
(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.
(Assignee)

Comment 11

4 years ago
Created attachment 8438707 [details] [diff] [review]
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
Attachment #8435324 - Attachment is obsolete: true
Attachment #8435324 - Flags: feedback?(wjohnston)
Attachment #8438707 - Flags: review?(wjohnston)
Attachment #8438707 - Flags: review?(lucasr.at.mozilla)
I like it!
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?
Attachment #8438707 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 14

4 years ago
(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!
(Assignee)

Updated

4 years ago
Attachment #8438707 - Flags: review?(wjohnston)
https://hg.mozilla.org/mozilla-central/rev/578bf8d6363c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
(Assignee)

Comment 17

4 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

4 years ago
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
(Assignee)

Comment 19

4 years ago
Created attachment 8440447 [details] [diff] [review]
(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.
Attachment #8440447 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8440447 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 21

4 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/eeec5cc72be7
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Depends on: 1026798

Comment 23

3 years ago
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];
---------------------
(Assignee)

Comment 24

3 years ago
(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?
Flags: needinfo?(wuhaoshu)

Comment 25

3 years ago
(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
Flags: needinfo?(wuhaoshu)
(Assignee)

Updated

3 years ago
Depends on: 1099500
You need to log in before you can comment on or make changes to this bug.