Closed
Bug 137505
Opened 22 years ago
Closed 22 years ago
extra space after <span style="font-variant: small-caps">foo</span> EXTRA SPACE HERE bar
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: sean, Assigned: attinasi)
References
()
Details
Attachments
(2 files)
921 bytes,
text/html
|
Details | |
820 bytes,
patch
|
timeless
:
review+
scc
:
superreview+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
I see it too on Windows NT/ build 2002041408
Updated•22 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•22 years ago
|
||
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•22 years ago
|
||
Simon? any idea what's up here?
Comment 4•22 years ago
|
||
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•22 years ago
|
||
Attachment #79302 -
Flags: review+
Comment 6•22 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.
Comment 7•22 years ago
|
||
Wouldn't it be better to just add | && ch != ' ' | to the |if| condition? (with a comment explaining why)
Comment 9•22 years ago
|
||
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•22 years ago
|
||
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•22 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•22 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
Comment 13•22 years ago
|
||
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
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•