430926 - Tab character displayed almost invisibly thin

Status: RESOLVED FIXED

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

Description

[Tom Edwards]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

In the code block seen at this bug's URL, Firefox tries to align the second parameter of $sequence (penultimate line) with the parameters of the commands above it, causing it to crash into its first parameter ('idle').

Reproducible: Always

Steps to Reproduce:
1. Visit bug URL
2. Observe penultimate line of code block
3. Compare to View Source rendering
Actual Results:  
Tab character between 'idle' and '"myfirstmodel-idle.smd"' is almost invisible.

Expected Results:  
Tab character aligns '"myfirstmodel-idle.smd"' somewhere further to the right, creating a visual space between it and 'idle'.

Bug only affects monospace fonts.

Comment 1

[Mats Palmgren (inactive)]

Yeah, I think 1 appunit is to little spacing here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsTextFrameThebes.cpp&rev=3.181&root=/cvsroot&mark=2345-2349#2321
even 1 CSS pixel is too small IMO -- I tried different values
and 1/2 of a space is what I think is the minimal for me to
clearly detect a word separation.

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Advance by at least half the width of a space.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Trying to encode the minimum visually-obvious width here seems like a bit of a hack. Isn't the real problem here that the tab widths are measured from the padding-edge or border-edge instead of the content edge? The spec says to use the content-edge and if we did, this bug would never happen for monospaced text, right?

http://www.w3.org/TR/CSS21/text.html#white-space-model

Comment 5

[Mats Palmgren (inactive)]

Yeah, after reading the spec I agree we shouldn't try to force a gap.
As you said, it's a border/content-edge bug and with that fixed the
problem will not occur unless there are mixed font-sizes, inline
padding/border, float displacement, or stuff like that which puts
the inline content edge off the block grid (which is probably rare
where tabs are used).

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase #4 (requires Ahem font)

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

GetCurrentFrameXDistanceFromBlock() returns distance between frame edges,
we need to subtract border+padding to get the content edge distance.
I'm a bit puzzled by the RTL case though -- why does it include the
*left* BorderPadding?  given the documentation I expected the right:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsLineLayout.h&rev=3.119&root=/cvsroot&mark=330-332#328

This patch makes Testcase #4 render correctly AFAICT.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The documentation is wrong. GetCurrentFrameXDistanceFromBlock returns the accumulated advance width of frames before the current frame on the line, plus the left border+padding. So your code is correct and you can drop the conditional. If you could fix that documentation too, and provide testcase #4 as a reftest, then we'll be all good here :-).

Comment 9

[Mats Palmgren (inactive)]

Attachment: reftest.diff

Requires Ahem font; I presume Tinderboxen that run reftest have it(?)

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Comment 11

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

(In reply to comment #9)
> Requires Ahem font; I presume Tinderboxen that run reftest have it(?)

They don't, and I'd prefer that we not require it if possible, since it imposes the requirement not only on the tinderboxes but on any developers who want to run reftests.  (Once we support downloadable fonts, however, we could have it in the source tree and reference it.)  See bug 423097.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Or we could use SVG fonts and it wouldn't even be subject to the vagaries of platform font shaping and rasterization.

Comment 13

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/2185e79a3614

I'll wait with the reftest until we have the Ahem font available.

-> FIXED

Comment 14

[Tom Edwards]

If this is going into Mozilla, how long might it be until it makes it's way into Firefox?

Comment 15

[Mats Palmgren (inactive)]

It's already in the latest nightly Firefox builds if you want to test it:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/
The first release that will have the fix is Firefox 3.1, by the end of the
year, I believe.