The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

({fixed1.8.1.8, polish, verified1.8.1.10})

Trunk
fixed1.8.1.8, polish, verified1.8.1.10
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.10 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: backed out from branch at 1.8.1.10)

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 274606 [details] [diff] [review]
patch
Attachment #274606 - Flags: review?(bzbarsky)
(Assignee)

Updated

10 years ago
Blocks: 389303
(Assignee)

Updated

10 years ago
Summary: xul labels should be cropped with unicode ellipses (\u2026) instead of three dots → xul labels should be cropped with unicode ellipsis (\u2026) instead of three dots
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.
(Assignee)

Comment 2

10 years ago
(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 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.
Attachment #274606 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

10 years ago
Created attachment 274647 [details] [diff] [review]
patch with comment

blank super-review request for the open font issue
Attachment #274647 - Flags: superreview?
(Assignee)

Updated

10 years ago
Depends on: 390333
(Assignee)

Comment 5

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

should be OK as per bug 390333
Attachment #274647 - Flags: superreview?
(Assignee)

Updated

10 years ago
Attachment #274606 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Updated

10 years ago
Attachment #274647 - Flags: approval1.9?

Comment 6

10 years ago
You may want to do the same for the tree (nsTreeBodyFrame.cpp)
(Assignee)

Comment 7

10 years ago
Created attachment 274982 [details] [diff] [review]
patch for tree

thanks, Neil. Tree obviously needs to be patched as well.
Attachment #274982 - Flags: review?(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?
(Assignee)

Comment 9

10 years ago
Created attachment 274985 [details] [diff] [review]
patch for tree

Okay, this is just a wild guess. I'm really unfamiliar with that stuff.
Attachment #274982 - Attachment is obsolete: true
Attachment #274985 - Flags: review?(bzbarsky)
Attachment #274982 - Flags: review?(bzbarsky)
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Comment on attachment 274985 [details] [diff] [review]
patch for tree

Looks good.  I wonder whether the other patch could also ditch the UCS4 stuff...
Attachment #274985 - Flags: superreview+
Attachment #274985 - Flags: review?(bzbarsky)
Attachment #274985 - Flags: review+
(Assignee)

Comment 11

10 years ago
Yep, just a few minutes.
(Assignee)

Comment 12

10 years ago
Created attachment 274986 [details] [diff] [review]
patch for label

note that #include "nsReadableUtils.h" is already right at the top of nsTextBoxFrame.cpp
Attachment #274647 - Attachment is obsolete: true
Attachment #274986 - Flags: review?(bzbarsky)
Attachment #274647 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Attachment #274985 - Flags: approval1.9?
Attachment #274986 - Flags: superreview+
Attachment #274986 - Flags: review?(bzbarsky)
Attachment #274986 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #274986 - Flags: approval1.9?
Comment on attachment 274986 [details] [diff] [review]
patch for label

a19=dbaron
Attachment #274986 - Flags: approval1.9? → approval1.9+
Comment on attachment 274985 [details] [diff] [review]
patch for tree

a19=dbaron
Attachment #274985 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
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
(Assignee)

Comment 16

10 years ago
Thanks, Frank.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 17

10 years ago
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.
(Assignee)

Comment 18

10 years ago
(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?
Is there a chance to get this on the MOZILLA_1_8_BRANCH, too? This would definitely help us in Calendar land.
(Assignee)

Comment 20

10 years ago
(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 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.
Attachment #274986 - Flags: approval1.8.1.7?
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.
Attachment #274985 - Flags: approval1.8.1.7?
Comment on attachment 274985 [details] [diff] [review]
patch for tree

approved for 1.8.1.7, a=dveditz for release-drivers
Attachment #274985 - Flags: approval1.8.1.7? → approval1.8.1.7+
Comment on attachment 274986 [details] [diff] [review]
patch for label

approved for 1.8.1.7, a=dveditz for release-drivers
Attachment #274986 - Flags: approval1.8.1.7? → approval1.8.1.7+
(Assignee)

Updated

10 years ago
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
(Assignee)

Updated

10 years ago
Whiteboard: [checkin needed (1.8 branch)]
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.
Keywords: checkin-needed → fixed1.8.1.7
Whiteboard: [checkin needed (1.8 branch)]
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?
Keywords: fixed1.8.1.7
Created attachment 278890 [details] [diff] [review]
patch for tree, against 1.8 branch
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?
Keywords: fixed1.8.1.7
(Assignee)

Comment 29

10 years ago
Yes and thanks!
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 30

10 years ago
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.
(Assignee)

Comment 31

10 years ago
(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

10 years ago
(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).
This patch should be backed-out from 1.8 branch. See bug 400237. This patch breaks Japanese UI.
Flags: blocking1.8.1.9?

Updated

10 years ago
Flags: blocking1.8.1.10?
(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.
> 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.
(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

10 years ago
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.
(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

10 years ago
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.
(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.
(Assignee)

Updated

10 years ago
Flags: blocking1.8.1.9?
My gut instinct is that we should back this out on branch until such time as we sort out the issues with it.
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.
(Assignee)

Comment 43

10 years ago
(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.
(Assignee)

Comment 44

10 years ago
Created attachment 287205 [details] [diff] [review]
branch backout
Attachment #287205 - Flags: superreview?(bzbarsky)
Attachment #287205 - Flags: review?(bzbarsky)
(Assignee)

Updated

10 years ago
Attachment #287025 - Attachment description: Screenshot → ja-JP Screenshot (top: three dots, bottom: \u2026)
Attachment #287205 - Flags: superreview?(bzbarsky)
Attachment #287205 - Flags: superreview+
Attachment #287205 - Flags: review?(bzbarsky)
Attachment #287205 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #287205 - Flags: approval1.8.1.10?
Flags: blocking1.8.1.11? → blocking1.8.1.10+
Comment on attachment 287205 [details] [diff] [review]
branch backout

approved for 1.8.1.10, a=dveditz for release-drivers
Attachment #287205 - Flags: approval1.8.1.10? → approval1.8.1.10+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
checked-in the backing-out patch, thanks!
Keywords: checkin-needed, fixed1.8.1.8
Whiteboard: checked-in to 1.8.1.8, but backed out from branch at 1.8.1.10
(Assignee)

Updated

10 years ago
Keywords: fixed1.8.1.8
Whiteboard: checked-in to 1.8.1.8, but backed out from branch at 1.8.1.10 → backed out from branch at 1.8.1.10
Keywords: fixed1.8.1.10
(Assignee)

Updated

10 years ago
Depends on: 403484
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/)?
(Assignee)

Comment 48

10 years ago
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
Keywords: fixed1.8.1.10 → verified1.8.1.10

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.