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

RESOLVED FIXED in Firefox 36

Status

()

P5
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: CristinaM, Assigned: capella)

Tracking

35 Branch
Firefox 36
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox32 affected, firefox33 affected, firefox34 affected, firefox35 affected, firefox36 affected, fennec+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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: --- → ?
(Reporter)

Comment 1

4 years ago
I tried to find a regression, but the issue is reproducible even on builds from January 2014.
tracking-fennec: ? → +
(Assignee)

Comment 2

4 years ago
Related to Bug 1072020 ? Or just similar ?
(Reporter)

Updated

4 years ago
status-firefox36: --- → affected
(Assignee)

Comment 3

4 years ago
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)
(Reporter)

Comment 4

4 years ago
(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
(Assignee)

Comment 6

4 years ago
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  :-)
(Assignee)

Comment 7

4 years ago
Created attachment 8510965 [details] [diff] [review]
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.
Attachment #8510965 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 8

4 years ago
Created attachment 8510971 [details] [diff] [review]
bug1080374.diff

Apologies for spam, I'd modified the main method and forgot to update the doc.
Attachment #8510971 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

4 years ago
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-
(Assignee)

Comment 11

4 years ago
Created attachment 8514761 [details] [diff] [review]
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).
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+

Updated

4 years ago
Assignee: nobody → markcapella
(Assignee)

Comment 13

4 years ago
Created attachment 8515252 [details] [diff] [review]
bug1080374.diff

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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.