The default bug view has changed. See this FAQ.

"Use Bookmark" button needs three dots after the text

RESOLVED FIXED in Firefox 9

Status

()

Firefox
Preferences
--
trivial
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Bamm Gabriana, Assigned: Javi Rueda)

Tracking

2.0 Branch
Firefox 9
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

6 years ago
Reproduced on Firefox 5 Win 7 x86
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
Created attachment 549623 [details] [diff] [review]
Patch for bug 378577

Same patch for Firefox 2 (mozilla-1.8 branch). Possible to land there too?
Attachment #549623 - Flags: review?(dietrich)
(Assignee)

Comment 4

6 years ago
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.
Attachment #549623 - Attachment is obsolete: true
Attachment #550669 - Flags: review?(dietrich)
Attachment #549623 - Flags: review?(dietrich)
(Assignee)

Updated

6 years ago
Attachment #550669 - Flags: review?(dietrich)
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.
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.
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
Created attachment 551002 [details] [diff] [review]
Patch for bug 378577 (ellipsis and id name change)
Attachment #550669 - Attachment is obsolete: true
Attachment #551002 - Flags: review?(dietrich)
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!
Attachment #551002 - Flags: review?(dietrich) → review-
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
Created attachment 558780 [details] [diff] [review]
Patch for bug 378577 (ellipsis and id name change) v1.1
Attachment #551002 - Attachment is obsolete: true
Attachment #558780 - Flags: review?(dietrich)
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 :)
Attachment #558780 - Flags: review?(dietrich) → review+
(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.
Attachment #558780 - Flags: feedback-
I defer to Gavin's judgement here. Sorry for steering you the wrong way Javi!
(Assignee)

Updated

6 years ago
Attachment #551002 - Attachment is obsolete: false
Attachment #551002 - Flags: review- → review?(dietrich)
(Assignee)

Updated

6 years ago
Attachment #558780 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #551002 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 16

6 years ago
(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 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.
Attachment #551002 - Flags: review?(dietrich)
Attachment #551002 - Flags: review+
Attachment #551002 - Flags: feedback?(gavin.sharp)
(Assignee)

Comment 18

6 years ago
(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.
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
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.
Attachment #551002 - Attachment is obsolete: true
Attachment #559313 - Flags: review?(gavin.sharp)
Comment on attachment 559313 [details] [diff] [review]
Patch for bug 378577 1.2.1

yes, precisely - thank you!
Attachment #559313 - Flags: review?(gavin.sharp) → review+
Assignee: nobody → leofigueres
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/466991fdc0f7
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/466991fdc0f7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
You need to log in before you can comment on or make changes to this bug.