997049 - "Switch to tab" does not work for Reading List entries

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[u421692]

Environment:
Device: Alcatel One Touch 8008D(Android 4.1.2)
Build: Nightly 31.0a1(2014-04-15)

Steps to reproduce:
1. Open an article
2. Long tap on "reader mode" icon
3. Tap on "reader mode" icon
4. Open about:home in a new tab
5. Go to "Reading List" tab and tap on the entry

Expected result:
The tab is switched to the article in reader mode in the original tab(from step 3)

Actual result:
Article is opened in reader mode in the same tab

Comment 1

[Alexandru Chiriac]

Hello, 

I would like to work on this bug, if it's ok with you.

Thanks, 
Alex

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Thanks Alexandru! Looks like you already have a development setup and you have been working on other bugs, so I don't need to tell you the basic introduction.

If you do have questions, ask here or jump on #mobile on the mozilla IRC server.

Comment 3

[Alexandru Chiriac]

Attachment: Fixed "Switch to tab" functionality for Reading List entries.

The actual url of the reader mode enabled tab was not extracted from its "about:reader" form and the wrong url was searched in the current opened tabs.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8417335 [details] [diff] [review]
Fixed "Switch to tab" functionality for Reading List entries.

Review of attachment 8417335 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see if there's a better way to do this without adding even more reader mode logic to Tabs.

::: mobile/android/base/Tabs.java
@@ +673,5 @@
>              }
>              String tabUrl = tab.getURL();
>              if (AboutPages.isAboutReader(tabUrl)) {
>                  tabUrl = ReaderModeUtils.getUrlFromAboutReader(tabUrl);
>              }

Couldn't we just fix this by removing this check to strip the about:reader part of the URL? I wonder why this is here in the first place. Doing some digging, I found that this bit of logic was originally added to fix bug 897822, but lots of things have changed since then.

This code is tricky... I don't think that we should be putting reader-mode specific code into this Tabs method at all. Instead, I think we should be using the reader mode URL instead of the page URL when we look for open tabs in ReadingListRow#updateDisplayedUrl. Can you try that out?

Comment 5

[Alexandru Chiriac]

Attachment: Fixed "Switch to tab" functionality for Reading List entries.

- removed the reader mode specific code from the Tabs#getFirstTabForUrlHelper() method.
- because a tab url in reader mode has 'tabId' query parameter attached, I used String#contains() method trying to match an opened tab with the given url method paramater, but...I'm not sure if it's ok to do that.

Comment 6

[:Margaret Leibovic]

Sorry for the slow response! I've been traveling, so I haven't had time to look at this until now.

