Last Comment Bug 378577 - "Use Bookmark" button needs three dots after the text
: "Use Bookmark" button needs three dots after the text
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: 2.0 Branch
: x86 Windows XP
: -- trivial (vote)
: Firefox 9
Assigned To: Javi Rueda
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-24 04:28 PDT by Bamm Gabriana
Modified: 2011-09-11 06:35 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for bug 378577 (1.12 KB, patch)
2011-07-30 21:29 PDT, Javi Rueda
no flags Details | Diff | Splinter Review
Patch for bug 378577 using UNICODE ellipsis (1.23 KB, patch)
2011-08-04 06:24 PDT, Javi Rueda
no flags Details | Diff | Splinter Review
Patch for bug 378577 (ellipsis and id name change) (2.51 KB, patch)
2011-08-05 04:36 PDT, Javi Rueda
gavin.sharp: review+
Details | Diff | Splinter Review
Patch for bug 378577 (ellipsis and id name change) v1.1 (2.49 KB, patch)
2011-09-07 05:55 PDT, Javi Rueda
dietrich: review+
gavin.sharp: feedback-
Details | Diff | Splinter Review
Patch for bug 378577 1.2.1 (2.55 KB, patch)
2011-09-08 15:55 PDT, Javi Rueda
gavin.sharp: review+
Details | Diff | Splinter Review

Description Bamm Gabriana 2007-04-24 04:28:00 PDT
In Options > Main:

The "Use Bookmark" button under Startup leaves some confusion as to which bookmark it uses. Putting an Ellipsis (i.e., three dots) in front of it will let the user know that clicking it actually opens a dialog box where you can select a bookmark of your choice for the Home Page.
Comment 1 Javi Rueda 2011-07-30 20:59:56 PDT
Reproduced on Firefox 5 Win 7 x86
Comment 2 Javi Rueda 2011-07-30 21:19:27 PDT
Found a fix. The patch is the same for Firefox 2 and Nightly, as it modifies the same line in the same file and same path.
Comment 3 Javi Rueda 2011-07-30 21:29:27 PDT
Created attachment 549623 [details] [diff] [review]
Patch for bug 378577

Same patch for Firefox 2 (mozilla-1.8 branch). Possible to land there too?
Comment 4 Javi Rueda 2011-08-04 06:24:45 PDT
Created attachment 550669 [details] [diff] [review]
Patch for bug 378577 using UNICODE ellipsis

I resend the original patch but using the … ellipsis Unicode character instead of three dots.
Comment 5 Dietrich Ayala (:dietrich) 2011-08-04 13:28:58 PDT
Thanks Javi!

* Can you make a single patch against mozilla-central that makes the change with the ellipsis character?

* We need to update the entity name, since nearly every localization needs to also be updated to add the ellipsis. Any suggestions? chooseBookmark.label, maybe?

* Re Firefox 2: There are no more changes on that branch.
Comment 6 Dietrich Ayala (:dietrich) 2011-08-04 13:30:58 PDT
Also, the instance the entity is used must be updated to use the new entity name: http://mxr.mozilla.org/mozilla-central/search?string=useBookmark.label.

