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)
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)
40 bytes,
text/x-review-board-request
|
rnewman
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
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•10 years ago
|
||
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]
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
(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+
status-firefox40:
--- → unaffected
status-firefox42:
--- → affected
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 | ||
Updated•10 years ago
|
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment 4•10 years ago
|
||
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•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
(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.)
Assignee | ||
Comment 7•10 years ago
|
||
> 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 :(
Assignee | ||
Comment 8•10 years ago
|
||
Bug 1175457 - Only dismiss super toasts at the start of tap actions. r=margaret
Attachment #8644518 -
Flags: review?(margaret.leibovic)
Comment 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
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
Assignee | ||
Comment 11•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(me)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Reporter | ||
Comment 13•10 years ago
|
||
"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•10 years ago
|
||
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+
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
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)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•