249141 - Incorrect header cropping on narrow tree columns (ellipsis in wrong column)

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Karsten Düsterloh]

When narrowing tree columns, the header text gets cropped eventually and
substituted by an ellipsis. But before reaching the minimal width, the ellipsis
"jumps" to the left into another column... (see attached screenshot)

Example: "Total" column in the MailNews folder pane tree.

Found in: Mozilla 1.3 on Win2k, Mozilla 1.8a2/20040627 on Win2k, Thunderbird 0.7
on Win2k, Mozilla 1.8a2/20040629 on Linux Debian Woody. Theme does not matter
(tested Classic, Modern, Noia).

Comment 1

[Karsten Düsterloh]

Attachment: Incorrect cropping

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

WFM Thunderbird version 0.7+ (20040630)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mmh I can see it for some header columns and not all. Affected columns are
Unread and Total.

Comment 4

[Karsten Düsterloh]

It seems as if only the folder and thread pane trees in MailNews and Thunderbird
are affected, and only the columns Unread, Total and Size...

Comment 6

[Pardal Freudenthal (:ShareBird)]

I can confirm this on both, Thunderbird 2.0 and 3.0.

The problem here is that those columns #folderUnreadCol, #folderTotalCol, #folderSizeCol on folder pane and #sizeCol, #unreadCol, #totalCol on message window have text-align: right but the property "crop" has a value "right" (they should be "left").

Comment 7

[Pardal Freudenthal (:ShareBird)]

Attachment: proposed patch [Backout: Comment 13]

This patch adds the property crop="left" for those columns on messenger.xul. I'm not sure if this columns follow rtl, ltr directions...

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 354908 [details] [diff] [review]
proposed patch
[Backout: Comment 13]

looks good

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 354908 [details] [diff] [review]
proposed patch
[Backout: Comment 13]

This should be crop="start" instead. (treecol should handle rtl vs ltr), and the part for the folder pane is now obsolete - as we don't have those columns for tb3.

I'll make those changes and check this in for you

Comment 10

[Magnus Melin [:mkmelin]]

changeset:   1528:ba1e5bafe929
http://hg.mozilla.org/comm-central/rev/ba1e5bafe929

New bug for seamonkey, please.

Comment 11

[Karsten Düsterloh]

> New bug for seamonkey, please.

Nonsens, why should I refile my own bug?

Comment 12

[Karsten Düsterloh]

I don't think the patch is right.

Changing the crop value changes the way the columns get cropped when they become too small. While it is debatable on which side you should crop a number, cropping eg. "Total in Thread" to "...Thread" is not desirable.

Basically, I do think the bug is in the tree column cropping code - cropping and positioning of the ellipsis is done right until all of the text is gone. Only then the ellipsis "jumps" to the wrong position...

Comment 13

[Magnus Melin [:mkmelin]]

I backed this out due to the unwanted side effects Karsten describes.
changeset:   1529:a554aabedc5d
http://hg.mozilla.org/comm-central/rev/a554aabedc5d

Comment 14

[Pardal Freudenthal (:ShareBird)]

I don't see a problem here. Ellipsis indicates that a part of a text lacks, and where the ellipsis is positioned shows in which direction it lacks. For example a quote like "...the columns get cropped when they become too small" indicates that there is more text before the citation and "Changing the crop value changes the way..." indicates more text after it. Also the visual effect when I make the place for the text smaller - the ellipsis shows the direction I'm really cropping the text.

Comment 15

[[e-mail]]

Attachment: Proposed patch [Checkin: Comment 23 & 27]

The problem is that mTitleWidth wasn't being set in the case of an early return. I had the choice of two approaches. One of the other cases used a break rather than a return, however this means that it ends up recomputing the ellipse width in the case that it can't get any text at all to fit, so I decided to compute the ellipse width directly into mTitleWidth to allow for both early returns.
I also decided to set mTitleWidth to zero when even the ellipse does not fit inside the width, as that seems to match the behaviour in other cases.

Comment 16

[Neil Deakin]

Comment on attachment 355054 [details] [diff] [review]
Proposed patch
[Checkin: Comment 23 & 27]

>             if (i == length-1)
>-                break;
>+                return;

Doesn't this mean that mTitleWidth will be the width of an ellipsis here? Don't you want it to remain the width of the text?

Comment 17

[[e-mail]]

(In reply to comment #16)
>(From update of attachment 355054 [details] [diff] [review])
>>             if (i == length-1)
>>-                break;
>>+                return;
>Doesn't this mean that mTitleWidth will be the width of an ellipsis here? Don't
>you want it to remain the width of the text?
No, this is in the left cropping case, and we haven't been able to use any of the characters from the right end of the text. (The right cropping case already has the early return, and it's the incorrect mTitleWidth that causes this bug.)

