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




17 years ago
17 years ago


(Reporter: sean, Assigned: attinasi)



Firefox Tracking Flags

(Not tracked)




(2 attachments)



17 years ago
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.
I see it too on Windows NT/ build 2002041408
Ever confirmed: true
Created attachment 79259 [details]

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.
Simon?  any idea what's up here?
In nsTextFrame::RenderString, starting at

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.


17 years ago
Attachment #79302 - Flags: review+

Comment 6

17 years ago
Comment on attachment 79302 [details] [diff] [review]

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
Wouldn't it be better to just add | && ch != ' ' | to the |if| condition?  (with 
a comment explaining why)

Comment 8

17 years ago
Changing QA contact
QA Contact: petersen → amar
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 11

17 years ago
Comment on attachment 79302 [details] [diff] [review]

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.
Attachment #79302 - Flags: superreview+

Comment 12

17 years ago
fwiw you can see who marked a state in
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)
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 :-)
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.