Use URL of page as the title if no title exists

VERIFIED FIXED in Firefox 3 beta5

Status

()

P2
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: reed, Assigned: mak)

Tracking

Trunk
Firefox 3 beta5
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

11 years ago
Created attachment 305246 [details]
Screenshot

Comment 2

11 years ago
This is very strange for text/plain visits; For instance http://wwwdev.bris.ac.uk/robots.txt 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
(Assignee)

Comment 4

11 years ago
Created attachment 305991 [details]
screenshot with ellipsis

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?
(Assignee)

Comment 5

11 years ago
Created attachment 305993 [details]
screenshot of alternative styles
(Assignee)

Updated

11 years ago
Blocks: 413979
(Assignee)

Updated

11 years ago
Keywords: uiwanted

Updated

11 years ago
Assignee: nobody → dietrich
(Assignee)

Comment 6

11 years ago
dietrich i can take this, i only need final UI team definition on the content to put there.
Assignee: dietrich → mak77
(Assignee)

Updated

11 years ago
Attachment #305991 - Flags: ui-review?(beltzner)
Target Milestone: --- → Firefox 3
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

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

Comment 8

11 years ago
Created attachment 307953 [details] [diff] [review]
patch

implements host/ellipsis/filename solution
Attachment #307953 - Flags: review?(mano)
Comment on attachment 307953 [details] [diff] [review]
patch

>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["@mozilla.org/preferences-service;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-
(Assignee)

Comment 10

11 years ago
Created attachment 308387 [details] [diff] [review]
patch

fixed comments
Attachment #307953 - Attachment is obsolete: true
Attachment #308387 - Flags: review?(mano)
Comment on attachment 308387 [details] [diff] [review]
patch

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+
(Assignee)

Comment 12

11 years ago
what's the difference? nodeIsURI does that check... Do you think that future changes to nodeIsURI could create problems here?
(Assignee)

Comment 13

11 years ago
Created attachment 308388 [details] [diff] [review]
for check-in
Attachment #308387 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Keywords: uiwanted → checkin-needed
Whiteboard: [swag: 1h] → [has patch][has review]
Won't this throw for non-nsStandardURLs, data: URIs for example?
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
(Assignee)

Updated

11 years ago
Attachment #308388 - Attachment is obsolete: true
(Assignee)

Comment 15

11 years ago
Created attachment 308399 [details] [diff] [review]
patch

thank you Gavin, while fixing the possible throw, i ended up thinking at other cases. 

So:

- 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 www.mozilla.org/one/ from www.mozilla.org/two/, while before we had only www.mozilla.org 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.
(Assignee)

Comment 17

11 years ago
Comment on attachment 308399 [details] [diff] [review]
patch

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)
(Assignee)

Comment 18

11 years ago
Created attachment 308478 [details] [diff] [review]
v2

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
else
- 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)
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][has review] → [has patch][needs review mano]
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Whiteboard: [has patch][needs review mano] → [has patch][has review]
(Reporter)

Comment 20

11 years ago
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
done
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
done
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
done
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
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
Target Milestone: Firefox 3 → Firefox 3 beta5

Comment 21

11 years ago
I've lost track, a bit, on what this is supposed to do; It is showing |bugzilla.mozilla.org/../attachment.cgi| 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]
(Assignee)

Comment 22

11 years ago
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
Status: RESOLVED → VERIFIED
(Assignee)

Updated

11 years ago
Duplicate of this bug: 413979
(Assignee)

Updated

10 years ago
Duplicate of this bug: 419649
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.

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