Last Comment Bug 390282 - xul labels should be cropped with unicode ellipsis (\u2026) instead of three dots
: xul labels should be cropped with unicode ellipsis (\u2026) instead of three ...
Status: RESOLVED FIXED
backed out from branch at 1.8.1.10
: fixed1.8.1.8, polish, verified1.8.1.10
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Dão Gottwald [:dao]
:
Mentors:
Depends on: 390333 403484
Blocks: 373623 389303
  Show dependency treegraph
 
Reported: 2007-07-31 04:19 PDT by Dão Gottwald [:dao]
Modified: 2008-07-31 03:24 PDT (History)
15 users (show)
dveditz: blocking1.8.1.10+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.57 KB, patch)
2007-07-31 04:19 PDT, Dão Gottwald [:dao]
bzbarsky: review+
Details | Diff | Splinter Review
patch with comment (2.60 KB, patch)
2007-07-31 10:49 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch for tree (3.81 KB, patch)
2007-08-02 12:20 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch for tree (3.35 KB, patch)
2007-08-02 12:40 PDT, Dão Gottwald [:dao]
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.8+
dbaron: approval1.9+
Details | Diff | Splinter Review
patch for label (2.09 KB, patch)
2007-08-02 12:51 PDT, Dão Gottwald [:dao]
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.8+
dbaron: approval1.9+
Details | Diff | Splinter Review
patch for tree, against 1.8 branch (3.17 KB, patch)
2007-08-29 18:08 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
ja-JP Screenshot (top: three dots, bottom: \u2026) (43.81 KB, image/gif)
2007-11-01 16:12 PDT, Pete Riley
no flags Details
branch backout (5.22 KB, patch)
2007-11-03 03:48 PDT, Dão Gottwald [:dao]
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.10+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2007-07-31 04:19:44 PDT
Created attachment 274606 [details] [diff] [review]
patch
Comment 1 Boris Zbarsky [:bz] 2007-07-31 09:27:23 PDT
This really needs review from someone who knows how common this character is in fonts, not from me.  If we're going to be doing substitution on it all the time, we don't want to do this.
Comment 2 Dão Gottwald [:dao] 2007-07-31 10:05:41 PDT
(In reply to comment #1)
> This really needs review from someone who knows how common this character is in
> fonts, not from me.

I don't know who that would be, but I'll try to find out. Can you review the code?

Note that the Apple HIG recommends "…": http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGText/chapter_13_section_3.html#//apple_ref/doc/uid/TP30000365-DontLinkElementID_218

And it's part of WGL4. "As of 2004, WGL4 characters are the only ones guaranteed to display correctly on recent versions of all major platforms. Some Unicode characters may be missing from default installations of Windows 9x (such as Armenian and Georgian), of Linux (such as Devanagari) or of Windows XP (such as Ethiopian and Runic), but WGL4 glyphs are found on all major platforms." (http://en.wikipedia.org/wiki/WGL-4)
Comment 3 Boris Zbarsky [:bz] 2007-07-31 10:18:30 PDT
Comment on attachment 274606 [details] [diff] [review]
patch

Code looks fine, esp. if you add a comment like "U+2026" to explain what the #define is.
Comment 4 Dão Gottwald [:dao] 2007-07-31 10:49:50 PDT
Created attachment 274647 [details] [diff] [review]
patch with comment

blank super-review request for the open font issue
Comment 5 Dão Gottwald [:dao] 2007-08-01 00:20:21 PDT
Comment on attachment 274647 [details] [diff] [review]
patch with comment

should be OK as per bug 390333
Comment 6 Neil Deakin 2007-08-02 11:51:37 PDT
You may want to do the same for the tree (nsTreeBodyFrame.cpp)
Comment 7 Dão Gottwald [:dao] 2007-08-02 12:20:36 PDT
Created attachment 274982 [details] [diff] [review]
patch for tree

thanks, Neil. Tree obviously needs to be patched as well.
Comment 8 Boris Zbarsky [:bz] 2007-08-02 12:24:03 PDT
Comment on attachment 274982 [details] [diff] [review]
patch for tree

Hmm.  Why do you need the nsAutoString?  Why not #define ELLIPSIS PRUnichar(0x2026) and use it directly?
Comment 9 Dão Gottwald [:dao] 2007-08-02 12:40:48 PDT
Created attachment 274985 [details] [diff] [review]
patch for tree

Okay, this is just a wild guess. I'm really unfamiliar with that stuff.
Comment 10 Boris Zbarsky [:bz] 2007-08-02 12:45:49 PDT
Comment on attachment 274985 [details] [diff] [review]
patch for tree

Looks good.  I wonder whether the other patch could also ditch the UCS4 stuff...
Comment 11 Dão Gottwald [:dao] 2007-08-02 12:48:14 PDT
Yep, just a few minutes.
Comment 12 Dão Gottwald [:dao] 2007-08-02 12:51:13 PDT
Created attachment 274986 [details] [diff] [review]
patch for label

note that #include "nsReadableUtils.h" is already right at the top of nsTextBoxFrame.cpp
Comment 13 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2007-08-02 23:24:31 PDT
Comment on attachment 274986 [details] [diff] [review]
patch for label

a19=dbaron
Comment 14 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2007-08-02 23:24:52 PDT
Comment on attachment 274985 [details] [diff] [review]
patch for tree

a19=dbaron
Comment 15 Frank Wein [:mcsmurf] 2007-08-06 02:51:50 PDT
Patches checked in:
Checking in nsTextBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsTextBoxFrame.cpp,v  <--  nsTextBoxFrame.cpp
new revision: 1.111; previous revision: 1.110
done
Checking in tree/src/nsTreeBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v  <--  nsTreeBodyFrame.cpp
new revision: 1.324; previous revision: 1.323
done
Comment 16 Dão Gottwald [:dao] 2007-08-06 02:53:25 PDT
Thanks, Frank.
Comment 17 neil@parkwaycc.co.uk 2007-08-07 05:43:17 PDT
I'm pretty sure that the old label code was worse, but I do wonder which new way of inserting the ellipsis into the middle is better.
Comment 18 Dão Gottwald [:dao] 2007-08-07 05:59:31 PDT
(In reply to comment #17)
> I'm pretty sure that the old label code was worse, but I do wonder which new
> way of inserting the ellipsis into the middle is better.

ellipsisString isn't really needed, is that what you mean? Or is the behavior actually flawed in some way?
Comment 19 Simon Paquet [:sipaq] 2007-08-10 04:44:54 PDT
Is there a chance to get this on the MOZILLA_1_8_BRANCH, too? This would definitely help us in Calendar land.
Comment 20 Dão Gottwald [:dao] 2007-08-10 04:53:44 PDT
(In reply to comment #19)
> Is there a chance to get this on the MOZILLA_1_8_BRANCH, too?

I don't think so. Most locales currently use "...", so that would be inconsistent.
But feel free to request approval.
Comment 21 Simon Paquet [:sipaq] 2007-08-11 05:18:12 PDT
Comment on attachment 274986 [details] [diff] [review]
patch for label

I'd like to nominate this for MOZILLA_1_8_BRANCH inclusion.

The Calendar Project would really like to have this functionality, as it would gain us a significant UI improvement for event boxes with long texts.

From what I can see the patch is low-risk, therefore a branch inclusion seems justifiable.
Comment 22 Simon Paquet [:sipaq] 2007-08-11 05:19:27 PDT
Comment on attachment 274985 [details] [diff] [review]
patch for tree

I'd like to nominate this for MOZILLA_1_8_BRANCH inclusion.

The Calendar Project would really like to have this functionality, as it would gain us a significant UI improvement for our daily agenda view (which gets much more prominent in our upcoming branch-based 0.7 version) with long texts.

From what I can see the patch is low-risk, therefore a branch inclusion seems justifiable.
Comment 23 Daniel Veditz [:dveditz] 2007-08-29 16:00:43 PDT
Comment on attachment 274985 [details] [diff] [review]
patch for tree

approved for 1.8.1.7, a=dveditz for release-drivers
Comment 24 Daniel Veditz [:dveditz] 2007-08-29 16:01:00 PDT
Comment on attachment 274986 [details] [diff] [review]
patch for label

approved for 1.8.1.7, a=dveditz for release-drivers
Comment 25 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-29 17:48:16 PDT
Checking in nsTextBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsTextBoxFrame.cpp,v  <--  nsTextBoxFrame.c
pp
new revision: 1.80.4.2; previous revision: 1.80.4.1
done

Checked in on the 1.8.1 branch.
Comment 26 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-29 18:06:27 PDT
I forgot about the patch for tree, but that one doesn't apply nicely for the 1.8.1 branch.
I'll attach a tree patch that is against the 1.8.1 branch.
I think it's all right. Should someone take a look at that one again?
Comment 27 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-29 18:08:32 PDT
Created attachment 278890 [details] [diff] [review]
patch for tree, against 1.8 branch
Comment 28 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-08-29 18:18:44 PDT
Checking in nsTreeBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v  <--  nsTree
BodyFrame.cpp
new revision: 1.240.10.13; previous revision: 1.240.10.12
done

Checked "patch for tree, against 1.8 branch" in on 1.8.1 brnach.

I talked to dao and boris on irc, and they said it's fine to land this, so I've landed this.

So this bug can be marked fixed, right?
Comment 29 Dão Gottwald [:dao] 2007-08-29 18:21:17 PDT
Yes and thanks!
Comment 30 neil@parkwaycc.co.uk 2007-09-22 13:56:13 PDT
Note that under the default Win9x/NT4 theme, or using the "Classic" theme on Windows 2000/XP, you can't embolden an ellipsis; you get an underscore instead.
Comment 31 Dão Gottwald [:dao] 2007-09-23 14:33:44 PDT
(In reply to comment #30)
> Note that under the default Win9x/NT4 theme, or using the "Classic" theme on
> Windows 2000/XP, you can't embolden an ellipsis; you get an underscore instead.

WFM on Win XP.
Comment 32 neil@parkwaycc.co.uk 2007-09-23 15:08:04 PDT
(In reply to comment #31)
>(In reply to comment #30)
>>Note that under the default Win9x/NT4 theme, or using the "Classic" theme on
>>Windows 2000/XP, you can't embolden an ellipsis; you get an underscore instead.
>WFM on Win XP.
Sorry, I meant "Classic" colour/font scheme, rather than theme (Windows 2000 doesn't support themes so "Classic" theme doesn't make sense there anyway).
Comment 33 Masayuki Nakano [:masayuki] (Mozilla Japan) 2007-11-01 06:09:49 PDT
This patch should be backed-out from 1.8 branch. See bug 400237. This patch breaks Japanese UI.
Comment 34 dynamis (Tomoya ASAI) 2007-11-01 06:29:36 PDT
(In reply to comment #22)
> The Calendar Project would really like to have this functionality, as it would
> gain us a significant UI improvement for our daily agenda view (which gets much
> more prominent in our upcoming branch-based 0.7 version) with long texts.

The reason this patch was included to mozilla1.8 branch is because ellipsis takes shorter space than three dots?
Japanese users feel funny if ellipsis is used (ellipsis and three dots looks different in Japanese). That's more important problem than taking a little space.
# and space needed for ellipsis/dots depends on user's fonts

If bug 400237 will be also included to 1.8 branch, it's OK. But otherwise this bug should be backed-out from 1.8 branch.
Comment 35 Boris Zbarsky [:bz] 2007-11-01 07:01:16 PDT
> The reason this patch was included to mozilla1.8 branch is because ellipsis
> takes shorter space than three dots?

No, because the ellipsis looks nicer than three dots...  at least for Western text.
Comment 36 Masayuki Nakano [:masayuki] (Mozilla Japan) 2007-11-01 07:14:56 PDT
(In reply to comment #35)
> > The reason this patch was included to mozilla1.8 branch is because ellipsis
> > takes shorter space than three dots?
> 
> No, because the ellipsis looks nicer than three dots...  at least for Western
> text.

"looks nicer" is not enough reason for changing on branch, I think...
Comment 37 Pete Riley 2007-11-01 12:01:57 PDT
Regarding the calendar project, it's not so much about "looks nice".  It's about removing two unnecessary characters from event titles so that people can see more of the important text.  For example, consider a monthly calendar which has 31 days in it.  When you try to fit 31 boxes on a screen, there is very little horizontal room for the event titles in each box.  If we can reduce the ellipsis from three characters down to one, then the user will be able to see two additional characters of each event title.  This makes a significant improvement in the calendar views.
Comment 38 Masayuki Nakano [:masayuki] (Mozilla Japan) 2007-11-01 14:03:21 PDT
(In reply to comment #37)
> If we can reduce the ellipsis from three characters down to one,
> then the user will be able to see two additional characters of each event title.

Is this true? The glyph of full stop is too narrower than many letters in many fonts. And note that the width of the Unicode ELLIPSIS of MS Gothic which is Japanese default font of Win32 is wider than three dots.
Comment 39 Pete Riley 2007-11-01 16:12:57 PDT
Created attachment 287025 [details]
ja-JP Screenshot (top: three dots, bottom: \u2026)

Well, it's true for fixed-width fonts (which I use).

For proportional fonts, you're right that the difference of width is less than two characters.  However, I think that in most proportional fonts, a single ellipsis character is still narrower than three dots.  I attached a smaller version of your screenshot from bug 400237.  It shows that the ellipsis character is narrower than the three dots with the MS Gothic font (not much, but it is narrower).

I completely agree with you that bug 400237 should be fixed.
Comment 40 Masayuki Nakano [:masayuki] (Mozilla Japan) 2007-11-01 23:51:22 PDT
(In reply to comment #39)
> I completely agree with you that bug 400237 should be fixed.

I'll fix bug 400237, but the patch cannot be applied to 1.8 branch, because it needs to change the l10n resource, it is not allowed on any branch. So, I have no idea for 1.8 branch without backing-out.
Comment 41 Boris Zbarsky [:bz] 2007-11-02 08:28:42 PDT
My gut instinct is that we should back this out on branch until such time as we sort out the issues with it.
Comment 42 Daniel Veditz [:dveditz] 2007-11-02 23:34:42 PDT
I suspect the whole concept embodied by this code is wrong-headed. Whether "..." or "…", expecting a single hardcoded representation (#define ELLIPSIS) to work for all scripts seems too much to hope for. If this were a localized resource western versions could use \u2026, Japanese could use three dots, and other translations could use whatever special glyph or sequence they prefer.
Comment 43 Dão Gottwald [:dao] 2007-11-03 03:46:35 PDT
(In reply to comment #42)
\u2026 is not a western glyph, it has i18n built-in. One problem is that applications have been using three dots since year one, so that Japanese users feel that the Japanese glyph is alien. The second problem is that updating tree and label without the locales makes the UI inconsistent, especially if \u2026 looks that different from three dots.
Comment 44 Dão Gottwald [:dao] 2007-11-03 03:48:07 PDT
Created attachment 287205 [details] [diff] [review]
branch backout
Comment 45 Daniel Veditz [:dveditz] 2007-11-07 14:47:46 PST
Comment on attachment 287205 [details] [diff] [review]
branch backout

approved for 1.8.1.10, a=dveditz for release-drivers
Comment 46 Masayuki Nakano [:masayuki] (Mozilla Japan) 2007-11-08 00:58:43 PST
checked-in the backing-out patch, thanks!
Comment 47 Al Billings [:abillings] 2007-11-15 16:50:02 PST
Can someone verify that this change made it into FF 2.0.0.10 (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.10-candidates/rc1/)?
Comment 48 Dão Gottwald [:dao] 2007-11-15 16:55:19 PST
verified using
Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.8.1.10) Gecko/20071115 Firefox/2.0.0.10

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