Closed Bug 436758 Opened 12 years ago Closed 10 years ago

Show tooltip and URL in status bar when hovering over a link inside "Recently Closed Tabs/Windows"

Categories

(Firefox :: Menus, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 3.7a1

People

(Reporter: Gabri, Assigned: Gabri)

References

Details

(Keywords: polish, Whiteboard: [polish-easy][polish-interactive][polish-p3])

Attachments

(1 file, 12 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.9) Gecko/2008052906 Firefox/3.0

Hovering over a link in the "Recently Closed Tabs" menu (under history menu) should display the bookmarks URL in the status bar.

Reproducible: Always
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
(In reply to comment #0)
> should display the bookmarks URL in the status bar.

Reopening a closed tab is really not the same as navigating to a bookmark's URL - in fact, we ideally don't navigate at all, which is however what the URL displayed in the status bar implies. Recommending WONTFIX.
Whiteboard: [wontfix?]
I don't think it's an obvious WONTFIX, it'll open that URL back up, so I don't think it's useless feedback.  Not at all critical though.
Priority: -- → P5
Whiteboard: [wontfix?]
Target Milestone: --- → Future
Attached patch patch (obsolete) — Splinter Review
This feedback is useful as it's difficult to distinguish similar tabs on the menu (it's often not wide enough to display the full title).

Thus, a patch!

This patch also displays a tooltip for the closed tabs (same tooltip as the history menu)
Thanks for the patch. You should ask for review. I'll do it right now.
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Whiteboard: [polish-easy][polish-interactive]
Comment on attachment 365064 [details] [diff] [review]
patch

Drive by nit-picking:

>-    m.setAttribute("oncommand", "undoCloseTab(" + i + ");");
>+    m.setAttribute("oncommand", "undoCloseTab(" + i + ");");

Nit: Somehow a DOS line ending managed to sneak in here.

>+    m.node = { type: 0, title: undoItems [i].title, uri: undoItems [i].state.entries [undoItems [i].state.index - 1].url };

Nits: Overlong line (limit it to 80 characters if possible); there shouldn't be a space between undoItems and the opening bracket; and commenting the magic 0 would make the code easier to read (if there's no named constant you could use instead).

Finally: It is theoretically possible that undoItems[i].state.index isn't defined or points to an invalid entry. See <http://hg.mozilla.org/mozilla-central/file/d3a9f84b14d0/browser/components/sessionstore/src/nsSessionStore.js#l1982> for what we usually do before accessing .entries.
This bug should also cover the new "Recently Closed Windows" menu, so I've updated the summary.
Summary: Show URL in status bar when hovering over a link inside "Recently Closed Tabs" → Show URL in status bar when hovering over a link inside "Recently Closed Tabs/Windows"
Attached patch patch v2 (obsolete) — Splinter Review
... and the patch.

Hopefully less for you nit-pickers to complain about ;)
Attachment #365064 - Attachment is obsolete: true
Attachment #365064 - Flags: review?(gavin.sharp)
Attachment #375495 - Flags: review?(gavin.sharp)
This bug's priority relative to the set of other polish bugs is:
P3 - Polish issue that is in a secondary interface, occasionally encountered, or is not easily identifiable.

I'm not expecting a lot of people to notice the change, or know to look at the status bar when hovering in a menu's sub menu.
Whiteboard: [polish-easy][polish-interactive] → [polish-easy][polish-interactive][polish-p3]
Flags: wanted-firefox3.6?
Attached patch patch v3 (obsolete) — Splinter Review
updated due to the functions patched moving from browser.js to browser-places.js
Attachment #375495 - Attachment is obsolete: true
Attachment #388018 - Flags: review?(gavin.sharp)
Attachment #375495 - Flags: review?(gavin.sharp)
Attached patch patch v3 + a bit (obsolete) — Splinter Review
Missed a moving target: previous patch gave status bar info but no tooltip. Fixed.
Attachment #388018 - Attachment is obsolete: true
Attachment #388048 - Flags: review?(gavin.sharp)
Attachment #388018 - Flags: review?(gavin.sharp)
Comment on attachment 388048 [details] [diff] [review]
patch v3 + a bit

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js

>+      let activeIndex = (tabData.index || tabData.entries.length) - 1;

Is that -1 supposed to be inside the parentheses? It's a bit strange to be potentially subtracting one from an "index" and assigning directly to the another "index"...