(In reply to Alexandru Chiriac from comment #5)
> Created attachment 8420112 [details] [diff] [review]
> Fixed "Switch to tab" functionality for Reading List entries.
> 
> - removed the reader mode specific code from the
> Tabs#getFirstTabForUrlHelper() method.

> - because a tab url in reader mode has 'tabId' query parameter attached, I
> used String#contains() method trying to match an opened tab with the given
> url method paramater, but...I'm not sure if it's ok to do that.

This doesn't seem very roubust, and the logic will break on sites that use URL parameters to actually show different pages. However, it does raise a good point about how these reader mode URLs can be difficult because they're special.

I wanted to avoid doing something special for these URLs, but I think we have to. I think that we should make a new helper method in Tabs called getFirstReaderTabForUrl. This way, we can remove the existing reader mode logic from getFirstTabForUrlHelper, but we can have a method that does the right thing without adding so much new logic to ReadingListRow. What do you think of this idea?

Comment 7

[:Margaret Leibovic]

Comment on attachment 8420112 [details] [diff] [review]
Fixed "Switch to tab" functionality for Reading List entries.

Review of attachment 8420112 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/Tabs.java
@@ +667,5 @@
>          for (Tab tab : mOrder) {
>              if (isPrivate != null && isPrivate != tab.isPrivate()) {
>                  continue;
>              }
> +            if(tab.getURL().contains(url)) {

As I mentioned in my last comment, I don't think this is a good idea, since it would break switch-to-tab for some sets of regular URLs.

Comment 8

[Alexandru Chiriac]

Attachment: Fixed "Switch to tab" functionality for Reading List entries.

- removed the reader mode specific code from the Tabs#getFirstTabForUrlHelper() method.

- moved the reader mode code to a new Tabs#getFirstReaderTabForUrlHelper() method. This method is eventually called in BrowserApp#maybeSwitchToTab(url, flags) if there is a reader mode enabled opened tab with the given url.

- removed the new logic added to ReadingListRow#updateDisplayedUrl() in the previous patch and used Tabs#getFirstReaderTabForUrl() method to get, in a cleaner way, the first reader mode enabled opened tab.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8421635 [details] [diff] [review]
Fixed "Switch to tab" functionality for Reading List entries.

Review of attachment 8421635 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is much better, thanks! I just have a few small comments for you to address. I especially like how you added comments to explain things, that's good for people who come along in the future :)

::: mobile/android/base/BrowserApp.java
@@ +1487,5 @@
>  
>          final Tabs tabs = Tabs.getInstance();
> +        final Tab tab;
> +
> +        if(AboutPages.isAboutReader(url)) {

Nit: Add a space between if and (

::: mobile/android/base/Tabs.java
@@ +685,5 @@
> +     * @param isPrivate
> +     *            if true, only look for tabs that are private. If false, only
> +     *            look for tabs that are not private.
> +     * @return the first tab in reader mode with the given URL, or null if there
> +     *         is no such tab.

Nice documentation! My only nit-pick would be to capitalize the beginning of sentences.

@@ +691,5 @@
> +    public Tab getFirstReaderTabForUrl(String url, boolean isPrivate) {
> +        return getFirstReaderTabForUrlHelper(url, isPrivate);
> +    }
> +
> +    private Tab getFirstReaderTabForUrlHelper(String url, Boolean isPrivate) {

Since there is only one getFirstReaderTabForUrl method signature, we don't need this helper method, and can implement this directly in getFirstReaderTabForUrl.

@@ +696,5 @@
> +        if (url == null) {
> +            return null;
> +        }
> +
> +        if (AboutPages.isAboutReader(url)) {

I think we should be explicit about whether or not we expect a reader mode URL to be passed in here. It looks like the pageUrl parameter in ReadingListRow is the actual article url, not the reader mode url, so I don't think we need this check. However, we should add information to the documentation explaining that we expect actual article urls to be passed in here.

Comment 10

[Alexandru Chiriac]

Attachment: Fixed "Switch to tab" functionality for Reading List entries.

The check whether the url parameter in Tabs#getFirstReaderTabForUrl(url, isPrivate) method is a reader mode URL is needed because the url parameter can be the actual article URL or the reader mode article URL:

1. When called in ReadingListRow#updateDisplayedUrl() in order set the displayed URL in the reading list row, the pageUrl parameter is the actual article URL.

2. When called in BrowserApp#maybeSwitchToTab(url, flags) - this happens when the user taps the reading list article entry - the url is in its reader mode state (the switch to the reader mode url is done in ReadingListPanel#onViewCreated() in the reading list's item click listener). Because the tab url in the reader mode has an additional 'tabId' query parameter attached, the first open tab in reader mode with the given url parameter is found comparing the actual URLs and not the reader mode URLs.

Comment 11

[:Margaret Leibovic]

(In reply to Alexandru Chiriac from comment #10)
> Created attachment 8422322 [details] [diff] [review]
> Fixed "Switch to tab" functionality for Reading List entries.
> 
> The check whether the url parameter in Tabs#getFirstReaderTabForUrl(url,
> isPrivate) method is a reader mode URL is needed because the url parameter
> can be the actual article URL or the reader mode article URL:
> 
> 1. When called in ReadingListRow#updateDisplayedUrl() in order set the
> displayed URL in the reading list row, the pageUrl parameter is the actual
> article URL.
> 
> 2. When called in BrowserApp#maybeSwitchToTab(url, flags) - this happens
> when the user taps the reading list article entry - the url is in its reader
> mode state (the switch to the reader mode url is done in
> ReadingListPanel#onViewCreated() in the reading list's item click listener).
> Because the tab url in the reader mode has an additional 'tabId' query
> parameter attached, the first open tab in reader mode with the given url
> parameter is found comparing the actual URLs and not the reader mode URLs.

Good catch. I went through this logic myself, but I forgot about the 'tabId' query parameter. This logic is too complicated! :)

Comment 12

[:Margaret Leibovic]

Comment on attachment 8422322 [details] [diff] [review]
Fixed "Switch to tab" functionality for Reading List entries.

Review of attachment 8422322 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, thanks for the thorough investigation.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Margaret, can we run this through Try before checking it in please :)

Comment 14

[:Margaret Leibovic]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> Margaret, can we run this through Try before checking it in please :)

Good call, here's a try push:
https://tbpl.mozilla.org/?tree=Try&rev=b81866ced625

Comment 15

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #14)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> > Margaret, can we run this through Try before checking it in please :)
> 
> Good call, here's a try push:
> https://tbpl.mozilla.org/?tree=Try&rev=b81866ced625

All green!

Comment 16

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

https://hg.mozilla.org/integration/fx-team/rev/e641f0a7a21e

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e641f0a7a21e

Comment 18

[Flaviu Cos]

Verified as fixed in build 32.0a1 (2014-05-26)
Device: Asus Transformer Tab (Android 4.1.1).

Comment 19

[Cristina Madaras, QA [:CristinaM]]

Still reproducible on Firefox for Android 31 Beta 1. Is this gonna be uplifted? Or won't be fix on the other channels?

Comment 20

[Aaron Train [:aaronmt]]

This is tracking +, meaning it's not targeting any particular release other than trunk which so happens to be mozilla-33.