"Switch-to-tab" should not be displayed in "Reading List" folder when the page is not opened in another tab

VERIFIED FIXED in Firefox 48

Status

()

Firefox for Android
Reader View
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: TeoVermesan, Assigned: ahunt)

Tracking

(Blocks: 1 bug)

48 Branch
Firefox 48
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox48 verified, fennec48+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
Created attachment 8739909 [details]
Screenshot_20160411-120301.png

Tested using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 48.0a1 (2016-04-10)

Steps to reproduce:
1. Open some pages from "Reading list" folder from Bookmarks panel
2. Close them
3. Go back to the "Reading list" folder

Actual results:
- "Switch to tab" is displayed.

Expected results:
- "Switch to tab" should not be displayed.
(Reporter)

Updated

2 years ago
Blocks: 1234314

Updated

2 years ago
Assignee: nobody → ahunt
tracking-fennec: --- → ?
(Assignee)

Comment 1

2 years ago
I can only reproduce this if I keep the bookmarks panel open while closing opened reader view bookmarks. It seems we do receive tab-closing events in the Bookmarks panel (if you close a normal bookmark, the "switch to tab" disappears), but they're not handled correctly for reader view bookmarks.

I think the issue is that we don't store the about:reader url when showing bookmarks, we convert into an about:reader url when actually opening a reader view bookmark. But tab-close events will contain the about:reader url, the two don't match, and we don't change the state of these reader view bookmarks.
(Assignee)

Comment 2

2 years ago
Created attachment 8740662 [details]
MozReview Request: Bug 1263571 - Pre: cleanup imports r=me

Review commit: https://reviewboard.mozilla.org/r/45867/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45867/
Attachment #8740663 - Flags: review?(liuche)
Attachment #8740664 - Flags: review?(liuche)
(Assignee)

Comment 3

2 years ago
Created attachment 8740663 [details]
MozReview Request: Bug 1263571 - Pre: onTabChanged data is always a String, declare it as such r?liuche

We're always using this as a String, we might as well make this explicit to avoid having to cast
anywhere. (We very rarely use the parameter, but some new code in the main part of Bug 1263571
would have to cast this to a String. We can avoid that if we just use the correct type.)

Review commit: https://reviewboard.mozilla.org/r/45869/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45869/
(Assignee)

Comment 4

2 years ago
Created attachment 8740664 [details]
MozReview Request: Bug 1263571 - Update switch-to-tab for reader view pages too r?liuche

Review commit: https://reviewboard.mozilla.org/r/45871/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45871/
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED

Updated

2 years ago
tracking-fennec: ? → 48+
Comment on attachment 8740664 [details]
MozReview Request: Bug 1263571 - Update switch-to-tab for reader view pages too r?liuche

https://reviewboard.mozilla.org/r/45871/#review45009
Attachment #8740664 - Flags: review?(liuche) → review+
Comment on attachment 8740663 [details]
MozReview Request: Bug 1263571 - Pre: onTabChanged data is always a String, declare it as such r?liuche

https://reviewboard.mozilla.org/r/45869/#review45001

I'm a little ambivalent about this change because it seems like a lot of churn for something really small but - this is probably named this way to match JS convention of listeners and interfaces, and if it hasn't been used otherwise for this long, I'm okay switching it to a String. If that changes, we could always use a JSON object, or some Java object. In general casting is jest not very explicit and often just leads to exceptions.

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:584
(Diff revision 1)
>              }
>          });
>      }
>  
>      public interface OnTabsChangedListener {
> -        public void onTabChanged(Tab tab, TabEvents msg, Object data);
> +        public void onTabChanged(Tab tab, TabEvents msg, String data);

IntelliJ tells me this public modifier is unnecessary, so might as well remove it while you're here.
Attachment #8740663 - Flags: review?(liuche) → review+

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4e12b2ddb63
https://hg.mozilla.org/mozilla-central/rev/332bbe886f11
https://hg.mozilla.org/mozilla-central/rev/9ebb4247d445
https://hg.mozilla.org/mozilla-central/rev/215083cf8511
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(Reporter)

Comment 10

2 years ago
Switch-to-tab" is not displayed anymore in "Reading List" folder if the page is not opened in another tab 
Tested using:
Device: Samsung Galaxy S7 Edge (Android 6.0)
Build: Firefox for Android 49.0a1 (2016-05-03)
status-firefox48: fixed → verified

Comment 11

2 years ago
I'm changing the status to verified fixed considering that the bug is verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.