Using places-menupopup for this seems to be introducing a lot of unneeded overhead. Perhaps you could factor out the non-places related logic in fillInBTTooltip and use that instead (and update fillInBTTooltip to also use the newly factored out code)?
(In reply to comment #12)
> It's a bit strange to be potentially subtracting one from an "index" and
> assigning directly to the another "index"...

SessionStore uses one-based indexes for data saved to sessionstore.js, in order to easily fall back to a reasonable default, just as seen in that line you quote.
Attached patch patch v4 (obsolete) — Splinter Review
Not sure if this is what you mean in comment #12 Gavin, but refactoring out the statusbar code made sense.
Attachment #388048 - Attachment is obsolete: true
Attachment #388048 - Flags: review?(gavin.sharp)
Attachment #389263 - Flags: review?(gavin.sharp)
Attached patch a simpler patch (obsolete) — Splinter Review
I updated the code of the patch and removed the useless code
Assignee: geoff → gabri.best
Attachment #389263 - Attachment is obsolete: true
Attachment #411411 - Flags: review?(mak77)
Attachment #389263 - Flags: review?(gavin.sharp)
Comment on attachment 411411 [details] [diff] [review]
a simpler patch

sorry but we don't want fake Places nodes that can potentially create obscure bugs.
Attachment #411411 - Flags: review?(mak77) → review-
Attachment #411411 - Attachment is obsolete: true
Attachment #411509 - Flags: review?(mak77)
Comment on attachment 411509 [details] [diff] [review]
Gabri's patch, v0.2 (works with patch of bug 476838)

comment 12 has already expressed concerns about using type="places"
I don't have clean ideas for now but will keep thinking
Attachment #411509 - Flags: review?(mak77)
Attached patch Gabri's patch, v0.3 (obsolete) — Splinter Review
Attachment #411509 - Attachment is obsolete: true
Attachment #411755 - Flags: review?(mak77)
Attachment #411755 - Attachment description: Gabri's patch, v0.3 (works with patch of bug 476838) → Gabri's patch, v0.3
Attachment #411755 - Attachment is obsolete: true
Attachment #411758 - Flags: review?(mak77)
Attachment #411755 - Flags: review?(mak77)
Attachment #411758 - Flags: review?(mak77)
Attached patch Gabri's patch, v0.4 (obsolete) — Splinter Review
Attachment #411758 - Attachment is obsolete: true
Attachment #412572 - Flags: review?(mak77)
Attached patch Gabri's patch, v0.5 (obsolete) — Splinter Review
Attachment #412572 - Attachment is obsolete: true
Attachment #412592 - Flags: review?(mak77)
Attachment #412572 - Flags: review?(mak77)
Depends on: tooltips
Attached patch Gabri's patch, v0.6 (obsolete) — Splinter Review
Attachment #412592 - Attachment is obsolete: true
Attachment #413811 - Flags: review?(mak77)
Attachment #412592 - Flags: review?(mak77)
using "placespopup" will apply the place-popup-base binding, that will also enable D&D on the popup. the approach is not so bad, i guess if we need to further split the binding into places-popup-base and places-popup-dragdrop (extends places-popup-base) and make places-menupopup extend places-popup-dragdrop.
But for now i think you could just test that trying to D&D from and to the popup does not show any strict js error or any other kind of error (And clearly that it does not allow to drop), they should already be able to handle static nodes, so ideally there should be no problem, but i'd like some testing regarding that before.
Summary: Show URL in status bar when hovering over a link inside "Recently Closed Tabs/Windows" → Show tooltip and URL in status bar when hovering over a link inside "Recently Closed Tabs/Windows"
Ok, I didn't find any javascript errors/problems with that patch I think we could land it for now.
Attachment #413811 - Flags: review?(mak77) → review+
Comment on attachment 413811 [details] [diff] [review]
Gabri's patch, v0.6

ok, let's get nightly testers feedback.

>diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js
>+++ b/browser/base/content/browser-places.js
>@@ -632,16 +632,25 @@ var HistoryMenu = {
>         // don't initiate a connection just to fetch a favicon (see bug 467828)
>         if (/^https?:/.test(iconURL))
>           iconURL = "moz-anno:favicon:" + iconURL;
>         m.setAttribute("image", iconURL);
>       }
>       m.setAttribute("class", "menuitem-iconic bookmark-item");
>       m.setAttribute("value", i);
>       m.setAttribute("oncommand", "undoCloseTab(" + i + ");");
>+
>+      // Set the targetURI attribute so it will be shown in tooltip and statusbar

period at the end of the phrase please.

>+      // SessionStore uses one-based indexes for data saved to sessionstore.js
>+      // so we need to normalize it.

i'd just say "SessionStore uses one-based indexes, so we need to normalize them."

>+      // Set the targetURI attribute so it will be shown in tooltip and statusbar.
>+      // SessionStore uses one-based indexes for data saved to sessionstore.js
>+      // so we need to normalize it.

ditto
Ok, done.
Attachment #413811 - Attachment is obsolete: true
Attachment #416053 - Flags: review?(mak77)
Attachment #416053 - Flags: review?(mak77) → review+
Flags: wanted-firefox3.6?
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/215ca7a9e2ba
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Future → Firefox 3.7a1
Blocks: tooltips
No longer depends on: tooltips
You need to log in before you can comment on or make changes to this bug.