Closed Bug 1175457 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

41 Branch
ARM
Android
defect
Not set
normal

Tracking

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

VERIFIED FIXED
Firefox 42
Tracking Status
firefox40 --- unaffected
firefox41 --- verified
firefox42 --- verified
fennec 41+ ---

People

(Reporter: TeoVermesan, Assigned: nalexander, Mentored)

References

Details

(Keywords: regression)

Attachments

(1 file)

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)
Status: ASSIGNED → RESOLVED
Closed: 10 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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: