583965 - Using a postfix string to signify an item is an update may not be l10n friendly

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Currently, for update items, the "Update" postfix string is a separate element, that doesn't allow l10n formatting. This may not be friendly to some locales that wouldn't normally use a postfix to signify this.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Something we would have to fix before string freeze?

Comment 2

[Axel Hecht [:Pike]]

I'd need a code-reference or a screenshot or something to say.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot

This is actually showing a disabled addon (probably the same problem), but just imagine "(disabled)" really says "Update".

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

To clarify: each one of those (name, version, postfix) has its position hardcoded. AFAIK, in the old extension manager, version was also hardcoded to come directly after name.

Comment 5

[Axel Hecht [:Pike]]

So, the code in question is really http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.xml#754, right?

The only way I'd know to fully localize would be to put all of the children of name-container into a localizable entity. Not really l10n friendly, either. Or, add a direction attribute to name-container? Not sure.

Another question, is stuff there accessible to hacks via intl.css?

Comment 6

[Dave Townsend [:mossop]]

I suspect the best option here is just to create a bunch of localized strings here, something like:

"%S %S (disabled)"
"%S %S (blocked)"

etc.

With the postfixes I know of right now that would mean 8 strings though.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #6)
> I suspect the best option here is just to create a bunch of localized strings
> here, something like:
> 
> "%S %S (disabled)"
> "%S %S (blocked)"
> 
> etc.
> 
> With the postfixes I know of right now that would mean 8 strings though.

Uh, that's a lot. There's also the issue of addons that don't have a version number - need some way of avoiding having a double space. So we'd need to do something like "%S (blocked)", with that token being replaced by either just the name, or another entity that holds both name and version (ie, "%S %S").

Thankfully, we're no longer styling the postfix differently from the rest of that line, so we can do this now.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #5)
> So, the code in question is really
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.xml#754,
> right?

Yes.
 
> Another question, is stuff there accessible to hacks via intl.css?

I'm not familiar with how that's typically used. We don't currently import intl.css directly, though maybe its imported via some other file. So maybe? Would like to keep that as a last resort though.

Comment 9

[Axel Hecht [:Pike]]

It's part of xul.css, so it's included. I just don't know if it propagates into xbl bindings. I would hope it works at the same level of userChrome.css, so you could use that to test.

Though, if we're going for an entry per case without markup, using CSS to hack stuff in will obviously not help. But if we can go without styling, just a line of text may be just fine.

Comment 10

[Dave Townsend [:mossop]]

Axel, can you just clarify whether this is something we really need to fix to make this properly localizable or will localizers be able to work around it?

Comment 11

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

Axel, need your decision to above to decide if this is a beta6+ blocker.

Comment 12

[Axel Hecht [:Pike]]

I'd love to get a bit of feedback from localizers, I don't know enough edgecases.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Note that if possible, I'd like to be able to crop the addon name (if its too long), but NOT crop the postfix. Cropping the postfix means we lose important information.

I want to have: Blair's Really Awesome Exten... (disabled)
Instead of: Blair's Really Awesome Extension (d...

(See bug 567652 comment 10.)

So unless its absolutely necessary, I want to avoid putting all that line in a localized string.

Comment 14

[Vlado Valastiak [:wladow]]

This is how I solved this in our localization:

Test Addon v0.1 Update doesn't work for us, but
Test Addon v0.1 (update) does.

If we already have
Test Addon v0.1 (disabled)
Test Addon v0.1 (blocked)
why it is
Test Addon v0.1 Update?

Comment 15

[Axel Hecht [:Pike]]

<ehsan>	I think we probably want to wrap things like "(disabled)" in an element which can be set to rtl explicitly
<ehsan>	for add-on names, descriptions, etc we want to look at the first character and see if it's an RTL character or not
<ehsan>	and display the strings in span elements with the correct direction based on that
<ehsan>	so, something like this should work:
<ehsan>	<div dir=rtl><span id=foo>Add-on Name</span> <span dir=rtl>(disabled)</span></div>
<ehsan>	where we set #foo's dir explicitly based on its textContent's first character
<ehsan>	we might need to stick an RLM before the opening parenthesis though
* Pike	copies and pastes that into the bug

That looks like we should stick to elements to separate the flag strings.

Also, I am too a bit confuzzled about the update postfix, and it's l10n note.

And, in particular if we go for things in braces and things without, the order in which they show up should probably match. Like, right now, the line could be 

Foo addon (disabled) Update

right? Looks confusing, though it may just very well be totally unrealistic to have that string.


Does that help?

Comment 16

[(no longer active)]

Mossop asked me to explain what I meant in comment 15.