Comment 18

[Markus Stange [:mstange]]

Does this patch fix bug 344579?

Comment 19

[[e-mail]]

(In reply to comment #16)
>(From update of attachment 355054 [details] [diff] [review] [details])
>>             if (i == length-1)
>>-                break;
>>+                return;
>Doesn't this mean that mTitleWidth will be the width of an ellipsis here? Don't
>you want it to remain the width of the text?
No, this is in the left cropping case, and we haven't been able to use any of
the characters from the right end of the text. (The right cropping case already
has the early return, and it's the incorrect mTitleWidth that causes this bug.)

(In reply to comment #18)
> Does this patch fix bug 344579?
It looks to be a very similar bug.

Comment 20

[Karsten Düsterloh]

Attachment: reftest testcase

Comment 21

[Karsten Düsterloh]

Attachment: reftest reference file

Comment 22

[Pardal Freudenthal (:ShareBird)]

Attachment: testcase

(In reply to comment #12)
> I don't think the patch is right.
> 
> Basically, I do think the bug is in the tree column cropping code - cropping
> and positioning of the ellipsis is done right until all of the text is gone.
> Only then the ellipsis "jumps" to the wrong position...

You are absolutely right. The real problem here AFAICS is that the ellipsis for text-align=right with crop=end is rendering wrong. For the other cases is right like this testcase demonstrate. I'm marking my patch as OBSOLETE.

But I still thinking the crop for text-align=right should have crop=start, so if someone else agree with this, I could open a new bug and attach that patch to it...

Comment 23

[[e-mail]]

Pushed changeset cd085064b5d1 to mozilla-central.

Comment 24

[[e-mail]]

Comment on attachment 355054 [details] [diff] [review]
Proposed patch
[Checkin: Comment 23 & 27]

Fixes painting issue reproducible easily in trees but also in other cases (see dependent bug). I can write a simpler branch patch if desired.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified on trunk with the reftest files:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090129 Minefield/3.2a1pre ID:20090129020336

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090129 Minefield/3.2a1pre ID:20090129033611

Can we checkin the reftest too, please?

Comment 26

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

Comment on attachment 355054 [details] [diff] [review]
Proposed patch
[Checkin: Comment 23 & 27]

a1.9.1=dbaron

Comment 27

[[e-mail]]

Pushed changeset 772c508564ee to releases/mozilla1.9.1

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on 1.9.1 branch with builds on OS X and Windows: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090211 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090211020510

Karsten, were you able to figure out how to get a patch for the reftest?

Comment 29

[Karsten Düsterloh]

Attachment: reftest as a patch [Checkin: See comment 33+34+35]

Comment 30

[Karsten Düsterloh]

The longer I stare at the trunk tree, the redder it gets - setting checkin-needed.

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 362108 [details] [diff] [review]
reftest as a patch
[Checkin: See comment 33+34+35]

That's the second patch which qimportbz gives me
{
UnicodeEncodeError: 'ascii' codec can't encode character u'\xfc' in position 15: ordinal not in range(128)
}
for :-/

Comment 32

[Serge Gautherie (:sgautherie)]

(In reply to comment #31)
> UnicodeEncodeError: 'ascii' codec can't encode character u'\xfc' in position
> 15: ordinal not in range(128)

Mnyromyr suggested this is triggered by the 'ü' in his name, and he was right: see bug 476994 comment 8.
Nothing wrong with the patch.

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 362108 [details] [diff] [review]
reftest as a patch
[Checkin: See comment 33+34+35]


http://hg.mozilla.org/mozilla-central/rev/57a0f015a43d

after fixing context for
{
patching file layout/reftests/bugs/reftest.list
Hunk #1 FAILED at 190
1 out of 1 hunks FAILED
}

***

I tried to save the qimportbz patch file as UTF-8,
which looks fine on Windows GUI,
but not in Hg web pages :-/

Comment 34

[Serge Gautherie (:sgautherie)]

Comment on attachment 362108 [details] [diff] [review]
reftest as a patch
[Checkin: See comment 33+34+35]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4071ed824459

after fixing (back !?) context for
{
patching file layout/reftests/bugs/reftest.list
Hunk #1 FAILED at 203
1 out of 1 hunks FAILED
}
and removing extra whitespace

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 362108 [details] [diff] [review]
reftest as a patch
[Checkin: See comment 33+34+35]


(In reply to comment #34)
> and removing extra whitespace

http://hg.mozilla.org/mozilla-central/rev/a750a4ebe950
(no bug) Removing ending whitespaces