Closed Bug 327184 Opened 14 years ago Closed 13 years ago

CSS property letter-spacing rendered incorrectly

Categories

(Core :: Graphics, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: hsumen, Assigned: pavlov)

References

()

Details

(Keywords: testcase)

Attachments

(8 files, 9 obsolete files)

15.25 KB, patch
vlad
: review+
Details | Diff | Splinter Review
8.78 KB, patch
Details | Diff | Splinter Review
3.72 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
621 bytes, text/html
Details
1.64 KB, patch
Details | Diff | Splinter Review
1.03 KB, text/html
Details
2.36 KB, patch
pavlov
: review+
Details | Diff | Splinter Review
19.43 KB, image/png
Details
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060214 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060214 Firefox/1.6a1

testcase - http://pxclassic.net/faces/letter-spacingTest.html

Letter-spacing is not being rendered correctly. Additionally, selecting text that has letter-spacing applied to it shifts the text (incorrectly renders it).

shockwave capture showing this, second line has letter-spacing applied...
http://pxclassic.net/faces/select.swf


Reproducible: Always
Depends on: 324870
Keywords: testcase
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: cairo
OS: Windows XP → All
Attached patch Patch rv1.0 (obsolete) — Splinter Review
This patch fixes on Windows. But it's crashed on justification text. And the letter-spacing width is not correct.
Attached patch Patch rv1.1 (obsolete) — Splinter Review
This patch fixes the problem. But we have another problem that is the error of rounding decimal.I think that we must not round to int in nsThebesFontMetrics. We should round to int just before rendering the string.
Assignee: nobody → masayuki
Attachment #213066 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #213171 - Flags: review?(vladimir)
Comment on attachment 213171 [details] [diff] [review]
Patch rv1.1

O.K. I understand the wrong mechanism. I'll create new patch.
Attachment #213171 - Flags: review?(vladimir) → review-
Attached patch Patch rv1.2 (obsolete) — Splinter Review
This fixes this bug and bug 324870 on Windows. On Mac and Linux(bug 324158), we need more work.
Attachment #213171 - Attachment is obsolete: true
Attachment #213230 - Flags: review?(vladimir)
Attached patch Patch rv1.3 (obsolete) — Splinter Review
Sorry. This is better.
Attachment #213230 - Attachment is obsolete: true
Attachment #213231 - Flags: review?(vladimir)
Attachment #213230 - Flags: review?(vladimir)
Attached patch Patch rv1.4 (obsolete) — Splinter Review
We have another problem. The width calculating is wrong on Windows.
Attachment #213231 - Attachment is obsolete: true
Attachment #213236 - Flags: superreview?(roc)
Attachment #213236 - Flags: review?(vladimir)
Attachment #213231 - Flags: review?(vladimir)
Attachment #213236 - Flags: superreview?(roc) → superreview?(pavlov)
Comment on attachment 213236 [details] [diff] [review]
Patch rv1.4

I checked in the fix to remove the abs() calls on abc.a and abc.c since that fixed 2 or 3 other bugs but was unrelated really to the letter-spacing problems.

