334928 - overlapping text when using Hebrew text with style: letter-spacing: 1px

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Niro]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060420 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060420 Minefield/3.0a1

The text is being displayed incorrectly and there is not enough space between the letters when using style: letter-spacing: 1px.
Testcase is coming.


Reproducible: Always

Comment 1

[Niro]

Attachment: Testcase

Comment 2

[Niro]

I've checked this on trunk version build 20060222 and the bug does not occur.
Therefore, it's regressed bug.

Comment 3

[Niro]

Attachment: Screenshot from build 2006022209

Screenshot: as it should be.

Comment 4

[Niro]

Attachment: Screenshot of the problem

Comment 5

[Uri Bernstein]

Confirming based on screenshots.

Comment 6

[Simon Montagu :smontagu]

Uri, does comment 5 imply that you can't reproduce on Mac? I can't reproduce on Linux.

A tighter regression window would be really helpful here.

Comment 7

[Niro]

I reproduced on build 20060226 but I could not reproduced on build 20060222.
Does it imply this is Cairo-related?

Comment 8

[Niro]

Please IGNORE comment #7. I posted it here instead of posting it in another bug report. I appologize.
I've checked with build 20060226 and the text is not overlapping but when selecting the text, there are some gaps between the letters.
I'll add screenshot.

Comment 9

[Niro]

Attachment: Screenshot for comment #8; build 20060226

Comment 10

[Uri Bernstein]

(In reply to comment #6)
> Uri, does comment 5 imply that you can't reproduce on Mac?

Correct. Sorry I didn't say this outright.

> A tighter regression window would be really helpful here.

Indeed. Unfortunately I can't help. Nir - can you try later builds and see when this started?

Comment 11

[Niro]

I could not reproduce it on build 20060220.
Is it tight enough?
If not, I'll check more builds.
Nir.

Comment 12

[Uri Bernstein]

(In reply to comment #11)
> I could not reproduce it on build 20060220.
> Is it tight enough?
> If not, I'll check more builds.
> Nir.
> 

You said earlier (comment #8) that you can't see the original problem (overlapping text) with 20060226. We need a better regression range for that problem, as it seems what we have now is only:
20060226 - OK (no overlapping text)
20060420 - broken

Let's deal with the selection issue later (perhaps in a separate bug).

Comment 13

[Niro]

Well, if I ignore the selection issue I get this:
20060407 - no overlapping text.
20060408 - broken (overlapping text as described in this bug).

Comment 14

[Uri Bernstein]

Checkins in this range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-04-07+04%3A00&maxdate=2006-04-08+07%3A00&cvsroot=%2Fcvsroot

Of which bug 327184 stands out as the most likely patch to have caused this.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, we don't support grapheme cluster level text layout.(This is same as non-Cairo build.) I think that we need to fix bug 333659 for this problem.

# I think that this is not a regression. Because pre bug 327184, the letter-spacing doesn't have any effect.

Comment 16

[Uri Bernstein]

(In reply to comment #15)
> Currently, we don't support grapheme cluster level text layout.(This is same as
> non-Cairo build.) I think that we need to fix bug 333659 for this problem.
> 

I don't see what this has to do with grapheme clusters. The tetscase only contains simple, standalone, letters.

> # I think that this is not a regression. Because pre bug 327184, the
> letter-spacing doesn't have any effect.
> 

No thaving any effect is still much better than distorting the text, making it hardly legible. So I certainly consider this a regression (and one which should be fixed prior to any release).

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #16)
> (In reply to comment #15)
> > Currently, we don't support grapheme cluster level text layout.(This is same as
> > non-Cairo build.) I think that we need to fix bug 333659 for this problem.
> > 
> 
> I don't see what this has to do with grapheme clusters. The tetscase only
> contains simple, standalone, letters.

Ah, O.K. I confirmed it. I'll check the code for 'simple' RTL text.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

simple fix.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

this is better, sorry.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Stuart:

Would you review the patch?

Comment 21

[Stuart Parmenter]

Comment on attachment 219517 [details] [diff] [review]
Patch rv1.1

oops, thought i had r='d this one

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 23

[Niro]

Verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060805 Minefield/3.0a1
on Windows XP.