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

VERIFIED FIXED in Firefox 41

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: TeoVermesan, Assigned: nalexander, Mentored)

Tracking

({regression})

41 Branch
Firefox 42
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 unaffected, firefox41 verified, firefox42 verified, fennec41+)

Details

Attachments

(1 attachment)

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.
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
Mentor: margaret.leibovic, michael.l.comella
Whiteboard: [lang=java][good first bug]
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.
Flags: needinfo?(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?
Blocks: 1167360
tracking-fennec: --- → 41+
Flags: needinfo?(margaret.leibovic) → needinfo?(me)
Keywords: regression
Summary: "Page already in your Reading List | SWITCH" super toast generated by long tapping on the reader view icon from URL Bar is displayed too short on screen → Reading list super toast generated by long tapping on the reader view icon from URL Bar is displayed too short on screen
Whiteboard: [lang=java][good first bug]
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
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.
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.
(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.)
> 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 :(
Bug 1175457 - Only dismiss super toasts at the start of tap actions. r=margaret
Attachment #8644518 - Flags: review?(margaret.leibovic)
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!
Attachment #8644518 - Flags: review+
url:        https://hg.mozilla.org/integration/fx-team/rev/519b41bafad236ff0168d80379e40b004c77794a
changeset:  519b41bafad236ff0168d80379e40b004c77794a
user:       Nick Alexander <nalexander@mozilla.com>
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 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.
Attachment #8644518 - Flags: review?(margaret.leibovic) → approval-mozilla-aurora?
Flags: needinfo?(me)
https://hg.mozilla.org/mozilla-central/rev/519b41bafad2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
"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 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.
Attachment #8644518 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
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?
Flags: needinfo?(ryanvm)
It landed on m-c prior the uplift.
Flags: needinfo?(ryanvm)
Verified as fixed on Firefox 41 Beta 2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.