Closed Bug 355284 Opened 18 years ago Closed 16 years ago

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

Tracking

()

Status:

VERIFIED FIXED

Milestone:

Firefox 3.1b1

People

(Reporter: stevee, Assigned: zeniko)

References

(
URL
)

Details

(Keywords: polish)

Attachments

(1 file, 4 obsolete files)

propose fix 18 years ago tabmix.onemen 1.74 KB, patch		Details \| Diff \| Splinter Review
fix 16 years ago Simon Bünzli 1.78 KB, patch		Details \| Diff \| Splinter Review
alternative fix 16 years ago Simon Bünzli 1.33 KB, patch		Details \| Diff \| Splinter Review
compromise 16 years ago Simon Bünzli 1.44 KB, patch	dietrich : review+	Details \| Diff \| Splinter Review
unbitrotted for check-in 16 years ago Simon Bünzli 1.39 KB, patch		Details \| Diff \| Splinter Review

Steve England [:stevee]

Reporter

Description

•

18 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.

Steve England [:stevee]

Reporter

Updated

•

18 years ago

Severity: normal → minor

Steve England [:stevee]

Reporter

Updated

•

18 years ago

Flags: blocking1.8.1.1?

tabmix.onemen

Comment 1

•

18 years ago

Attached patch propose fix (obsolete) — Details — Splinter Review

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

Tomal

Comment 2

•

18 years ago

Yeah moreover if you click on that link in the History menu, it brings up the same page in the current tab.

Daniel Veditz [:dveditz]

Comment 3

•

18 years ago

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-

Simon Bünzli

Assignee

Updated

•

17 years ago

Component: History → Session Restore

Keywords: polish

QA Contact: history → session.restore

Simon Bünzli

Assignee

Comment 4

•

16 years ago

Attached patch fix (obsolete) — Details — Splinter Review

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)

tabmix.onemen

Comment 5

•

16 years ago

Simon why not to use setTabTitle 

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

Simon Bünzli

Assignee

Comment 6

•

16 years ago

Attached patch alternative fix (obsolete) — Details — Splinter Review

Simpler fix as suggested by onemen, it however relies on side-effects.

Assignee: nobody → zeniko

Status: NEW → ASSIGNED

Attachment #334713 - Flags: review?(dietrich)

tabmix.onemen

Comment 7

•

16 years ago

even simpler....

add this.setTabTitle(aTab) to _beginRemoveTab method

Simon Bünzli

Assignee

Comment 8

•

16 years ago

Attached patch compromise (obsolete) — Details — Splinter Review

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)

Dietrich Ayala (:dietrich)

Comment 9

•

16 years ago

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+

Simon Bünzli

Assignee

Comment 10

•

16 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

Dão Gottwald [:dao]

Comment 11

•

16 years ago

Please update the patch, and please use gBrowser instead of getBrowser().

Keywords: checkin-needed

tabmix.onemen

Comment 12

•

16 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);
     }
   },

Simon Bünzli

Assignee

Comment 13

•

16 years ago

Attached patch unbitrotted for check-in — Details — Splinter Review

Attachment #334765 - Attachment is obsolete: true

Simon Bünzli

Assignee

Updated

•

16 years ago

Keywords: checkin-needed

OS: Windows 2000 → All

Hardware: PC → All

Version: 2.0 Branch → unspecified

Dão Gottwald [:dao]

Comment 14

•

16 years ago

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

Closed: 16 years ago

Keywords: checkin-needed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 3.1b1

Tony Chung [:tchung]

Comment 15

•

16 years ago

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.

tabmix.onemen

Comment 16

•

16 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

Michael Kohler [:mkohler]

Comment 17

•

15 years ago

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?

u279076

Comment 18

•

15 years ago

in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7792

URL: https://litmus.mozilla.org/show_test....

Flags: in-litmus? → in-litmus+

You need to log in before you can comment on or make changes to this bug.