Closed Bug 22502 Opened 25 years ago Closed 25 years ago

"white-space: -moz-pre-wrap; width: 10ch;" doesn't work with variable-width fonts

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Platform:
Other
Other
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: akkzilla, Assigned: akkzilla)

Details

(Keywords: regression, Whiteboard: [PDT+] Have workaround)

Attachments

(1 file)

Patch to add font: monospace to style
4.11 KB, patch
Details | Diff | Splinter Review 
Plaintext mail uses style="white-space: -moz-pre-wrap; width: 72ch; ..." to
achieve wrapping.  This used to work, but on the tip, plaintext mail and the
plaintext edit window no longer wrap; instead, when you type a long line, a
horizontal scrollbar appears.  This makes it hard to compose plaintext email
messages, though the message is still sent correctly wrapped.
Assignee: troy → kipp
Probably a block/inline issue
Whiteboard: PDT-
Marking PDT- because you can still compose and send, and there is a workaround
of using HTML compose
Priority: P3 → P1
Target Milestone: M14
nobody's touched this code in a while, so I don't know what the problem might
be.  Setting to M14 (the first milestone in which Kipp's bugs will start getting
fixed.)  Set to P1.
Summary: [DOGFOOD] [REGRESSION] -moz-pre-wrap doesn't → [DOGFOOD] [REGRESSION] [BLOCK] -moz-pre-wrap doesn't
Whiteboard: PDT-
Requesting PDT reconsideration.  Using html compose isn't an option for people
who need to send plaintext mail (e.g. to people who use other mailers, or to
newsgroups or mailing lists) because of bug 17072.  Either 17072 or this one
should be marked dogfood since otherwise there's no way to send plaintext mail.
Whiteboard: [PDT+]
Putting on PDT+ radar.
do we have any idea when this stopped working?  that would be very helpful to
know.
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
It looks like this has magically gotten fixed again ... seems to be working in
today's build.  Yeehaw!
akkana:  if you could provide a simple test case, I will add that to the
regression test suite I'm building for block/inline layout.  Just attach it to
this bug.  That'll help QA verify as well.

Regression test cases must be a url that can be run in viewer (not mozilla, so
no XUL) and that demonstrates the problem without any user intervention.
This is broken again -- the plaintext editor isn't wrapping even though it has
<body style="white-space: -moz-pre-wrap; width: 72ch; ">
Status: RESOLVED → REOPENED
severity set to critical. assigning to myself, even though I haven't touched a 
line of code that effects this, it's in my area so I'll help hunt it down.  
again, we can pinpoint a day where it stopped working, I can scan the list of 
checkins for that day and hopefully find the culprit.
Chris, can you help with tracking down when this broke, and also which 
platform(s) it is broken on?
Assignee: kipp → buster
Severity: normal → critical
Status: REOPENED → NEW
Correction: it turns out it is wrapping, but at roughly double the number of
characters specified in the style.  I see this on Linux and Windows both (my Mac
is still building).

Note also that we're still seeing variable width fonts in plaintext, which might
make wrapping at a fixed column width look a little weird ...  there's a
separate bug on that (rods?)

Can't pinpoint when it broke since it's been broken off and on for so long, and
in different ways.
Resolution: FIXED → ---
marking potential beta1 bugs
Keywords: beta1
Putting dogfood in the keyword field.
Keywords: dogfood
changed the summary to reflect what's going on.
measuring the width of the body in characters ("width: 72ch") resolves to a 
numeric width in twips.  It uses a representative character from the current 
font to generate the width in twips.  If the font is monospace, you get exactly 
what you expect.  If the font is variable-width, you get 72 representative 
characters in width, not a counting of 72 characters per line (egads, what a 
performance dog that would be!)

All that is needed is whenever we are creating a plaintext edit window, add a 
style attribute on the body "font: monospace;" (careful, don't mess with single 
line text controls who also use -moz-pre-wrap).  This should be the 
responsibility of the application which knows it's a plain text editor *with 
wrapping* to set a flag on the editor to indicate that "font: monospace;" is 
needed.  I guess another way of saying that is whenever you know to set the 
wrapping to "width: 72ch;", you also know that you need "font: monospace;"  So I 
guess a separate flag really isn't necessary if the app is already responsible 
for telling us how many characters we should wrap at (or for telling us to pick 
some default number of characters, however that is done today.)
Assignee: buster → akkana
Summary: [DOGFOOD] [REGRESSION] [BLOCK] -moz-pre-wrap doesn't → "white-space: -moz-pre-wrap; width: 10ch;" doesn't work with variable-width fonts
I still don't understand why the style system doesn't do this automatically. 
What you're basically saying is that "white-space: -moz-pre-wrap; width=" is
useless unless it's also accompanied by a "font: monospace".  If moz-pre-wrap
always relies on monospace, isn't there any way it can set that automatically?

