If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

If you quickly close a tab whilst it's loading, in History > Recently Closed Tabs it can be titled 'Loading...'

VERIFIED FIXED in Firefox 3.1b1

Status

()

Firefox
Session Restore
--
minor
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: stevee, Assigned: Simon Bünzli)

Tracking

({polish})

unspecified
Firefox 3.1b1
polish
Points:
---
Bug Flags:
blocking1.8.1.1 -
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1) Gecko/20061003 BonEcho/2.0 ID:2006100303

From onemen @ http://forums.mozillazine.org/viewtopic.php?p=2521916#2521916

1. New profile, start firefox.
2. Middle click on a link (eg: in the bbc news rss feed.)
3. Quick as you can, close the tab
4. Check in History > Recently Closed Tabs

Actual:
If you were fast enough at step 3, then the closed tab is listed as 'Loading...' in the Recently Closed Tabs history.

Expected:
Not sure actually. Closed tab should be listed with it's proper title, or maybe not listed at all in this case. Being listed in the Recently Closed Tabs history as 'Loading...' isn't very useful, but really, I don't know what to suggest.
(Reporter)

Updated

11 years ago
Severity: normal → minor
(Reporter)

Updated

11 years ago
Flags: blocking1.8.1.1?

Comment 1

11 years ago
Created attachment 241743 [details] [diff] [review]
propose fix

if the tab is loading replace Loading... with the document title

Comment 2

11 years ago
Yeah moreover if you click on that link in the History menu, it brings up the same page in the current tab.
Not blocking the release, but we'll consider the patch for the branch if it gets reviewed and landed on the trunk. (Add the approval1.8.1.1? flag to the patch once that's happened)
Flags: blocking1.8.1.1? → blocking1.8.1.1-
(Assignee)

Updated

10 years ago
Component: History → Session Restore
Keywords: polish
QA Contact: history → session.restore
(Assignee)

Comment 4

9 years ago
Created attachment 334669 [details] [diff] [review]
fix

This is mostly onemen's patch with one less dependency on <tabbrowser> internals and one edge case fixed where document's didn't have a title but shouldn't have been "(Untitled)".
Attachment #241743 - Attachment is obsolete: true
Attachment #334669 - Flags: review?(dietrich)

Comment 5

9 years ago
Simon why not to use setTabTitle 

if (tabTitle == browserBundle.GetStringFromName("tabs.loading")) {
  aWindow.getBrowser().setTabTitle(aTab)
  tabTitle = aTab.getAttribute("label");
}
(Assignee)

Comment 6

9 years ago
Created attachment 334713 [details] [diff] [review]
alternative fix

Simpler fix as suggested by onemen, it however relies on side-effects.
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Attachment #334713 - Flags: review?(dietrich)

Comment 7

9 years ago
even simpler....

add this.setTabTitle(aTab) to _beginRemoveTab method
(Assignee)

Comment 8

9 years ago
Created attachment 334765 [details] [diff] [review]
compromise

I'd prefer keeping the changes required to fix this bug to SessionStore itself, as there's otherwise hardly an argument for changing the label of a closing tab.

Still, setTabTitle does a much better job of mimicking itself than we ever could, so let's use that one - just undo the label change once we no longer need it (so event listeners after us get pretty much the same browser state as we got). Then again, if we already rely on setTabTitle, why not use mStringBundle as well?
Attachment #334669 - Attachment is obsolete: true
Attachment #334713 - Attachment is obsolete: true
Attachment #334765 - Flags: review?(dietrich)
Attachment #334669 - Flags: review?(dietrich)
Attachment #334713 - Flags: review?(dietrich)
Comment on attachment 334765 [details] [diff] [review]
compromise

r=me. please make sure that this doesn't cause visible changes to the tab label while it's loading.
Attachment #334765 - Flags: review?(dietrich) → review+
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)
> please make sure that this doesn't cause visible changes to the tab label

It doesn't, as we change it and change it back without leaving time for a visual update.
Keywords: checkin-needed
Please update the patch, and please use gBrowser instead of getBrowser().
Keywords: checkin-needed

Comment 12

9 years ago
proposal, split setTabTitle to getTitleForTab and setTabTitle. then there is no risk at all.

-     <method name="setTabTitle">
+     <method name="getTitleForTab">
        <parameter name="aTab"/>
        <body>
          <![CDATA[
            var browser = this.getBrowserForTab(aTab);
            var crop = "end";
            var title = browser.contentTitle;

            if (!title) {
              if (browser.currentURI.spec) {
                try {
                  title = this.mURIFixup.createExposableURI(browser.currentURI).spec;
                } catch(ex) {
                  title = browser.currentURI.spec;
                }
              }

              if (title && title != "about:blank") {
                // At this point, we now have a URI.
                // Let's try to unescape it using a character set
                // in case the URI is not ASCII.
                try {
                  var characterSet = browser.contentDocument.characterSet;
                  const textToSubURI = Components.classes["@mozilla.org/intl/texttosuburi;1"]
                                                 .getService(Components.interfaces.nsITextToSubURI);
                  title = textToSubURI.unEscapeNonAsciiURI(characterSet, title);
                } catch(ex) { /* Do nothing. */ }

                crop = "center";

              } else // Still no title?  Fall back to our untitled string.
                title = this.mStringBundle.getString("tabs.untitled");
            }

+           return title;
+         ]]>
+       </body>
+     </method>
+
+     <method name="setTabTitle">
+       <parameter name="aTab"/>
+       <body>
+         <![CDATA[
-           aTab.label = title;
+           aTab.label = this.getTitleForTab(aTab);
            aTab.setAttribute("crop", crop);
          ]]>
        </body>
      </method>
      
and in SessionStoreService:
     // store closed-tab data for undo
     if (tabState.entries.length > 1 || tabState.entries[0].url != "about:blank") {
+      let tabTitle = aTab.label;
+      let tabbrowser = aWindow.gBrowser;
+      // replace "Loading..." with the document title
+      if (tabTitle == tabbrowser.mStringBundle.getString("tabs.loading")) {
+        tabTitle = tabbrowser.getTitleForTab(aTab);
+      }
+      
       this._windows[aWindow.__SSi]._closedTabs.unshift({
         state: tabState,
-        title: aTab.getAttribute("label"),
+        title: tabTitle,
         image: aTab.getAttribute("image"),
         pos: aTab._tPos
       });
       var length = this._windows[aWindow.__SSi]._closedTabs.length;
       if (length > maxTabsUndo)
         this._windows[aWindow.__SSi]._closedTabs.splice(maxTabsUndo, length - maxTabsUndo);
     }
   },
(Assignee)

Comment 13

9 years ago
Created attachment 337193 [details] [diff] [review]
unbitrotted for check-in
Attachment #334765 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
OS: Windows 2000 → All
Hardware: PC → All
Version: 2.0 Branch → unspecified
http://hg.mozilla.org/mozilla-central/rev/618823bd363f

(In reply to comment #12)
> proposal, split setTabTitle to getTitleForTab and setTabTitle. then there is no
> risk at all.

Could you please file a new bug for that?
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b1
Steve, can you verify if this is fixed yet on the latest beta 1 build?  I dont see it on mac, but you reported it on windows.   Thanks.

Comment 16

9 years ago
Loading tabs don't have also an image. so if we close tab while it is loading we don't see the tab image in to closed tabs list
Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090617 Minefield/3.6a1pre ID:20090617031528
Status: RESOLVED → VERIFIED
Flags: in-litmus?
in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7792
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.