In the context bar when searching in the library "selected folder" should be the actual folder name

VERIFIED FIXED in Firefox 3.6a1

Status

()

defect
P4
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: faaborg, Assigned: adw)

Tracking

({late-l10n, verified1.9.1})

Trunk
Firefox 3.6a1
Points:
---
Dependency tree / graph
Bug Flags:
wanted-firefox3.5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

I think I messed up in a mockup of Firefox 3 and forgot to specify that the text "selected folder" was meant to mean "folder name here" as opposed to literally "selected folder".  This context option should  be the literal name of the parent folder.

Also, "All Bookmarks" should just be "Bookmarks" in the context bar, since the All is implied (for instance we don't write All History).  We can fix that in a follow up bug if it is late-l10n, but I would imagine that we have the string "Bookmarks" available.
(In reply to comment #0)

> I think I messed up in a mockup of Firefox 3 and forgot to specify that the
> text "selected folder" was meant to mean "folder name here" as opposed to
> literally "selected folder".  This context option should  be the literal name
> of the parent folder.

i suppose we will have to cut it someway, or some folder name could be really long
Target Milestone: --- → Firefox 3.1
Priority: -- → P4
Whiteboard: [good first bug]
Assignee

Updated

10 years ago
Assignee: nobody → adw
Assignee

Comment 2

10 years ago
Posted patch v1 (obsolete) — Splinter Review
Solves the problem of truncating the folder title if it's too long by setting the flex of the folder button to 1 and the flex of the spacer to a really big number.  When the title is too long, it's automatically truncated with "..." at the end.  This feels like cheating, but it works.

Also, changed the scopeAll entity in the DTD from "All Bookmarks" to "Bookmarks".
Attachment #365562 - Flags: review?(mak77)
Assignee

Comment 3

10 years ago
Oh, and because it no longer make sense to disable the folder button when folder scope is inappropriate, it's now hidden instead.
Attachment #365562 - Flags: review?(mak77) → review-
Comment on attachment 365562 [details] [diff] [review]
v1

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>@@ -261,30 +273,18 @@ var PlacesOrganizer = {
>       PlacesQueryBuilder.setScope("history");
>     }
>     // Default to All Bookmarks for all other nodes, per bug 469437.
>     else
>       PlacesQueryBuilder.setScope("bookmarks");
> 
>     // Enable or disable the folder scope button.
>     var folderButton = document.getElementById("scopeBarFolder");
>-    folderButton.disabled = !PlacesUtils.nodeIsFolder(aNode) ||
>-                            itemId == PlacesUIUtils.allBookmarksFolderId;
>-    if (!folderButton.disabled) {
>-      // XXXadw When bug 469436 is fixed, set folder scope button label here
>-
>-      // Update the label of the "Find in <current collection>" command.
>-      // The label is not currently displayed anywhere, but for consistency
>-      // we'll update it anyway.  To be totally thorough we should also listen
>-      // for changes in the selected folder's title.
>-      var cmd = document.getElementById("OrganizerCommand_find:current");
>-      var label = PlacesUIUtils.getFormattedString("findInPrefix",
>-                                                   [aNode.title]);
>-      cmd.setAttribute("label", label);
>-    }
>+    folderButton.hidden = !PlacesUtils.nodeIsFolder(aNode) ||
>+                          itemId == PlacesUIUtils.allBookmarksFolderId;

i would move this button to the right end, for 2 reasons:
1. is no more our primary target (now we select Bookmarks by default)
2. it's growing and shrinking and hiding, so makes other buttons moving targets

>-              <spacer flex="1"/>
>+              <spacer flex="2147483647"/>

i hope there's a better way to do this :\
could you investigate using packed hboxes or eventually a grid?

>diff --git a/browser/locales/en-US/chrome/browser/places/places.dtd b/browser/locales/en-US/chrome/browser/places/places.dtd
>
> <!ENTITY search.scopeFolder.label                  "Selected Folder">
> <!ENTITY search.scopeFolder.accesskey              "F">
>-<!ENTITY search.scopeAll.label                     "All Bookmarks">
>+<!ENTITY search.scopeAll.label                     "Bookmarks">

when you change a string you have to generate a new entity, so translators can see the missing string, you can maybe use scopeBookmarks.label

r- for the entity, please investigate if there's a less hacky way to make that flexible too
Flags: wanted-firefox3.1+
Target Milestone: Firefox 3.1 → ---
Assignee

Comment 5

10 years ago
Posted patch v2 (obsolete) — Splinter Review
- Moves the folder scope button so it's the last button
- Caps the folder scope button to 30ch in length -- no more flexing.  Seems to be a reasonable size for en-US.  Enough for "Bookmarks Toolbar", "Unsorted Bookmarks", and "Bookmarks Menu" but not too long.
- The length cap and crop are set in places.dtd.  Using style="" to set max-width.  This seems better than using the maxwidth attribute because it allows localizers to use em and ch instead of only pixels.
- Changed scopeAll to scopeBookmarks in the DTD
Attachment #365562 - Attachment is obsolete: true
Attachment #366452 - Flags: review?(mak77)
Comment on attachment 366452 [details] [diff] [review]
v2

Asking a review from Axel, to be sure we are doing the right thing allowing localizers to set both the button maxwidth and crop.
I suppose crop="end" is already taking in count RTL.

What happens for a folder without a title? i guess for this case we should still show the old "Selected Folder" scope, since we would end up with an empty button otherwise.
Attachment #366452 - Flags: review?(l10n)

Comment 7

10 years ago
Comment on attachment 366452 [details] [diff] [review]
v2

r-, we shouldn't have "end" in the localized strings. Folks are just going to translate end, and not work.

I'd personally expect that if I see the button being cropped, and I resize the library window, that I'd get better display for the cropped strings.

Though, granted, the whole dialog doesn't react well to resizing for me.

I don't know where the width for the search field is specified, that might be a good guide. I'd use em as unit in any way, though. ch is a uncommon unit to CSS specs in our UI, and thus could lead to bad translation. In any way, it should get a rich localization note to describe things.
Attachment #366452 - Flags: review?(l10n) → review-

Updated

10 years ago
Keywords: late-l10n
Assignee

Updated

10 years ago
Attachment #366452 - Flags: review?(mak77)
Assignee

Comment 8

10 years ago
Posted patch v3 (obsolete) — Splinter Review
Uses JS to set the folder button's pre-flex maxwidth and then flex it.  That way the button is free to flex but will not take up more space than its title needs.  This is what I was looking for.

(In reply to comment #6)
> What happens for a folder without a title? i guess for this case we should
> still show the old "Selected Folder" scope, since we would end up with an empty
> button otherwise.

Good catch, thanks.  This patch uses PlaceUIUtils.getBestTitle(), so when the title is empty we get "(no title)", which is what the tree in the left pane displays for the folder.  However, getBestTitle() allows titles that are nonempty but contain only whitespace.  Maybe that's fine for the tree view, but I think we shouldn't allow that for the folder button because visually the button disappears (at least on OS X).  So I decided to use "(no title)" in that case also.  If you feel that using "Selected Folder" in this narrow case would be better because "(no title)" isn't technically correct, let me know.

(In reply to comment #7)
> (From update of attachment 366452 [details] [diff] [review])
> r-, we shouldn't have "end" in the localized strings. Folks are just going to
> translate end, and not work.

OK, I figured there might be some languages where cropping in the middle makes more sense than the end, but that's probably overthinking it.  It doesn't matter with this new patch anyway.
Attachment #366452 - Attachment is obsolete: true
Attachment #366650 - Flags: review?(mak77)
Comment on attachment 366650 [details] [diff] [review]
v3

new version coming i think
Attachment #366650 - Attachment is obsolete: true
Attachment #366650 - Flags: review?(mak77)
Assignee

Comment 10

10 years ago
Posted patch v4 (obsolete) — Splinter Review
Reverts back to the flexing method of patch v1, per IRC conversation with Marco.  As we also decided on, 1) the UI displays "Selected Folder" in the folder scope button when folder's title is empty, and 2) if the folder's title is nonempty whitespace, we don't do anything special.  I note that in case 2, the folder scope button visually disappears.

Also fixes a related problem:
For folders with empty titles, emptytext of search box is "Search Bookmarks" instead of "Search in Selected Folder".  With this patch the text is now actually "Search in 'Selected Folder'" (notice the quotes around Selected Folder), which is a small bit of ugliness since Selected Folder is not really the title.  We'd need a new string for this one corner case to fix it.
Attachment #366937 - Flags: review?(mak77)
Attachment #366937 - Flags: review?(mak77) → review+
Comment on attachment 366937 [details] [diff] [review]
v4

>diff --git a/browser/locales/en-US/chrome/browser/places/places.dtd b/browser/locales/en-US/chrome/browser/places/places.dtd
>--- a/browser/locales/en-US/chrome/browser/places/places.dtd
>+++ b/browser/locales/en-US/chrome/browser/places/places.dtd
>@@ -137,18 +137,18 @@
> <!ENTITY search.collection.label                   "Current Collection Only">
> <!ENTITY search.collection.accesskey               "C">
> <!ENTITY search.allBookmarks.label                 "All Bookmarks">
> <!ENTITY search.allBookmarks.accesskey             "A">

These look unused actually, i doubt we are going to use the "Collection" word still
I would remove them both since we use "Search Bookmarks" FROM places.properties and we are going toward discarding the "All bookmarks" idea in favor of the simple "Bookmarks"

> <!ENTITY search.scopeFolder.label                  "Selected Folder">
> <!ENTITY search.scopeFolder.accesskey              "F">
>-<!ENTITY search.scopeAll.label                     "All Bookmarks">
>-<!ENTITY search.scopeAll.accesskey                 "A">
>+<!ENTITY search.scopeBookmarks.label               "Bookmarks">
>+<!ENTITY search.scopeBookmarks.accesskey           "A">

Any reason to not use B as the accesskey?

> <!ENTITY search.scopeMenu.label                    "Bookmarks Menu">
> <!ENTITY search.scopeMenu.accesskey                "M">
> <!ENTITY search.scopeToolbar.label                 "Bookmarks Toolbar">
> <!ENTITY search.scopeToolbar.accesskey             "T">

These have gone time ago, we can remove them the user can restrict search there using the scope button.

> <!ENTITY search.scopeDownloads.label               "Downloads">
> <!ENTITY search.scopeDownloads.accesskey           "D">
> <!ENTITY search.scopeHistory.label                 "History">
> <!ENTITY search.scopeHistory.accesskey             "A">

History accesskey is A? why not H? Is it used anywhere else? A is an undiscoverable accesskey.
Changing an accesskey should not require changing the entity since are mostly relevant to en-us and other locales will most likely change them. We can ask Axel to be sure though.

Btw, would be cool if those accesskey would be associated to something! Actually they are unused :\

R+ on the code, but please take a look if you can fix these inconsistencies on accesskeys.
Whiteboard: [good first bug]
Assignee

Comment 12

10 years ago
Posted patch v5 (obsolete) — Splinter Review
Changes re: comment 11.

Removed search.accesskey, the accesskey on the "Search:" label to the left of the scope buttons.  It was the same as the "Save" search button accesskey, and it wasn't even being used.  Not sure how it would have been useful anyway; the "Search:" label's control attribute is not set, so it's not hooked up to any element.

Search scope button accesskeys are hooked up and set to "o" for bookmarks ("B" is taken by the back button), "H" for history, "l" for folder (lowercase L, "F" is taken by the forward button).
Attachment #366937 - Attachment is obsolete: true
Comment on attachment 367247 [details] [diff] [review]
v5

Axel, are accesskey changes fine for you?
Attachment #367247 - Flags: review?(l10n)
Assignee

Updated

10 years ago
Depends on: 451151
Did you mean to request ui-review? I don't quite understand what Axel is expected to review there (but maybe he does).
I only would like he taking a really fast look if accesskey changes in the dtd are ok from an intl point of view, i think they don't require an entity change right?
Comment on attachment 367247 [details] [diff] [review]
v5

'l' is a bad accesskey, and IIRC, 'o' ain't better.
Attachment #367247 - Flags: review?(l10n) → review-
Assignee

Comment 17

10 years ago
(In reply to comment #16)
OK Axel, could you provide some reasons why and maybe alternate suggestions?  I explained why I chose those in the last paragraph of comment 12.
https://developer.mozilla.org/en/XUL_Accesskey_FAQ_and_Policies#How_do_I_pick_an_accesskey_letter.3f has docs. It doesn't mention 'o', but I recall something about o, O, and 0 making that a bad choice.

Anyway, you should probably get the OK from someone in a11y on that.

From an l10n point of view, the changes to the accesskeys don't require entity key changes.
(In reply to comment #18)
> https://developer.mozilla.org/en/XUL_Accesskey_FAQ_and_Policies#How_do_I_pick_an_accesskey_letter.3f
> has docs. It doesn't mention 'o', but I recall something about o, O, and 0
> making that a bad choice.

Do you know who we should ask about this?

I'd rather not update that wiki page with "'O' is not allowed because of something that Pike can't remember." :)
Marco would be a good choice :)

Marco, we're debating good and bad accesskeys, and I have found docs that recommend to avoid 'l' (lower case ell), I'm not sure though if I've seen comments about 'o' elsewhere. 'o' vs 'O' vs '0' or something.
Assignee

Comment 21

10 years ago
For what it's worth, I understand the case against "l" after reading the devmo page you linked to, Axel.  But all of the letters in "folder" are taken elsewhere, and this patch changes the folder label to display the actual folder name anyway, so I'm not sure what to do.  But "o" is perfectly good for "Bookmarks".  I understand confusing O for zero, etc., but that argument holds up only if there's no context.  We're talking about the word "Bookmarks", which has two big fat O's.
Assignee

Comment 22

10 years ago
Posted patch v6 (obsolete) — Splinter Review
Simply changes the accesskey of the bookmarks scope button to "k" and the accesskey of the folder scope button to "r", per Marco B.'s suggestions.
Attachment #367247 - Attachment is obsolete: true
Attachment #367816 - Flags: review?(l10n)
Comment on attachment 367816 [details] [diff] [review]
v6

The l10n changes look good to me.
Attachment #367816 - Flags: review?(l10n) → review+
Assignee

Updated

10 years ago
Keywords: checkin-needed
Posted patch v7Splinter Review
a small update since search.accesskey was still i nuse in the sidebar
Attachment #367816 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/ce5169d70fbb
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Keywords: checkin-needed
Comment on attachment 367851 [details] [diff] [review]
v7

asking approval 1.9.1, this is late-l10n and implements a more consistent behavior Faaborg requested for the release.

Risk is medium, Drew has done a lot of testing on this though and 451151 has automatic tests (this does not have a specific since the final change is practically only a label change).

This patch requires approval on bug 451151 too, since is built upon those fixes.
Attachment #367851 - Flags: approval1.9.1?
Whiteboard: [baking on trunk]
Comment on attachment 367851 [details] [diff] [review]
v7

a191=beltzner
Attachment #367851 - Flags: approval1.9.1? → approval1.9.1+
This patch does not apply cleanly to mozilla-1.9.1
this is the full list of bugs needed (in order) before this:
bug 474831 (needs unbitrot in tests makefile)
bug 433231 + test (needs unbitrot in tests makefile)
bug 475331
bug 475390
bug 451151
bug 469436
All those patches now have a191, please make sure to grab the tests from them as well.

Marco: I bet Shawn's gonna teach you how to make an hg bundle sometime real soon now ;)
Keywords: checkin-needed
With a bit of unbitrotting, got all the rest in, and now this:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/405813492c73
Whiteboard: [baking on trunk]
(In reply to comment #30)
> Marco: I bet Shawn's gonna teach you how to make an hg bundle sometime real
> soon now ;)

heh yes, btw i did not have enough time to do more than a list :\
Thanks!

Comment 33

10 years ago
What’s the use of the Selected folder access key when this has been replaced by its folder name? Currently [Folder name (key)] shows up for any folder name not containing the key.
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090415 Shiretoko/3.5b4pre
Status: RESOLVED → VERIFIED

Comment 35

10 years ago
Sorry for bringing this up again, but as there hasn’t been feedback on comment 33, I really urge to act on this prior to the 3.5 release in order to prevent a new bug report about this issue.

Again, from my point of view, there’s no need to use the access key for the selected folder name, as it’s clearly unpredictable if the folder name contains the chosen key (r) at all. Could anyone please delete the call to search.scopeFolder.accesskey, or empty its content for all locales?
Assignee

Comment 36

10 years ago
(In reply to comment #35)
From what I understand it's an accessibility issue for people who don't or can't use mice.  In general all elements in the UI that support accesskeys should have them.  I'm no expert though.

Maybe we could dynamically pick the best accesskey based on the letters in the folder name.  That wouldn't work 100% of the time of course, and a changing accesskey probably wouldn't be so accessible in the first place.  If you have some ideas or would like to follow up about it, feel free to file a new bug on it.
Is it possible for the user to tab to and select the item, without using an access key?  If so, than I think the interface is ok.  For instance, individual tabs in the tabstrip don't have access keys.
Yeah, tab navigation is preferred over access keys; can someone check?
Assignee

Comment 39

10 years ago
(In reply to comment #37)
> Is it possible for the user to tab to and select the item, without using an
> access key?

The search scope buttons are toolbarbuttons, which are unfocusable by default.  I tried adding style="-moz-user-focus: normal;" to them and was able to tab to them, but there was no visual feedback that they were focused (on Pinstripe at least).
(In reply to comment #39)
> The search scope buttons are toolbarbuttons, which are unfocusable by default. 
> I tried adding style="-moz-user-focus: normal;" to them and was able to tab to
> them, but there was no visual feedback that they were focused (on Pinstripe at
> least).
We could fix that with CSS though, right?
Winstripe and Gnomestripe give visual feedback. It looks like Pinstripe should have something equivalent in toolbarbutton.css.
You want to use class="tabbable" instead of the style attribute, btw.
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.