I'm going over the rest of this now.
Attached patch Patch rv1.5 (obsolete) — Splinter Review
recreated for latest trunk.
Attachment #213236 - Attachment is obsolete: true
Attachment #213410 - Flags: superreview?(pavlov)
Attachment #213410 - Flags: review?(vladimir)
Attachment #213236 - Flags: superreview?(pavlov)
Attachment #213236 - Flags: review?(vladimir)
Assignee: masayuki → nobody
Status: ASSIGNED → NEW
Sorry about that...
Assignee: nobody → masayuki
*** Bug 331882 has been marked as a duplicate of this bug. ***
*** Bug 332042 has been marked as a duplicate of this bug. ***
Seems that some other checkin has resolved this bug in Today's build.
(In reply to comment #12)
> Seems that some other checkin has resolved this bug in Today's build.
> 

I think you're testing the wrong builds.  Nothing has gone in to fix this.
Attached patch Patch v2.0Splinter Review
Sorry for taking so long to get to this.  I've gone ahead and done this based on the patch from Masayuki.  Instead of passing the pointer through to Draw, I've made it call a setter.
Assignee: masayuki → pavlov
Attachment #213410 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #217631 - Flags: review?(vladimir)
Attachment #213410 - Flags: superreview?(pavlov)
Attachment #213410 - Flags: review?(vladimir)
Comment on attachment 217631 [details] [diff] [review]
Patch v2.0

I /think/ this is ok; r=me provided you fix any other platforms if they break!
Attachment #217631 - Flags: review?(vladimir) → review+
Stuart:

Hi, thank you for your work.

But you round to dev unit on |nsThebesFontMetrics.cpp|. I think that we should round it on |gfx*Fonts.cpp|. When I select the text that is applied letter-spacing or jsutify, the characters shaken. Please check it.
Sorry, I misread your patch, comment16 has some wrong things.
But we have some rounding bugs, this patch fixes some problems, but not perfectly. ..
Attachment #217873 - Flags: review?(pavlov)
Attached patch Patch rv1.0 for Pango (obsolete) — Splinter Review
This fixes this bug on Pango. But I can test only on Linux. I cannot test on BeOS. I don't know this patch fixes this bug on BeOS too.
Attachment #218069 - Flags: review?(pavlov)
Comment on attachment 217873 [details] [diff] [review]
Patch for suppress text shaken on selecting

I'm worried about these changes:
-    textrun->Draw(aContext->Thebes(), gfxPoint(NSToIntRound(aX * app2dev), NSToIntRound(aY * app2dev)));
+    textrun->Draw(aContext->Thebes(), gfxPoint(aX * app2dev, aY * app2dev));

We really want these to be rounded -- not rounding them means we'll draw sub-pixel text causing the text to look blurred.

Keeping the rest in floats is probably a good idea though.

I'll play with this some more and see what I can come up with.
Depends on: 334512
Blocks: 334512
No longer depends on: 334512
Depends on: 334928
Comment on attachment 217873 [details] [diff] [review]
Patch for suppress text shaken on selecting

I found another problem on selection, I think that we need to change nsTextFrame's selection text rendering code for thebes after bug 333659.
Attachment #217873 - Flags: review?(pavlov)
Attachment #218069 - Flags: review?(pavlov) → review+
*** Bug 324158 has been marked as a duplicate of this bug. ***
*** Bug 335869 has been marked as a duplicate of this bug. ***
Flags: blocking1.9a1? → blocking1.9a1+
Attachment #220204 - Attachment description: Patch rv1.1 for Pango(cehcked-in) → Patch rv1.1 for Pango(checked-in)
(In reply to comment #17)
> You can test on here.
> https://bugzilla.mozilla.org/attachment.cgi?id=209763
> 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060522 Minefield/3.0a1 ID:2006052205 [cairo]

Testcase still shows shifting text. Is there a seperate bug for this?
(In reply to comment #26)
> (In reply to comment #17)
> > You can test on here.
> > https://bugzilla.mozilla.org/attachment.cgi?id=209763
> > 
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060522
> Minefield/3.0a1 ID:2006052205 [cairo]
> 
> Testcase still shows shifting text. Is there a seperate bug for this?
> 

Yeah, but we cannot try to fix it. see comment 21.
Stuart:

The using mSpacing rendering was broken by bug 340590 on Win32.
But I cannot find the wrong logic, do you have an idea?
Not sure yet -- Going to look at this and some perf stuff next.
Stuart:

This patch fixes the regression. But this makes another regression that is some characters width returns wrong value. I don't understand what means the value.
We can't use the 32 scale -- it breaks bitmap fonts because they have a set of specific sizes they support and can't scale like truetype fonts.  I don't know why it would help here either.  It didn't before...
Attached file testcase 2 (obsolete) —
I found interesting thing. If opacity is not 1.0, this regression is not reproduced.
Attached file testcase 2
Oops. Sorry, previous testcase is wrong.
Attachment #225615 - Attachment is obsolete: true
I think that we lost the accuracy for calculating the position of characters by bug 340590. Therefore, we can look the problem easily. But we have had the shaking problem already. This regression may not be a new one.
O.K. This patch fixes the regression. We should round the spacing before gfxTextRun::Draw.
Attachment #225711 - Flags: review?(pavlov)
This is better. This patch fixes the shaking problem at selecting on Windows(On linux, it's still there.)
Attachment #225711 - Attachment is obsolete: true
Attachment #225991 - Flags: review?(pavlov)
Attachment #225711 - Flags: review?(pavlov)
(In reply to comment #37)
> (On > linux, it's still there.)

Sorry. This comment is wrong. I confirmed it's fixed on Linux too.
Attachment #225991 - Flags: review?(pavlov) → review+
is this a problem still on linux or mac?
Has patch check-in resulted in regression? 

For example, a 9 point font (predominantly, although others) rendering is over-spaced, often overflowing, and a mis-match is noticeable with linked text between the rendered area and the link activation zone. 

For example, please see text under "World News Headlines" on the bottom right of the following page, and move mouse over links;
http://www.stuff.co.nz/stuff/0,2106,3716180a12,00.html
(In reply to comment #43)
> Has patch check-in resulted in regression? 
> 
> For example, a 9 point font (predominantly, although others) rendering is
> over-spaced, often overflowing, and a mis-match is noticeable with linked text
> between the rendered area and the link activation zone. 
> 
> For example, please see text under "World News Headlines" on the bottom right
> of the following page, and move mouse over links;
> http://www.stuff.co.nz/stuff/0,2106,3716180a12,00.html

I cannot find the wrong rendering on 2006062904-trunk/Win2k. What's your environment and would you attach a simple testcase or screenshot?
also WFM
I think this image shows the problem mentioned in comment 43:
  http://twpol.dyndns.org/temp/text_links_1.png

Importantly, this only occurs at some sizes - if I Ctrl-- or Ctrl-+, it is fine, and I expect it is rendering a different sized font to that which it is calculating the size from. More than that I don't know.

As the testcase attached to this bug does not have a problem as far as I can make out, I don't think it is necessarily related to this bug at all, but it certainly is broken (build ID 2006-07-03-20).
(In reply to comment #46)

There are no letter-spacing, word-spacing, justification and small-caps in the screenshot... It is not a this bug.
(In reply to comment #44)
> (In reply to comment #43)
> > Has patch check-in resulted in regression? 
> > 
> > For example, a 9 point font (predominantly, although others) rendering is
> > over-spaced, often overflowing, and a mis-match is noticeable with linked text
> > between the rendered area and the link activation zone. 
> > 
> > For example, please see text under "World News Headlines" on the bottom right
> > of the following page, and move mouse over links;
> > http://www.stuff.co.nz/stuff/0,2106,3716180a12,00.html
> 
> I cannot find the wrong rendering on 2006062904-trunk/Win2k. What's your
> environment and would you attach a simple testcase or screenshot?

Example screenshot showing uneven letter spacing, and mismatch between rendered text and active link zone. This example also shows a larger font than that normally affected.

http://img220.imageshack.us/img220/5821/telecom6za.jpg
(In reply to comment #48)
> (In reply to comment #44)
> > (In reply to comment #43)
> > > Has patch check-in resulted in regression? 
> > > 
> > > For example, a 9 point font (predominantly, although others) rendering is
> > > over-spaced, often overflowing, and a mis-match is noticeable with linked text
> > > between the rendered area and the link activation zone. 
> > > 
> > > For example, please see text under "World News Headlines" on the bottom right
> > > of the following page, and move mouse over links;
> > > http://www.stuff.co.nz/stuff/0,2106,3716180a12,00.html
> > 
> > I cannot find the wrong rendering on 2006062904-trunk/Win2k. What's your
> > environment and would you attach a simple testcase or screenshot?
> 
> Example screenshot showing uneven letter spacing, and mismatch between rendered
> text and active link zone. This example also shows a larger font than that
> normally affected.
> 
> http://img220.imageshack.us/img220/5821/telecom6za.jpg

I cannot find the same page of the screenshot. And I cannot find the letter-spacing specified for anchor in the site. (only p.kicks has "letter-spacing: 1px;", but I cannot find the element in some pages.) 'letter-spacing' means the property of CSS. Do you understand?

Please file a new bug. (And also please write a your environments in the new bug, i.e., OS, build ID of Fx, the default font in your settings of Fx. Because I cannot reproduce the bug.)
Yeah, what you're seeing isn't related to letter spacing.  I've seen it a couple of times and have some ideas on the problem, but haven't gotten to look in more details yet.  Please file a new bug.
I have some reports. Some linux testers said that this bug is still existing.(the text is moving by selecting.) But I cannot confirm it. I'll ask/confirm to the testers ASAP.
Text-select shifting is rock solid in Windows now, nice work. Can someone confirm for Linux?
Testcase...
https://bugzilla.mozilla.org/show_bug.cgi?id=327184#c17

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060803 Minefield/3.0a1 ID:2006080304 [cairo]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060803 Minefield/3.0a1

Yeah, the testcase in attachment 209763 [details] works for me. 

Masayuki, have you heard back from any of the Linux testers that reported this wasn't working?
Whiteboard: cairo
I heard to the reporters, the all reporters cannot reproduce the shaking problem now. But Mac doesn't have the spacing rendering mechanism, but we should mark to FIXED?
If you believe the issue was fixed by one of the patches in this bug, yes. Otherwise mark it as WORKSFORME.

And yeah, just file another bug for the Mac issue.
This bug should be marked fixed.
Attached image screenshot
Screenshot of the testcase https://bugzilla.mozilla.org/attachment.cgi?id=225152 on a gtk2 build of Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060827 Minefield/3.0a1.

No text-select shifting though, but text is not displayed after the first space (the first line should be "letter-spacing: 0.12em;"). To reproduce this behavior, use truetype fonts and libfreetype with bytecode interpreter enabled.
this works on windows and linux for me.. file new bugs for any remaining issues.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.