Closed Bug 419216 Opened 16 years ago Closed 16 years ago

Use URL of page as the title if no title exists


(Firefox :: Bookmarks & History, defect, P2)




Firefox 3 beta5


(Reporter: reed, Assigned: mak)




(4 files, 4 obsolete files)

Bug 406528 caused bad titles to be removed. That's great, but it makes history look very awkward. I think a better approach would be to use the URL of the page as the title so it isn't just completely blank.

Another possibility is saying "(no title)" like the Places Library does for titleness pages instead of just being blank.

Screenshot to be attached.
Flags: blocking-firefox3?
Attached image Screenshot
This is very strange for text/plain visits; For instance gives 'robots.txt' on 2 and is blank on 3.0b4pre (cf images 'xyz.jpg (JPEG image, 123 x 145 pixels)'
I agree with Reed, but don't think that we should be *setting* the title to the URL, just *using* the URL when there's no title information to display. Nicer still would be to strip the protocol so it's just the domain name.

I note that the back/forward drop down already seems to do this.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
should be an easy fix on menus
what about showing host and filename with ellipsis? do you think that the full path would be more useful?
Blocks: 413979
Keywords: uiwanted
Assignee: nobody → dietrich
dietrich i can take this, i only need final UI team definition on the content to put there.
Assignee: dietrich → mak77
Attachment #305991 - Flags: ui-review?(beltzner)
Target Milestone: --- → Firefox 3
Whiteboard: [needs UI review][swag: 1h]
Comment on attachment 305991 [details]
screenshot with ellipsis

nice, ui-r=beltzner
Attachment #305991 - Flags: ui-review?(beltzner) → ui-review+
Whiteboard: [needs UI review][swag: 1h] → [swag: 1h]
Attached patch patch (obsolete) — Splinter Review
implements host/ellipsis/filename solution
Attachment #307953 - Flags: review?(mano)
Comment on attachment 307953 [details] [diff] [review]

>Index: browser/components/places/content/utils.js

>   getFormattedString: function PU_getFormattedString(key, params) {
>     return this._bundle.formatStringFromName(key, params, params.length);
>   },
>   getString: function PU_getString(key) {
>     return this._bundle.GetStringFromName(key);
>   },
>+  getEllipsis: function PU_getEllipsis() {
>+    if (!this._ellipsis) {
>+      try {
>+        var pref = Components.classes[";1"].
>+                              getService(Components.interfaces.nsIPrefBranch);
>+        this._ellipsis = pref.getComplexValue("intl.ellipsis",
>+                          Components.interfaces.nsIPrefLocalizedString).data;
>+      } catch (ex) {
>+        return this._ellipsis = "\u2026";
>+      }
>+    }
>+    return this._ellipsis;
>+  },

Please use a smart getter here (delete on first access). What's the try/catch block for? isn't it set by default

Cc/Ci, too.

>   /**
>    * Determines whether or not a ResultNode is a Bookmark folder.
>    * @param   aNode
>    *          A result node
>    * @returns true if the node is a Bookmark folder, false otherwise
>    */
>   nodeIsFolder: function PU_nodeIsFolder(aNode) {
>     NS_ASSERT(aNode, "null node");
>@@ -1872,17 +1886,26 @@ var PlacesUtils = {
>         element.appendChild(popup);
>         if (aContainersMap)
>           aContainersMap.push({ resultNode: aNode, domNode: popup });
>         element.className = "menu-iconic bookmark-item";
>       }
>       else
>         throw "Unexpected node";
>-      element.setAttribute("label", aNode.title);
>+      if (!aNode.title && aNode.uri) {

This should rather be an uriTypes.indexOf check, otherwise this would label untitled folders as well!
Attachment #307953 - Flags: review?(mano) → review-
Attached patch patch (obsolete) — Splinter Review
fixed comments
Attachment #307953 - Attachment is obsolete: true
Attachment #308387 - Flags: review?(mano)
Comment on attachment 308387 [details] [diff] [review]

I still prefer an explicit check against urlTypes (like we do for containerTypes in the same function). r=mano either way.
Attachment #308387 - Flags: review?(mano) → review+
what's the difference? nodeIsURI does that check... Do you think that future changes to nodeIsURI could create problems here?
Attached patch for check-in (obsolete) — Splinter Review
Attachment #308387 - Attachment is obsolete: true
Keywords: uiwantedcheckin-needed
Whiteboard: [swag: 1h] → [has patch][has review]
Won't this throw for non-nsStandardURLs, data: URIs for example?
Keywords: checkin-needed
Attachment #308388 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
thank you Gavin, while fixing the possible throw, i ended up thinking at other cases. 


- for non standard uris we show the full uri

- if we don't have a fileName we should use the path (hiding the scheme as originally requested by beltzner, so only host + path), so we can easily distinguish from, while before we had only in both cases, and that was bad

- ellipsis should be used only if we have both host and fileName

since something more has changed i'm asking a new review
Attachment #308399 - Flags: review?(mano)
> - for non standard uris we show the full uri

I think we should rather use treeview's (no title) in these edge cases. very long data: uris aren't so useful.
Comment on attachment 308399 [details] [diff] [review]

will change the edge case of non standard uris with (no title), since they could be very long. We still have the tooltip and statusbartext to distinguish them.
Attachment #308399 - Attachment is obsolete: true
Attachment #308399 - Flags: review?(mano)
Attached patch v2Splinter Review
next step, we are going to use the same title algo on all menupopup/tree views in places. This patch works fine with liveUpdate too.

For items directly on toolbar we still use the empty title, to allow users have toolbars made up of only icons.

For menupopups and trees (sidebar/Library) we are using a new util that act as following:

- if node has a title we use that
- if node has a valid host and fileName, use host + ellipsis + fileName
- if no fileName, use host + path
- if node has non-standard uri use (no title)
- if node does not have an uri (ie: folder) use (no title)
Attachment #308478 - Flags: review?(mano)
Whiteboard: [has patch][has review] → [has patch][needs review mano]
Comment on attachment 308478 [details] [diff] [review]

Attachment #308478 - Flags: review?(mano) → review+
Keywords: checkin-needed
Whiteboard: [has patch][needs review mano] → [has patch][has review]
Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.117; previous revision: 1.116
Checking in browser/components/places/content/toolbar.xml;
/cvsroot/mozilla/browser/components/places/content/toolbar.xml,v  <--  toolbar.xml
new revision: 1.140; previous revision: 1.139
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.56; previous revision: 1.55
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.118; previous revision: 1.117
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
Target Milestone: Firefox 3 → Firefox 3 beta5
I've lost track, a bit, on what this is supposed to do; It is showing || on the history menu - as I would expect - but I am seeing the full URL in the back/forward drop down [However this does match the tab title]
this affects history menu, bookmark menu, toolbar menus
verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.