137505 - extra space after <span style="font-variant: small-caps">foo</span> EXTRA SPACE HERE bar

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Sean Young]

At the page (see URL) the first few words of every chapter have the style
small-caps, but after that part there is extra space which shouldn't be there.

If you select the text, you see that the extra space does not become blue.

This is also verified with linux 2002041421. The HTML is valid html 4.01 strict
and the style is also verified.

Comment 1

[Sander van Rijnswou]

I see it too on Windows NT/ build 2002041408

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

Make sure that your browser is narrow enough to cause the text to wrap but not
so narrow that the small-caps content is the only thing on the first line.  The
problem is that the "justify" stretches out the spaces, but the spaces inside
the small-caps text are not stretched.	Layout _thinks_ they were, however, so
leaves a lot more space for the small-caps text than the text takes up.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Simon?  any idea what's up here?

Comment 4

[Simon Montagu :smontagu]

In nsTextFrame::RenderString, starting at
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsTextFrame.cpp#2649.

The justification spacing is applied in the |else| of 
     if (aTextStyle.mSmallCaps &&
         (IsLowerCase(ch) || (ch == kSZLIG))) {
so the spaces in the small-caps do not get justification (or word-spacing) applied.

It looks as if the bug is in IsLowerCase(), which is returning TRUE for spaces.

Comment 5

[Simon Montagu :smontagu]

Attachment: fix

Comment 6

[Scott Collins]

Comment on attachment 79302 [details] [diff] [review]
fix

hmmm.  well... this change does alter the semantics of the functions in
question.  Previously, only lower-case letters were considered lower-case. 
Now, everything that is not an upper-case letter is considered lower-case (and
similarly in reverse).	I don't see one reason to prefer one over the other as
long as the meaning is well documented.  It's not.  It seems as though this
could have been fixed as easily at the call site by changing |IsLowerCase| to
|!IsUpperCase| in two places.  This doesn't change the semantics of the
underlying calls.  I searched the mozilla tree.  We don't seem to make much use
of |IsUpperCase| and |IsLowerCase|, but still.	I'm leary of this.  Can you
argue why you should change the underlying functions rather than the call
sites?	And who reviewed this patch?  I see it is checked, but I don't see
r=someone.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Wouldn't it be better to just add | && ch != ' ' | to the |if| condition?  (with 
a comment explaining why)

Comment 8

[Christine Hoffman]

Changing QA contact

Comment 9

[Simon Montagu :smontagu]

I think comment 6 is based on a misunderstanding. The bug is exactly that in the
*current* semantics of |IsUpperCase()| and |IsLowerCase()| everything that is
not uppercase is considered lowercase and vice-versa. Consider the results of
the old and new IsLowerCase(c) for a few possible values of c.

c    ToLowerCase(c)   ToLowerCase(c)==c   ToUpperCase(c)   ToUpperCase(c)!=c
-    --------------   -----------------   --------------   -----------------

a         a                TRUE                  A              TRUE
A         a                FALSE                 A              FALSE
@         @                TRUE                  @              FALSE

The r= was from timeless.

Comment 10

[Simon Montagu :smontagu]

Notice also that before
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/html/base/src&command=DIFF_FRAMESET&file=nsTextFrame.cpp&rev2=1.330&rev1=1.329
nsTextFrame::RenderString was calling nsCRT::IsLower(), which has the same
semantics as my patch.
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCRT.cpp#126

Comment 11

[Scott Collins]

Comment on attachment 79302 [details] [diff] [review]
fix

OK, I had it backwards :-(  Now I'm convinced by my own argument.  sr=scc. 
Also, get timeless to put a comment in the bug.

Comment 12

[timeless]

fwiw you can see who marked a state in
http://bugzilla.mozilla.org/show_activity.cgi?id=137505
View Bug Activity
perhaps we should make it so that hovering over flags indicate who set it and
when (maybe even with a hyperlink to the comment if it exists)
r=timeless

Comment 13

[Simon Montagu :smontagu]

Fix checked in to trunk. I don't think this is sufficiently important to pursue
branch approval, but anyone who disagrees is welcome to take it on :-)