680812 - Excessive space between "280" and "KB" in download dialog

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Jesse Ruderman]

https://bugzilla.mozilla.org/attachment.cgi?id=553174

Comment 1

[Kailas]

Attachment: Patch against changeset:75619:482742e6fff7

Patch to remove excessive space between file size and unit.
Patch on changeset:75619:482742e6fff7

Comment 2

[Kailas]

Attachment: Screenshot of patch

Screenshot

Comment 3

[Justin Dolske [:Dolske]]

Huh. Why does it look like there's still a space between "280" and "KB"? The visual effect is correct (as, in ui-r+ for that screenshot), but from looking at the various .properties and APIs involved, I'd expect there to be no space.

Could you do a little debugging to see if there is actually still a space in there, and where's in coming from? We may want to kill _that_ space, and leave .properties file alone. (because I suspect you'll need to change the string name, so that localizers know to update their version to no include the space.)

Comment 4

[Kailas]

There is a space between 280 and KB and its coming from fileSizeWithType in unknownContentType.properties.

Comment 5

[Kailas]

Attachment: Patch against changeset:75619:482742e6fff7

Patch against changeset:75619:482742e6fff7

Comment 6

[Kailas]

Attachment: Screenshot for Patch 2

Screenshot of Patch

Comment 7

[Justin Dolske [:Dolske]]

Now I'm even more confused. What's the difference between the first two patches in this bug, and why are the screenshots different?

The original screenshot (attachment 553174 [details]) shows current Firefox with a whole lot of space between the number and unit.

The first patch removes the 1 space in the .properties file. The screenshot (attachment 554796 [details]) looks like the correct fix to me (space between number and unit, but not too much... "just right"). But I was asking in comment 3 where the just-right amount of space is coming from.

The second patch looks almost identical to the first patch (removes the 1 space), but now it's screenshot (attachment 555056 [details]) shows the number and unit touching each other (no space at all, looks too tight).

Comment 8

[Justin Dolske [:Dolske]]

I just double-checked with DOM Inspector on a current trunk OS X nightly. The string in the dialog is "Preview Document (3.2  MB)" -- there are definitely 2 spaces between the number and unit.

The string in unknownContentType.properties accounts for 1 of the spaces, where's the other coming from?

Comment 9

[Kailas]

The string in unknowContentType.properties was (%2S %3S). One space was coming from the space between %2s and %3S and another was from the number "3". 
In attachment (attachment 554796 [details]) I removed the space between %2S and %3S. So there was only 1 space left between "280" and "KB". 
After removing the number "3" from "%3S" that 1 space is also deleted and there is no space at all in "280" and "KB". 
I also feel second patch looks very tight. First patch (attachment 554794 [details] [diff] [review]) seems correct fix to me.

Comment 10

[Justin Dolske [:Dolske]]

Hmm, there shouldn't be any difference between "%1S %2S" and %S %S", the number are just to allow disambiguation for locales which need to change the order thof things (eg "%2S %1S").

Comment 11

[Kailas]

Yeah, number makes a difference and culprit is "fill2" function in nsTextFormatter.cpp file (line 142: rv = (*ss->stuff)(ss, &space, 1); ). 

Main cause is:
variable 'width' passed to "fill2" as a parameter is calculated as
"width = (width * 10) + (c - '0'); " 
Therefore when 'c' is 3, value of 'width' become 3 and data value of %3S is "KB". That is just 2 chars in length. Therefore, one extra space is inserted. 
The number of spaces to insert are calculated using expression
"width -= srclen;"

So if data value of %3S is less than the number 3 then the number of spaces inserted are equal to the difference between the (number - data_value_lenth).
In our case the difference is 1 therefore one space is inserted. However, if we don't use number then there will be no extra space insertion occurs.

Comment 12

[Kailas]

I suspect if there is any %9S and data value of it is just 2 chars in a length then number of spaces inserted will be 7.

Comment 13

[Kailas]

Dolske: I don't know why that Right adjusting done in "fill2" is required?. Are we still going with patch 1 for this bug?

Comment 14

[Kailas]

Attachment: Patch against changeset:75619:482742e6fff7

patch with localization note to remove an extra space.

Comment 15

[Justin Dolske [:Dolske]]

Holy schmoley. I also traced the code flow, and was a little surprised to see we treat these as printf-style formatters! 

But that's actually the problem. The correct (and normal) way to specify this stuff is like this:

  label=My %1$S is full of %2$S

not

  label=My %1S is full of %2S

No only does the latter lead to formatting glitches like this, but it also doesn't work for localization... "My %2S is full of %1S" won't print the arguments in reverse order.

Thus, the correct fix here would be:

-fileSizeWithType=%1S (%2S %3S)
+fileSizeWithType=%1$S (%2$S %3$S)

(This string has been broken since bug 595888 first added it).

I did a quick MXR regex search for "%[0-9][^\$]", looks like there are a couple other questionable uses like this in other property files.

Pike, should be rev the entity name here to make sure localizers didn't just cut'n'paste the wrong string? I guess so, a quick l10n MXR shows all (17) locales just copied this as-is. More terrifying is that the regex search finds a number of busted-looking strings, though some look unused. I'll file a new bug for that in a moment.

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 558727 [details] [diff] [review]
Patch against changeset:75619:482742e6fff7

r- per previous comment. But HUGE KUDOS for helping to discover the root cause of what was happening here!

I look forward to the patch I suggested so we can close out this seemingly-trivial issue! :-)

Comment 17

[Kailas]

Attachment: Patch for Aurora

Patch according to comment 15

Comment 18

[Axel Hecht [:Pike]]

For central, we should do the key change, I suggest something like orderedFileSizeWithType.

Sadly, this is already on aurora. Aurora is string frozen, so we can't take that patch on aurora. As the change is cosmetic in nature, I'd be fine with landing the value-only change on aurora and post to .l10n to invite people to fix that in their locales, too. Nothing deep, we'll catch the real fix in the next cycle then.

Comment 19

[Justin Dolske [:Dolske]]

Comment on attachment 559042 [details] [diff] [review]
Patch for Aurora

r+ing this as the version suitable to land on Aurora. For mozilla-central we should change the label from "fileSizeWithType" to something else, as Pike suggested in the last comment.

So sorry this has dragged out, but at least we're in the final stretch and made an interesting discovery along the way! Can you do the patch for mozilla-central too as described above?

Comment 20

[Kailas]

Attachment: Patch on changset: 75619:482742e6fff7

Modification done according to comment 18.

Comment 21

[Justin Dolske [:Dolske]]

Comment on attachment 559819 [details] [diff] [review]
Patch on changset: 75619:482742e6fff7

Thanks!

Comment 22

[(no longer active)]

Landed on inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/b3eb728a2d54

Comment 23

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/b3eb728a2d54

Comment 24

[LegNeato]

This has been approved for aurora but hasn't landed. Please land there ASAP if you would like to get this in Fx10.

Comment 25

[LegNeato]

Comment on attachment 559042 [details] [diff] [review]
Patch for Aurora

Actually, jared pointed out that this made it into central when Fx9 was there, so this is already in the repo. I'm clearing the aurora approval.