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

Status: VERIFIED FIXED

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

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Current context bar in the library window

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.

Comment 1

[Marco Bonardo [:mak]]

(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

Comment 2

[Drew Willcoxon :adw]

Attachment: v1

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

Comment 3

[Drew Willcoxon :adw]

Oh, and because it no longer make sense to disable the folder button when folder scope is inappropriate, it's now hidden instead.

Comment 4

[Marco Bonardo [:mak]]

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

Comment 5

[Drew Willcoxon :adw]

Attachment: v2

- 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

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Axel Hecht [:Pike]]

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.

Comment 8

[Drew Willcoxon :adw]

Attachment: v3

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.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 366650 [details] [diff] [review]
v3

new version coming i think

Comment 10

[Drew Willcoxon :adw]

Attachment: v4

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.

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Drew Willcoxon :adw]

Attachment: v5

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

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 367247 [details] [diff] [review]
v5

Axel, are accesskey changes fine for you?

Comment 14

[Dão Gottwald [:dao]]

Did you mean to request ui-review? I don't quite understand what Axel is expected to review there (but maybe he does).

Comment 15

[Marco Bonardo [:mak]]

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 16

[Axel Hecht [:Pike]]

Comment on attachment 367247 [details] [diff] [review]
v5

'l' is a bad accesskey, and IIRC, 'o' ain't better.

Comment 17

[Drew Willcoxon :adw]

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

Comment 18

[Axel Hecht [:Pike]]

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.

Comment 19

[Dietrich Ayala (:dietrich)]

(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." :)

Comment 20

[Axel Hecht [:Pike]]

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.

Comment 21

[Drew Willcoxon :adw]

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.

Comment 22

[Drew Willcoxon :adw]

Attachment: v6

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.

Comment 23

[Axel Hecht [:Pike]]

Comment on attachment 367816 [details] [diff] [review]
v6

The l10n changes look good to me.

Comment 24

[Marco Bonardo [:mak]]

Attachment: v7

a small update since search.accesskey was still i nuse in the sidebar

Comment 25

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ce5169d70fbb

Comment 26

[Marco Bonardo [:mak]]

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.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 367851 [details] [diff] [review]
v7

a191=beltzner

Comment 28

[Shawn Wilsher :sdwilsh]

This patch does not apply cleanly to mozilla-1.9.1

Comment 29

[Marco Bonardo [:mak]]

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

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

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 ;)

Comment 31

[Shawn Wilsher :sdwilsh]

With a bit of unbitrotting, got all the rest in, and now this:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/405813492c73

Comment 32

[Marco Bonardo [:mak]]

(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

[Ton [:Tonnes]]

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.

Comment 34

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090415 Shiretoko/3.5b4pre

Comment 35

[Ton [:Tonnes]]

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?

Comment 36

[Drew Willcoxon :adw]

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

Comment 37

[Alex Faaborg [:faaborg] (Firefox UX)]

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.

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

Yeah, tab navigation is preferred over access keys; can someone check?

Comment 39

[Drew Willcoxon :adw]

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

Comment 40

[Shawn Wilsher :sdwilsh]

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

Comment 41

[Dão Gottwald [:dao]]

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.

Comment 42

[Gervase Markham [:gerv]]

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