390282 - xul labels should be cropped with unicode ellipsis (\u2026) instead of three dots

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[Boris Zbarsky [:bzbarsky]]

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]]

(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 [:bzbarsky]]

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]]

Attachment: patch with comment

blank super-review request for the open font issue

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 274647 [details] [diff] [review]
patch with comment

should be OK as per bug 390333

Comment 6

[Neil Deakin]

You may want to do the same for the tree (nsTreeBodyFrame.cpp)

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch for tree

thanks, Neil. Tree obviously needs to be patched as well.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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]]

Attachment: patch for tree

Okay, this is just a wild guess. I'm really unfamiliar with that stuff.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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]]

Yep, just a few minutes.

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch for label

note that #include "nsReadableUtils.h" is already right at the top of nsTextBoxFrame.cpp

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 274986 [details] [diff] [review]
patch for label

a19=dbaron

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 274985 [details] [diff] [review]
patch for tree

a19=dbaron

Comment 15

[Frank Wein [:mcsmurf]]

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]]

Thanks, Frank.

Comment 17

[[e-mail]]

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]]

(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]]

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]]

(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]]

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]]

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]]

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]]

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 (dead)]

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 (dead)]

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 (dead)]

Attachment: patch for tree, against 1.8 branch

Comment 28

[Martijn Wargers (dead)]

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]]

Yes and thanks!

Comment 30

[[e-mail]]

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]]

(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

[[e-mail]]

(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] (he/him)(JST, +0900)]

This patch should be backed-out from 1.8 branch. See bug 400237. This patch breaks Japanese UI.

Comment 34

[dynamis (Tomoya ASAI)]

(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 [:bzbarsky]]

> 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] (he/him)(JST, +0900)]

(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]

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] (he/him)(JST, +0900)]

(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]

Attachment: 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] (he/him)(JST, +0900)]

(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 [:bzbarsky]]

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]]

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]]

(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]]

Attachment: branch backout

Comment 45

[Daniel Veditz [:dveditz]]

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] (he/him)(JST, +0900)]

checked-in the backing-out patch, thanks!

Comment 47

[Al Billings [:abillings - ex-MoCo]]

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]]

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