Closed Bug 1080374 Opened 6 years ago Closed 6 years ago

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

Categories

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

35 Branch
ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
firefox32 --- affected
firefox33 --- affected
firefox34 --- affected
firefox35 --- affected
firefox36 --- affected
fennec + ---

People

(Reporter: CristinaM, Assigned: capella)

Details

Attachments

(1 file, 3 obsolete files)

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.
tracking-fennec: --- → ?
I tried to find a regression, but the issue is reproducible even on builds from January 2014.
tracking-fennec: ? → +
Related to Bug 1072020 ? Or just similar ?
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.
Flags: needinfo?(cristina.madaras)
(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
Flags: needinfo?(cristina.madaras)
filter on [mass-p5]
Priority: -- → P5
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  :-)
Attached patch bug1080374.diff (obsolete) — Splinter Review
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.
Attachment #8510965 - Flags: review?(margaret.leibovic)
Attached patch bug1080374.diff (obsolete) — Splinter Review
Apologies for spam, I'd modified the main method and forgot to update the doc.
Attachment #8510971 - Flags: review?(margaret.leibovic)
Attachment #8510965 - Flags: review?(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 on attachment 8510971 [details] [diff] [review]
bug1080374.diff

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

I don't think we should pursue this approach.
Attachment #8510971 - Flags: review?(margaret.leibovic) → review-
Attached patch bug1080374.diff (obsolete) — Splinter Review
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).
Attachment #8510965 - Attachment is obsolete: true
Attachment #8510971 - Attachment is obsolete: true
Attachment #8514761 - Flags: review?(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.
Attachment #8514761 - Flags: review?(margaret.leibovic) → feedback+
Assignee: nobody → markcapella
Attached patch bug1080374.diffSplinter Review
Ick, yah, I'd worked backwards from my previous patch and left too much cruft ... this is good :)
Attachment #8514761 - Attachment is obsolete: true
Attachment #8515252 - Flags: review?(margaret.leibovic)
Comment on attachment 8515252 [details] [diff] [review]
bug1080374.diff

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

Nice.
Attachment #8515252 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/c8eefd3e3ac3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.