The main problem here is that the "(disabled)" string might be in RTL and "Add-on Name" might be in LTR (or vice versa).  We know the directionality of the former (which is determined by the locale's direction), but we don't know that of the latter.

The direction of the add-on name (and its description and whatever other textual data we retrieve for it from AMO) can be determined by looking at the first character (well, this is somewhat simplistic, but it should be enough) and check to see if it's in this range using this formula (assuming c is a character code):

(((0x0590 <= (c)) && ((c)<= 0x05FF)) || (((c) >= 0xfb1d) && ((c) <= 0xfb4f))) || ((0x0600 <= (c)) && ((c)<= 0x06FF))

(This is basically IS_HEBREW_CHAR(c) || IS_ARABIC_CHAR(c) stolen from <http://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/util/nsBidiUtils.h#274>)

Comment 17

[Dave Townsend [:mossop]]

Ok I don't think there is an RTL problem here then. The actual XUL here is roughly this:

<hbox>
  <label>Add-on name</label>
  <label>Add-on version</label>
  <label>(disabled)</label>
</hbox>

The labels will display ltr or rtl depending on the locale and the hbox will flip direction too so this seems to work out unless I am missing something?

The only problem is that as Ehsan mentions the add-on name may be a different direction to the locale's. This isn't great but is already a problem in 3.6 so I wouldn't consider it a blocker for 4.0 (and really should be fixed by making XUL label's direction-aware).

Comment 18

[(no longer active)]

I think this evaluation is fair.  I tested Force RTL against an en-US build, and things seem fine...

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #14)
> This is how I solved this in our localization:
> 
> Test Addon v0.1 Update doesn't work for us, but
> Test Addon v0.1 (update) does.
> 
> If we already have
> Test Addon v0.1 (disabled)
> Test Addon v0.1 (blocked)
> why it is
> Test Addon v0.1 Update?

Spoke with Boriss about this. The "(disabled)" and "(blocked)" postfixes are really states for installed add-ons. The "Update" postfix describes the thing you are looking at: "Foo 1.0 Update" is the update to Foo 1.0. It should never appear along with the other postfixes since updates won't be disabled or blocked. That 

She said it would also be fine to do it as you have done, we could even change it for English if that makes more sense for all locales to match up.

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #19)
> She said it would also be fine to do it as you have done, we could even change
> it for English if that makes more sense for all locales to match up.

In fact unless we want to change the default locale in this way I think we can call this bug WFM. I've not yet heard of real problems here and keeping the postfixes allows us to do the wrapping properly as Blair mentions.

Comment 21

[Axel Hecht [:Pike]]

I tend to agree. Still sounds like there'd be follow-ups, do we have those on file and triaged for string freeze?

Comment 22

[Dave Townsend [:mossop]]

(In reply to comment #21)
> I tend to agree. Still sounds like there'd be follow-ups, do we have those on
> file and triaged for string freeze?

Which follow-ups are you referring to?

Comment 23

[Axel Hecht [:Pike]]

Whether it's (Update) or Update, and if Update for itself needs a different string markup. Because, if that'd be really different, it might.

I really have a hard time to discuss (Update) vs Update as I haven't seen it in real life yet. Can I haz screencats?

Comment 24

[Dave Townsend [:mossop]]

(In reply to comment #23)
> Whether it's (Update) or Update, and if Update for itself needs a different
> string markup. Because, if that'd be really different, it might.
> 
> I really have a hard time to discuss (Update) vs Update as I haven't seen it in
> real life yet. Can I haz screencats?

http://grab.by/6ikk

Comment 25

[Dave Townsend [:mossop]]

(In reply to comment #24)
> (In reply to comment #23)
> > Whether it's (Update) or Update, and if Update for itself needs a different
> > string markup. Because, if that'd be really different, it might.
> > 
> > I really have a hard time to discuss (Update) vs Update as I haven't seen it in
> > real life yet. Can I haz screencats?
> 
> http://grab.by/6ikk

(I should probably file a bug on that showing as incompatible, it isn't meant to I think)

Comment 26

[Axel Hecht [:Pike]]

We talked about this on irc a bit more, and want to make the localization note a it more clear on the update postfix that offers () or somesuch for languages for which "foo 1.x Update" doesn't linguistically resolve to an update to that version.

Comment 27

[Johnathan Nightingale [:johnath]]

Where does that put us in terms of b6? Is there string-freeze-sensitive change required here, or not?

Comment 28

[Dave Townsend [:mossop]]

(In reply to comment #27)
> Where does that put us in terms of b6? Is there string-freeze-sensitive change
> required here, or not?

I'm going to file the patch for the localisation note today and assuming Pike is happy with it it can also land today. I think we probably want localisers to see the message before they localise so it probably still wants to block b6.

Comment 29

[Johnathan Nightingale [:johnath]]

Thanks, Dave - sounds good.

Comment 30

[Dave Townsend [:mossop]]

Attachment: patch rev 1

How does this read?

Comment 31

[Axel Hecht [:Pike]]

Comment on attachment 473636 [details] [diff] [review]
patch rev 1

Looks good to me.

The intended version number for update.postfix is the one to update to, right? Then it might be sweet to use "<Addon name> <1.1> Update" to make that more obvious?

r=me either way.

Comment 32

[Dave Townsend [:mossop]]

Attachment: patch for checkin

Done, for checkin

Comment 33

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/bd1e6e73c594

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with an fa build of Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100916 Firefox/4.0b7pre

Dave, I assume we do not want to have any automated test?

Comment 35

[Dave Townsend [:mossop]]

No, no need