For future reference, I made a document that has the guidelines for making string changes: https://developer.mozilla.org/en/Making_String_Changes.
Comment 7 Javi Rueda 2011-08-04 13:57:52 PDT
(In reply to comment #5)
> Thanks Javi!
> 
> * Can you make a single patch against mozilla-central that makes the change
> with the ellipsis character?

Sorry for the bugmail. First patch used the 3 dots. Then I realized that … should be used, but in my cloned repository I was testing the Mercurial Queues, so the following patch wasn't good as it doesn't contains the patch to the code in the mozilla-central repository. (note to myself: don't use mq in local repository).

Yeah, I am going to create a new one with the change of the ellipsis using the code from the repository.

> 
> * We need to update the entity name, since nearly every localization needs
> to also be updated to add the ellipsis. Any suggestions?
> chooseBookmark.label, maybe?

This entity name is good to me. I chose this bug because the change needed was obvious. However, I am beggining to develop in this project, so I am still learning how it works.
> 
> * Re Firefox 2: There are no more changes on that branch.

Oks.
Comment 8 Javi Rueda 2011-08-04 14:00:25 PDT
(In reply to comment #6)
> Also, the instance the entity is used must be updated to use the new entity
> name: http://mxr.mozilla.org/mozilla-central/search?string=useBookmark.label.
> 
> For future reference, I made a document that has the guidelines for making
> string changes: https://developer.mozilla.org/en/Making_String_Changes.

I will read this doc and will code the changes from both the bug itself and Comment 5. Next, I will submit a new patch with the corrected string and entity name change.
Comment 9 Javi Rueda 2011-08-05 04:36:03 PDT
Created attachment 551002 [details] [diff] [review]
Patch for bug 378577 (ellipsis and id name change)
Comment 10 Dietrich Ayala (:dietrich) 2011-08-10 20:29:21 PDT
Comment on attachment 551002 [details] [diff] [review]
Patch for bug 378577 (ellipsis and id name change)

Review of attachment 551002 [details] [diff] [review]:
-----------------------------------------------------------------

don't change the access key, since it's value didn't change. i think that'd just confuse localizers. however, everything else looks fine. with that change, this patch should be good to go!
Comment 11 Javi Rueda 2011-08-11 03:45:53 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #10)
> Comment on attachment 551002 [details] [diff] [review]
> Patch for bug 378577 (ellipsis and id name change)
> 
> Review of attachment 551002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> don't change the access key, since it's value didn't change. i think that'd
> just confuse localizers. however, everything else looks fine. with that
> change, this patch should be good to go!

I didn't change the access key but its name. Following the link cited in Comment 6, I updated every place where the new entity name appears. Maybe I am wrong, but, although the value doesn't change ("B"), not changing this entity name from useBookmark.accesskey to chooseBookmark.accesskey is not coherent.
Comment 12 Javi Rueda 2011-09-07 05:55:17 PDT
Created attachment 558780 [details] [diff] [review]
Patch for bug 378577 (ellipsis and id name change) v1.1
Comment 13 Dietrich Ayala (:dietrich) 2011-09-07 18:01:13 PDT
Comment on attachment 558780 [details] [diff] [review]
Patch for bug 378577 (ellipsis and id name change) v1.1

Review of attachment 558780 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. regarding the incoherency in the naming scheme: yeah, i agree it's not ideal. i'm weighing coherency of two strings that only us few developers will ever look at, vs the hundreds (thousands?) of people who are doing translations across 90+ locales.

thanks for the patch, and your patience to make this change :)
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-09-07 18:19:58 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #10)
> don't change the access key, since it's value didn't change. i think that'd
> just confuse localizers.

Actually, I think the opposite is true - there are tools that associate labels and accesskeys based on the string names (and common suffixes like ".label" and ".accesskey"), so keeping the two in sync would be best. Localizers are used to dealing with accesskey/label churn.
Comment 15 Dietrich Ayala (:dietrich) 2011-09-07 18:26:14 PDT
I defer to Gavin's judgement here. Sorry for steering you the wrong way Javi!
Comment 16 Javi Rueda 2011-09-08 07:09:48 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #15)
> I defer to Gavin's judgement here. Sorry for steering you the wrong way Javi!

No problem. :-)
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-09-08 15:11:31 PDT
Comment on attachment 551002 [details] [diff] [review]
Patch for bug 378577 (ellipsis and id name change)

Please omit the ID change - addons may have come to rely on it, and there's no reason it needs to match the entity name used for the label/accesskey.

r=me with that change omitted.
Comment 18 Javi Rueda 2011-09-08 15:22:30 PDT
(In reply to Gavin Sharp from comment #17)
> 
> Please omit the ID change - addons may have come to rely on it, and there's
> no reason it needs to match the entity name used for the label/accesskey.

So, is it needed a new patch (which will made last reviewed one obsolete) that adds the ellipsis to the string _only_? I should not take into count the guidelines that instructed the coder to change entity name, then. Just to be sure of this before posting a new one.
Comment 19 Javi Rueda 2011-09-08 15:30:57 PDT
(In reply to Javi Rueda from comment #18)
> (In reply to Gavin Sharp from comment #17)
> > 
> > Please omit the ID change - addons may have come to rely on it, and there's
> > no reason it needs to match the entity name used for the label/accesskey.
> 
> So, is it needed a new patch (which will made last reviewed one obsolete)
> that adds the ellipsis to the string _only_? I should not take into count
> the guidelines that instructed the coder to change entity name, then. Just
> to be sure of this before posting a new one.

Sorry. I am auto-responding to myself. I should keep the entity changes from last patch and omit changes in the XUL file.
Comment 20 Javi Rueda 2011-09-08 15:55:36 PDT
Created attachment 559313 [details] [diff] [review]
Patch for bug 378577 1.2.1

This patch changes the entity names, keeping in sync both string and access-key and modifies main.xul file as it should use new entity name. Also adds an unicode ellipsis char to the button text.

As my comment 19 wasn't good enough in describing the changes, I prefer to ask again for a review to Gavin. Sorry about that, because you give already a + to the old patch.
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-09-08 15:57:28 PDT
Comment on attachment 559313 [details] [diff] [review]
Patch for bug 378577 1.2.1

yes, precisely - thank you!
Comment 22 Phil Ringnalda (:philor, back in August) 2011-09-10 21:20:35 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/466991fdc0f7
Comment 23 Matt Brubeck (:mbrubeck) 2011-09-11 06:35:58 PDT
http://hg.mozilla.org/mozilla-central/rev/466991fdc0f7

Note You need to log in before you can comment on or make changes to this bug.