58384 - letter-spacing causes walking when text is aligned

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Hixie (not reading bugmail)]

STEPS TO REPRODUCE
   1. Right-align some text.
   2. Set the letter-spacing to some non-zero value.
   3. Resize the window.

ACTUAL RESULTS
   Text walks left and is no longer right aligned.

Reproduced on a Win2K Netscape commercial branch build.

Comment 1

[Hixie (not reading bugmail)]

Attachment: Simple testcase. Resize the window to see the effect.

Comment 2

[Pierre Saslawsky]

It works for me on Mac-MN6-2000-10-17.  What build are you using?

Comment 3

[Chris Waterson]

Weird. It definitely does *not* work for me on Win32, either in mozilla or 
viewer.

Comment 4

[Marc Attinasi]

Yea, I see this too (Win2K and Linux)

Comment 5

[Hixie (not reading bugmail)]

*** Bug 67348 has been marked as a duplicate of this bug. ***

Comment 6

[Hixie (not reading bugmail)]

See also bug 86279, which is almost certainly related.

Comment 7

[Chris Waterson]

Attachment: include letter spacing when trimming trailing whitespace

Comment 8

[Chris Waterson]

The problem is that we include the letter spacing implicitly in the word spacing
when we set up the TextStyle struct (around nsTextFrame.cpp:577), however, we
back out only the word spacing in nsTextFrame::TrimTrailingWhiteSpace(). The fix
is to back out the letter spacing, as well. This should fix bug 86279, also.

Comment 9

[Shanjian Li]

The inconsistency in using mWordSpacing is very confusing. To include 
TextStyle::mLetterSpacing implicitly inside TextStyle::mWordSpacing might not be 
a good idea. But either way, your fix is necessary. 
r=shanjian

Comment 10

[Chris Waterson]

Yeah, it might be easier on the brain-print to explicitly add the letter-spacing
where appropriate instead of building it in to the word spacing, but I could go
either way. pierre did this in r1.277 of the file to fix bug 1046: perhaps to
make it through the PDT's ``smaller must be safer'' gauntlet? ;-)

If we decide to leave the letter-spacing munged in with the word-spacing, we
probably ought to remove some of the redundant tests that check both values to
see if they're non-zero: we can simply check the word-spacing.

Comment 11

[Chris Waterson]

Attachment: alternative patch; explicitly include letter spacing where appropriate

Comment 12

[Pierre Saslawsky]

The spec is a little bit ambiguous.  Please make sure that the results are 
similar to what we get with IE5 on Mac and Windows for left-aligned, right-
aligned and centered text.  Setting a large value for letter-spacing may help you 
to see the problems, if any.

Comment 13

[rbs]

I find the alternative patch nicer.

Comment 14

[Shanjian Li]

(to pierre) Because the spec is ambiguous, we are allowed to choose either way. 
I believe there is strong reasons to include letter-spacing inside word-spacing 
when doing layout. A obvious one is that if use specify a larger letter-spacing 
and a very small word-spacing, the space between letters inside a word might be 
larger than word-spacing. That is unacceptable. The problem that this bug  
targets is the inconsistency in such usage. 

(to waterson) The alternative patch does not seem to consume significant extra 
CPU cycles. I prefer it too.  (r=shanjian)

Comment 15

[Pierre Saslawsky]

Shanjian: It is precisely because we are allowed to choose either way that I 
recommended we choose the way that is compatible with competitors.  

I also recommended we use a testcase that sets a relatively large value for 
letter-spacing because the problems, if any, will be more easily noticeable (we 
seem to be on an agreement here).  The problems I was thinking of were not about 
whether we should add letter-spacing when we calculate the spacing between words 
(the issue was settled in bug 1046) but to a possible spurious spacing on the 
right-hand side of a right-aligned text with an 'ltr' direction (or the left-hand 
side of a left-aligned text with a 'rtl' direction) or even with centered text.  
I think we originally had a bug like this and I would like it to come back.

Comment 16

[Chris Waterson]

The code in TrimTrailingWhiteSpace() is what ensures we don't leave the word and
letter spacing at the end of the line. I haven't tried it on left-aligned RTL
text though.

Comment 17

[Chris Waterson]

Attachment: test case illustrating rtl & ltr text left-aligned, right-aligned, centered, justified

Comment 18

[Chris Waterson]

Comment on attachment 51097 [details]
test case illustrating rtl & ltr text left-aligned, right-aligned, centered, justified

Boioioing! Wrong MIME type.

Comment 19

[Chris Waterson]

Attachment: test case illustrating rtl & ltr text left-aligned, right-aligned, centered, justified

Comment 20

[Chris Waterson]

Attachment 51099 [details] illustrates three bugs, which I'll file separately.

1. Right-aligned and justified text are not flush with the right edge
   of the div.

2. At certain widths, right-aligned text is incorrectly measured, and spills
   over the boundary of the div. (Possibly poor accounting for a space
   at the end of a line?)

3. Justified RTL text that has only a single word on a line ends up aligning
   the word with the left edge of the div, not the right.

These problems exists regardless of whether the patch is applied.

Comment 21

[rbs]

Some related bugs: bug 18160 and bug 92671

Comment 22

[Chris Waterson]

Looks like bug 18160 corresponds to problem (2), above. It looks like (1) likely
has to do with the letter-spacing: altering that value in the test case above
shows that the space between the right edge of the div and the right edge of the
text differs by the letter spacing. I've filed bug 102016 on that issue. I've
filed bug 102019 on issue (3).

Comment 23

[Chris Waterson]

Attachment: as above, with merge conflicts fixed after rbs's landing

Comment 24

[Marc Attinasi]

Comment on attachment 51130 [details] [diff] [review]
as above, with merge conflicts fixed after rbs's landing

sr=attinasi - glad to see wh have a new TextFrame expert ;)

Comment 25

[Chris Waterson]

Fix checked in.