Last Comment Bug 680812 - Excessive space between "280" and "KB" in download dialog
: Excessive space between "280" and "KB" in download dialog
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: File Handling (show other bugs)
: Trunk
: x86 Windows XP
: -- trivial (vote)
: Firefox 9
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 595888
  Show dependency treegraph
 
Reported: 2011-08-21 16:52 PDT by Jesse Ruderman
Modified: 2011-12-16 15:02 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch against changeset:75619:482742e6fff7 (1.37 KB, patch)
2011-08-21 22:39 PDT, Kailas
no flags Details | Diff | Review
Screenshot of patch (39.48 KB, image/png)
2011-08-21 22:39 PDT, Kailas
no flags Details
Patch against changeset:75619:482742e6fff7 (1.36 KB, text/plain)
2011-08-23 02:42 PDT, Kailas
no flags Details
Screenshot for Patch 2 (41.24 KB, image/png)
2011-08-23 02:44 PDT, Kailas
no flags Details
Patch against changeset:75619:482742e6fff7 (1.45 KB, patch)
2011-09-06 21:34 PDT, Kailas
dolske: review-
Details | Diff | Review
Patch for Aurora (1.37 KB, patch)
2011-09-07 21:35 PDT, Kailas
dolske: review+
Details | Diff | Review
Patch on changset: 75619:482742e6fff7 (2.47 KB, patch)
2011-09-11 22:39 PDT, Kailas
dolske: review+
Details | Diff | Review

Comment 1 Kailas 2011-08-21 22:39:00 PDT
Created attachment 554794 [details] [diff] [review]
Patch against changeset:75619:482742e6fff7

Patch to remove excessive space between file size and unit.
Patch on changeset:75619:482742e6fff7
Comment 2 Kailas 2011-08-21 22:39:59 PDT
Created attachment 554796 [details]
Screenshot of patch

Screenshot
Comment 3 Justin Dolske [:Dolske] 2011-08-22 13:03:12 PDT
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 2011-08-23 02:26:06 PDT
There is a space between 280 and KB and its coming from fileSizeWithType in unknownContentType.properties.
Comment 5 Kailas 2011-08-23 02:42:47 PDT
Created attachment 555055 [details]
Patch against changeset:75619:482742e6fff7

Patch against changeset:75619:482742e6fff7
Comment 6 Kailas 2011-08-23 02:44:10 PDT
Created attachment 555056 [details]
Screenshot for Patch 2

Screenshot of Patch
Comment 7 Justin Dolske [:Dolske] 2011-08-23 17:13:17 PDT
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] 2011-08-23 17:21:40 PDT
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 2011-08-23 18:02:18 PDT
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] 2011-08-23 18:54:00 PDT
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 2011-08-23 23:17:49 PDT
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 2011-08-23 23:20:14 PDT
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 2011-08-24 19:14:09 PDT
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 2011-09-06 21:34:47 PDT
Created attachment 558727 [details] [diff] [review]
Patch against changeset:75619:482742e6fff7

patch with localization note to remove an extra space.
Comment 15 Justin Dolske [:Dolske] 2011-09-07 18:15:14 PDT
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] 2011-09-07 18:17:58 PDT
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 2011-09-07 21:35:14 PDT
Created attachment 559042 [details] [diff] [review]
Patch for Aurora

Patch according to comment 15
Comment 18 Axel Hecht [:Pike] 2011-09-08 04:21:24 PDT
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] 2011-09-11 21:31:48 PDT
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 2011-09-11 22:39:47 PDT
Created attachment 559819 [details] [diff] [review]
Patch on changset: 75619:482742e6fff7

Modification done according to comment 18.
Comment 21 Justin Dolske [:Dolske] 2011-09-12 00:05:25 PDT
Comment on attachment 559819 [details] [diff] [review]
Patch on changset: 75619:482742e6fff7

Thanks!
Comment 22 :Ehsan Akhgari (out sick) 2011-09-12 07:24:53 PDT
Landed on inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/b3eb728a2d54
Comment 23 Matt Brubeck (:mbrubeck) 2011-09-13 06:51:32 PDT
https://hg.mozilla.org/mozilla-central/rev/b3eb728a2d54
Comment 24 christian 2011-12-16 13:20:47 PST
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 christian 2011-12-16 15:02:21 PST
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.

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