1080374 - The URL appears instead of 'Switch to tab' under some already opened bookmarks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P5)

Description

[Cristina Madaras, QA [:CristinaM]]

Environment:
Device: Motorola Razr (Android 4.1.2)
Build: Firefox for Android 35.0a1 (2014-10-08)

Steps to reproduce:
1. Launch Fennec and go to Bookmarks panel;
2. Long tap on some bookmarks and choose 'Open in new tab';
3. Swipe to another panel or pan the Bookmarks list.

Expected results:
'Switch to tab' is displayed under the opened page titles.

Actual results:
'Switch to tab' disappears and the URL is displayed on some bookmarks.

Comment 1

[Cristina Madaras, QA [:CristinaM]]

I tried to find a regression, but the issue is reproducible even on builds from January 2014.

Comment 2

[Mark Capella [:capella]]

Related to Bug 1072020 ? Or just similar ?

Comment 3

[Mark Capella [:capella]]

I can't reproduce this on my SGS3 or my N7.

If this is still reproducible for you, can you describe your step 3) above ... what "another panel" are you on when you expected to see Tabs you've openned from long-tap on the Bookmark panel, with "Switch to tabs" indicators (but don't).

Most obviously "History" should and does for me. "Recent" won't until you close the tabs, and then of course the "Switch to tab" is no longer valid present, (Unless you still have the page open in a second tab still, etc.)

Readinglist doesn't provide the "Switch" info anymore.

Comment 4

[Cristina Madaras, QA [:CristinaM]]

(In reply to Mark Capella [:capella] from comment #3)
> I can't reproduce this on my SGS3 or my N7.
> 
> If this is still reproducible for you, can you describe your step 3) above
> ... what "another panel" are you on when you expected to see Tabs you've
> openned from long-tap on the Bookmark panel, with "Switch to tabs"
> indicators (but don't).
> 
> Most obviously "History" should and does for me. "Recent" won't until you
> close the tabs, and then of course the "Switch to tab" is no longer valid
> present, (Unless you still have the page open in a second tab still, etc.)
> 
> Readinglist doesn't provide the "Switch" info anymore.

The issue is still reproducible.
At step 3: Swipe to 'Synced tabs' panel and then go back to Bookmarks panel. All bookmarks opened at step 2 should have the 'Switch to tab' indicator, but some of them don't.

Please check the video: https://www.youtube.com/watch?v=EjznR5pATo4&feature=youtu.be

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 6

[Mark Capella [:capella]]

oic! addons.mozilla.org and support.mozilla.org pull a localization switch ...

When you tap the bookmark to navigate to |https://addons.mozilla.org/android/| for example , you wind up on |https://addons.mozilla.org/en-US/android/| ... so the source URL isn't open in another tab  :-)

Comment 7

[Mark Capella [:capella]]

Attachment: bug1080374.diff

This bit of special-casing seems to work, though my assumptions about the way localized URLS are generated is based simply on observations and testing, and without specific code knowledge in that area.

Also, part of me leans towards wontfix-ing this, as technically the behavior is currently correct, if somewhat confusing.

Comment 8

[Mark Capella [:capella]]

Attachment: bug1080374.diff

Apologies for spam, I'd modified the main method and forgot to update the doc.

Comment 9

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #7)

> This bit of special-casing seems to work, though my assumptions about the
> way localized URLS are generated is based simply on observations and
> testing, and without specific code knowledge in that area.
> 
> Also, part of me leans towards wontfix-ing this, as technically the behavior
> is currently correct, if somewhat confusing.

Looking at the patch, I'm also leaning that way. I think you have the correct intuition that this is too much special-cased logic. This poses a problem for any bookmarks that redirect, so I don't think we should land a fix that's specific to a few Mozilla URLs.

Another fix we could explore is actually making default bookmarks with the correctly localized URLs. However, that would cause a problem for users who switch their default locale.

Comment 10

[:Margaret Leibovic]

Comment on attachment 8510971 [details] [diff] [review]
bug1080374.diff

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

I don't think we should pursue this approach.

Comment 11

[Mark Capella [:capella]]

Attachment: bug1080374.diff

This marginally improves the situation.

Currently, if the user long-presses a bookmark and opens one of the two Mozilla localizable items in a new tab, the "open in tab" indicator stays displayed until they force a UI update (as noted in the original description) by swiping panels.

With this patch, after the user long-taps to open in a new tab, the open-in-tab indicator in the bookmark panel will briefly be displayed, but won't be visible after the page LOCATION_CHANGE event quickly occurs.

(The UI stays logically consistent even without further user interaction).

Comment 12

[:Margaret Leibovic]

Comment on attachment 8514761 [details] [diff] [review]
bug1080374.diff

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

I'm still a bit skeptical of adding this special case, but this does address a valid user-facing issue, and in the worst case, we'll just call updateDisplayedUrl more often, which doesn't hurt anything. I just have some feedback about how to make this simpler.

::: mobile/android/base/home/TwoLinePageRow.java
@@ +122,5 @@
>       * <p>
>       * This method is always invoked on the UI thread.
>       */
>      @Override
> +    public void onTabChanged(final Tab tab, final Tabs.TabEvents msg, final Object originalURL) {

The data parameter is only the original URL in the case of the LOCATION_CHANGE event, so I would keep the parameter named data. That would also be consistent with all our other onTabChanged implementations.

However, looking more closely into our tab events, it looks like we only use this data parameter ever for the LOCATION_CHANGE event. Maybe we should update the OnTabsChangedListener interface to just pass an "originalURL" String, rather than a generalized data Object. But that should happen in another bug :)

@@ -133,5 @@
>          if (tab == null) {
>              return;
>          }
> -        tabUrl = tab.getURL();
> -        if (!pageUrl.equals(tabUrl)) {

We'll want to make sure we keep this early return for the other tab event cases.

@@ +139,5 @@
> +        if (msg == Tabs.TabEvents.LOCATION_CHANGE) {
> +            if (pageUrl.equals(changedURL) ||
> +                pageUrl.equals(originalURL.toString())) {
> +                updateDisplayedUrl();
> +            }

Since the data parameter will only ever hold this original URL or the empty String, you could just do:

// Return early if the page URL doesn't match the current tab URL, or the old tab URL.
if (!pageUrl.equals(tabUrl) && !pageUrl.equals(data)) {
  return;
}

And then keep the LOCATION_CHANGE case down below.

Comment 13

[Mark Capella [:capella]]

Attachment: bug1080374.diff

Ick, yah, I'd worked backwards from my previous patch and left too much cruft ... this is good :)

Comment 14

[:Margaret Leibovic]

Comment on attachment 8515252 [details] [diff] [review]
bug1080374.diff

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

Nice.

Comment 15

[Mark Capella [:capella]]

try-server
https://tbpl.mozilla.org/?tree=Try&rev=584f9405bd92

Comment 16

[Mark Capella [:capella]]

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

Comment 17

[Phil Ringnalda (:philor)]

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