1175457 - Reading list super toast generated by long tapping on the reader view icon from URL Bar is displayed too short on screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Steps to reproduce:
1. Go to news.google.com and choose an article
2. Long tap on the reader view icon from the URL Bar 

Actual results:
- "Added to list | SWITCH" toast notification is displayed. You are able to switch to it.
- Long-tapping once again on the reader view icon from the URL Bar will display too short the "Page already in your Reading List | SWITCH" notification. You have no time to tap the "Switch" button.

Comment 1

[:Margaret Leibovic]

I think this is good feedback. To address this, we would just need to change the duration in here from LENGTH_SHORT to LENGTH_LONG:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#589

Comment 2

[Nicholas Rosbrook [:enr0n]]

I played around with this one a little bit, and it seems that the duration is not the problem. Replacing LENGTH_SHORT with LENGTH_LONG has no effect--the notification still disappears immediately after the reader view icon is released.

Comment 3

[:Margaret Leibovic]

(In reply to Nicholas Rosbrook [:enr0n] from comment #2)
> I played around with this one a little bit, and it seems that the duration
> is not the problem. Replacing LENGTH_SHORT with LENGTH_LONG has no
> effect--the notification still disappears immediately after the reader view
> icon is released.

Good call. I just tried this out myself, and you're right that it disappears immediately. This affects both the "page added" and the "page already in list" super toasts.

It looks like this is a regression from bug 1167360.

Amin, can you look into fixing this?

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

On tablets I noticed something similar: Swiping away a tab in the tab switcher -> Toast appears, Swiping away another tab -> Toast appears and immediately disappears again.

Comment 5

[Vivek Balakrishnan[:vivek]]

I suspect this is because we clear the ButtonToast when the touch is outside of toast view.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#2859
The notifications didn't disappear when I commented out the above lines.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Vivek Balakrishnan[:vivek] from comment #5)
> I suspect this is because we clear the ButtonToast when the touch is outside
> of toast view.
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java#2859
> The notifications didn't disappear when I commented out the above lines.

OK, vivek is correct that we're dismissing the super toast immediately.

We're seeing an interaction between touch event handlers and displaying super toasts that would be witnessed by *any* Android view dispatching a super toast while the user is touching the "main_layout".  That is, if the URL bar, or the lock icon, or some page action other than the Reading List icon, displayed a super toast, this would occur.

It is probably possible, but certainly not easy and robust, to distinguish which views might display super toasts and handle such touches specially.  I would advise against.

I think the expedient solution might be to open a context menu (like long pressing the URL bar itself) with the options to add/remove from reading list, and then show the toast in response to the action in the context menu.  Since the context menu is dismissed and the action occurs only when the touch is lifted, this neatly sidesteps the issue.

(The other expedient solution is to back out the offending patch, which would be a shame.)

Comment 7

[Nick Alexander :nalexander [he/him]]

> I think the expedient solution might be to open a context menu (like long
> pressing the URL bar itself) with the options to add/remove from reading
> list, and then show the toast in response to the action in the context menu.
> Since the context menu is dismissed and the action occurs only when the
> touch is lifted, this neatly sidesteps the issue.

The plot thickens!  The Reader Mode icon is a PageAction.  It's not obvious how to present a context menu in response to a long click on the page action, and it's doubly not-obvious because the underlying PageAction view registers for onLongClick(), which *does not wait for the click to be released*.  So we'd still run into touch event problems if we waited for onLongClick() and then popped up a context menu: the touch would still be ongoing.

Given this, I think we would need to make PageAction only fire the long click event when the long click is *released*.  I haven't a clue how that interacts with other page actions :(

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1175457 - Only dismiss super toasts at the start of tap actions. r=margaret

Bug 1175457 - Only dismiss super toasts at the start of tap actions. r=margaret

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8644518 [details]
MozReview Request: Bug 1175457 - Only dismiss super toasts at the start of tap actions. r=margaret

https://reviewboard.mozilla.org/r/15253/#review13745

Ship It!

Comment 10

[Nick Alexander :nalexander [he/him]]

url:        https://hg.mozilla.org/integration/fx-team/rev/519b41bafad236ff0168d80379e40b004c77794a
changeset:  519b41bafad236ff0168d80379e40b004c77794a
user:       Nick Alexander <[e-mail]>
date:       Thu Aug 06 12:02:20 2015 -0700
description:
Bug 1175457 - Only dismiss super toasts at the start of tap actions. r=rnewman

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8644518 [details]
MozReview Request: Bug 1175457 - Only dismiss super toasts at the start of tap actions. r=margaret

Approval Request Comment
[Feature/regressing bug #]: Bug 1167360
[User impact if declined]: useless toasts that appear in response to a long click and then are immediately discarded.
[Describe test coverage new/current, TreeHerder]: manual local builds.
[Risks and why]: fairly low.  We might leave super toasts on screen longer than expected.  In these cases, the user would likely remove their finger from the screen and then touch the screen again, which would then remove the toast.  I'd be more concerned about some other unanticipated interaction between touch events and toasts, but it's hard to say what that would be.
[String/UUID change made/needed]: none.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/519b41bafad2

Comment 13

[Teodora Vermesan (:TeoVermesan)]

"Added to Reading List | SWITCH" and "Page already in your Reading List | SWITCH" toasts are displayed on screen and you are able to switch to it, so verified as fixed using:
Device: Samsung S5 (Android 4.4.2)
Build: Firefox for Android 42.0a1 (2015-08-09)

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8644518 [details]
MozReview Request: Bug 1175457 - Only dismiss super toasts at the start of tap actions. r=margaret

[Triage Comment]
Fix was verified on Nighlty42, patch is a one-liner and addresses a usability scenario on Fennec. Approved for uplift to m-b.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a8464872f1af

Comment 16

[Nick Alexander :nalexander [he/him]]

RyanVM: I believe this is on m-c and m-b, now; I think this needs to land on m-a as well.  Could you do so?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

It landed on m-c prior the uplift.

Comment 18

[u421692]

Verified as fixed on Firefox 41 Beta 2