I can put font: monospace into the editor code; but it seems like we're working
around a problem that ought to be fixed at a lower level.
Status: NEW → ASSIGNED
Keywords: regression
Doing it this way is slightly more flexible.  It's possible that someone might 
want "width: 10 ch;" for a variable width font and really mean that, though 
they'd probably have to understand how we measure "ch" widths.  I think the 
algorithm is in the CSS spec.  Maybe it makes sense for some non-english or 
symbolic fonts, I don't know.  It's a bit of a stretch, but I'd rather have the 
flexibility in the engine in case someone has a need, rather than be overly 
restrictive.  Let the style system interpret the style it's given, not make 
guesses about what styles go together.  Who knows what's coming in future revs 
of CSS that might effect this?  If the style system is pure, we're in a much 
better position to support new extensions.
Okay, I guess the flexibility is good, though it would be nice to have a default
so all clients who want this functionality don't have to know that both are
required.  We should aim at having good documentation on this eventually.

Now, here's the other problem: I have code to build up a string:
"font: monospace; white-space: -moz-pre-wrap; width: 72ch;"
and I attempt to insert that as a style value on the body, via:
bodyElement->SetAttribute(styleName, styleValue);
(styleName is nsAutoString "style"), then I do a fragment dump of the body node
immediately afterward, and what I get is:
body@0x8705bcc style=white-space: -moz-pre-wrap; width: 72ch;  refcount=10<
>

In other words, for some reaon it doesn't work to set font: monospace as part of
the value of a style attribute.  (I've tried putting the font: monospace at the
end of the string instead of the beginning, doesn't help.)

Any idea why this might be?  Who would know more about this?  BTW, I'll attach
the patch that shows this.
Turns out that it should be font-family, not font.  With that, it seems to work,
and the code has been reviewed and is ready to check in when the tree opens.

Joe expressed some concern about not being able to use a variable-width font for
his plaintext mail windows (apparently this is a fairly common thing to do on
Mac -- I've heard other people on the newsgroups and in bug reports).  We both
think that the problem of the width being recalculated for variable width fonts
(so that it typically wraps at twice the number of characters specified) is
still a bug that should be fixed.  But meanwhile, I'll check in this workaround
for M14.  The remaining bug (which the summary still describes) probably doesn't
need to be beta1/PDT+ once the workaround is checked in.

Sending back to buster in the hope that he knows who owns the font sizing code
used for wrapping inside layout.
Assignee: akkana → buster
Status: ASSIGNED → NEW
Whiteboard: [PDT+] → [PDT+] Have workaround
I own the layout code that determines the wrapping width.  It's not a font 
metrics issue, it would mean a wholesale change in the way we calculate where to 
wrap.  A lot of work, not for first release when I look at the list of Kipp's 
bugs that need to get fixed.  So when you check in your fix, go ahead and mark 
this an enhancement with a milestone of M20 and a keyword "helpwanted."  If 
someone else wants to tackle this, I can guide them to the right spot.  But I 
wouldn't recommend any netscape developer spending time on it, since it isn't on 
our beta list.
I've checked in the fix, so we'll always see monospace font.  Meanwhile, it
turns out, the bug that was actually reported -- that it didn't behave as Steve
said and in fact wrapped at roughly double the number of letters specified (i.e.
it didn't use a representative character, but something much larger) has
magically gotten fixed.  Now with variable-width, it uses a width somewhere
between an m and an n, which seems fairly reasonable.  I think this bug can be
closed.

Joe, you might want to give some thought to whether we want a UI in the editor
(or elsewhere) for a pref to use a non-monospace font for plaintext.  Perhaps in
one of the editor's built-in style sheets?  That's probably a separate bug,
though.
Assignee: buster → akkana
Marking fixed.
Status: NEW → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → FIXED
I would like to verify this fix but not sure what steps I need to do. Akkana, 
could you please provide a way to check this ?

See the description at the very beginning of this bug.  Bring up plaintext mail
or the plaintext editor window, type a bunch of words, and see if it wraps at
somewhere around 72 columns instead of bringing up a horizontal scrollbar.
With the Feb 09 th build (2000020908), this problem appears to be fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: