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

RESOLVED FIXED

Status

()

Core
Layout
--
minor
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Sean Young, Assigned: Marc Attinasi)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

16 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.

Comment 1

16 years ago
I see it too on Windows NT/ build 2002041408
Status: UNCONFIRMED → NEW
Ever confirmed: true
Created attachment 79259 [details]
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.
Simon?  any idea what's up here?
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.
Created attachment 79302 [details] [diff] [review]
fix

Updated

16 years ago
Attachment #79302 - Flags: review+

Comment 6

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

Comment 8

16 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.
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

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

Comment 12

16 years ago
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
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 :-)
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.