Closed Bug 588 Opened 26 years ago Closed 24 years ago

{feature} [TEXT] Full justification of text doesn't work

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: angus, Assigned: roc)

References

(
URL
)

Details

(Keywords: css1)

Attachments

(4 files)

Simple fully justified text test case
642 bytes, patch
Details | Diff | Splinter Review
Implementation of text justification
28.92 KB, patch
Details | Diff | Splinter Review
A hacked-up test case that exercises spacing and PRE with justification
1.40 KB, text/html
Details
Patch for justification support + assorted bugfixes, version 2
31.21 KB, patch
Details | Diff | Splinter Review 
We don't support justified text yet. Does this fit in with the whitespace work?
It's also a style issue. See URL for test case and sample code.
Status: NEW → ASSIGNED
Component: Unknown → Style System
Priority: P2 → P3
It is now partially implemented, though it could use some work. I'm leaving the
bug open, but at a reduced priority because there are some easy to create
conditions where the justification fails to operate as expected.
*** Bug 1047 has been marked as a duplicate of this bug. ***
Setting all current Open/Normal to M4.
*** Bug 3222 has been marked as a duplicate of this bug. ***
Summary: Full justification of text doesn't work → Full justification of text doesn't work
The regression I reported in 3222 occurred between the builds of 99-02-11 and
99-02-16.
*** Bug 2475 has been marked as a duplicate of this bug. ***
*** Bug 1783 has been marked as a duplicate of this bug. ***
*** Bug 3923 has been marked as a duplicate of this bug. ***
*** Bug 3493 has been marked as a duplicate of this bug. ***
Severity: normal → enhancement
Priority: P3 → P5
Summary: Full justification of text doesn't work → {feature} Full justification of text doesn't work
*** Bug 4397 has been marked as a duplicate of this bug. ***
*** Bug 4447 has been marked as a duplicate of this bug. ***
See also http://schist/4447.html
*** Bug 5017 has been marked as a duplicate of this bug. ***
Summary: {feature} Full justification of text doesn't work → {css1{feature} Full justification of text doesn't work
Summary: {css1{feature} Full justification of text doesn't work → {feature} Full justification of text doesn't work
Priority: P5 → P4
Summary: {feature} Full justification of text doesn't work → {css1} {feature} Full justification of text doesn't work
Increasing priority a little because this is a CSS1 issue and there are some
CSS2 issues at enhancement/P5.

The interaction of text-align:justify and white-space:pre is covered by bug
6011.
Summary: {css1} {feature} Full justification of text doesn't work → [4.xP]{css1} {feature} Full justification of text doesn't work
Priority: P4 → P5
*** Bug 10798 has been marked as a duplicate of this bug. ***
Summary: [4.xP]{css1} {feature} Full justification of text doesn't work → [4.xP] {css1} {feature} Full justification of text doesn't work
*** Bug 14279 has been marked as a duplicate of this bug. ***
Target Milestone: M17 → M15
*** Bug 19195 has been marked as a duplicate of this bug. ***
A simple testcase (also demonstrating bug 20260) is at
http://www.thirdnipple.com/mozilla/.

This also occurs without the table in this page, but I didn't feel like typing a
whole screen's worth of filler text :) I'd suggest that this is a problem which
ought to be fixed before beta, as a lot of websites rely on full justification
to look nice... The priority IMHO ought to be higher, but I'll leave that to the
people who know this bug better.
*** Bug 20703 has been marked as a duplicate of this bug. ***
*** Bug 20571 has been marked as a duplicate of this bug. ***
Assignee: kipp → pierre
Status: ASSIGNED → NEW
Updating to default Style System Assignee...kipp no longer with us :-(
Summary: [4.xP] {css1} {feature} Full justification of text doesn't work → [4.xP] {css1} {feature} [TEXT] Full justification of text doesn't work
Keywords: css1
Migrating from {css1} to css1 keyword. The {css1}, {css2}, {css3} and {css-moz}
radars should now be considered deprecated in favour of keywords.
I am *really* sorry about the spam...
Assigned to kipp so that this hopefully doesn't get
overlooked for much longer by the new layout kings.
OS: Windows 95 → All
Hardware: PC → All
based on the number of pages effected, the comments in this bug, and follow-up 
comments on the newsgroup, maing this P1.
Priority: P3 → P1
Setting the keyword all open [4.xp] bugs to 4xp.
Keywords: 4xp
*** Bug 25803 has been marked as a duplicate of this bug. ***
*** Bug 26287 has been marked as a duplicate of this bug. ***
Keywords: beta1
Adding 'beta1' to keywords. Beta1 should support all CSS1 styles. 
Putting on PDT+ radar for beta1.
Summary: [4.xP] {css1} {feature} [TEXT] Full justification of text doesn't work → {feature} [TEXT] Full justification of text doesn't work
Whiteboard: [PDT+]
this is likely to be very hard.
Whiteboard: [PDT+] → [PDT+] no idea when this will land, no one has looked into this yet
This used to work (somewhat buggily) about a year ago, so the code is there,
somewhere.  It stopped working correctly in March 1999 and then I think Kipp
disabled it.  It may also have had i18n problems.
Give it to Knuth! Knuth will code anything!
I looked at this today.  There is some code in the block frame that figures out 
whether a line can be fully justified and passes that boolean to 
nsLineLayout::HorizontalAlignFrames().  This code was ifdefed out and I 
re-enabled it in my build.  Even with this enabled, though, text does not get 
fully justified because HorizontalAlignFrames() doesn't do anything for that 
case.

I'm attaching a really simple test case which has a single line of fully 
justified text.

Since I can't find any code that tries to fully justify text, implementing it 
will take some time.  This is definitely not possible for M14.  Updating status 
whiteboard to indicate as such.
Whiteboard: [PDT+] no idea when this will land, no one has looked into this yet → [PDT+] Cannot be done for M14
Here is Kipp's initial justify work:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=kipp*&whotype=regexp&sortby=Date&hours=2&date=explicit&mindate=10%2F25%2F1998+00%3A00%3A00&maxdate=10%2F27%2F1998+09%3A00%3A00&cvsroot=%2Fcvsroot

It seemed to involve calculating a special wordspacing, and adding the extra
space to the whitespace glyph width. There was also a JustifyFrames method that
took care of this, but, that was probably replaced by
nsInlineFrame::AdjustFrameSize
A good place to start would probably be enabling the disabled code in
nsBlockFrame::PlaceLine, and seeing what happens.
Removing PDT+ and beta1 keyword, because this work cannot be done for M14. We 
also think it's not required for beta 1 that we support full justification
Keywords: beta1
Whiteboard: [PDT+] Cannot be done for M14 → Cannot be done for M14
mine! mine mine mine!  all mine!  whoo-hoo!
Assignee: kipp → buster
Note that the justification algorithm should handle the properities
word-spacing, letter-spacing, and white-space (and probably others...).
*** Bug 31056 has been marked as a duplicate of this bug. ***
*** Bug 31920 has been marked as a duplicate of this bug. ***
I've done some work on this, and I'm ready to try testing it. However, I don't 
have a compiler. If anyone wants to take a look, I've uploaded a copy here:

http://fantasai.tripod.com/Mozilla/TXJ/

So, if anyone gets this to compile, please tell me what happens? 
I'm rather curious to know what it will do.
*** Bug 34764 has been marked as a duplicate of this bug. ***
moving all buster m15 bugs to m16.
Target Milestone: M15 → M16
I have implemented this. I will attach the patch momentarily.

I got a few ideas from fantasai's patch (thanks!) but mine is rather different.
In particular, I've avoided adding any fields to nsTextFrame or any of the style
contexts. The basic idea is that nsLineLayout collects information from the text
frames during reflow; in particular it collects the number of spaces in each
text frame. It then uses this in HorizontalAlignFrames to distribute leftover
line space appropriately to each text frame. Justified text frames notice when
they are bigger than they need to be and distribute the extra space internally
in the correct manner.

There are probably some bugs. In fact, I found and fixed a number of other bugs
while I was doing this; they are marked "NOTE:" in the patch.

Regarding PRE: I treat nonbreaking spaces (which is what all PRE spaces turn
into) as regular spaces for the purposes of justification. Spaces derived from
tab expansion are also treated the same way. I don't know if this is "correct"
or not, but it works in that it makes justification useful in the context of PRE.

Word-spacing and letter-spacing work. It all even works in Composer (modulo a
few display glitches).

I have to say I don't think justification should do anything for PRE, since the
lines in PRE are broken by forced breaks, and forced breaks are not justified.
Turning off justification for preformatted text is as simple as making 
mJustifying depend on !mPreformatted, a one-line change to 
TextStyle::TextStyle(). Then such text frames will not report any spaces or 
letters to nsLineLayout, and will receive no extra space for justification.

One other thing I should mention --- single-word lines are currently not 
justified in any way. My patch prepares the way for using letter-spacing to 
stretch such words across the line, but I haven't actually put it in, because I 
think it looks ugly.
You're quite welcome! I'm glad it helped, at least. Can't really expect one's 
first C++ code to be perfect. ^o^ Actually, I still don't even know if mine 
works or not. -_-;;

I thought of giving Line Layout the number of spaces and characters during 
reflow, but I figured, why waste function calls when most text isn't justified 
anyway? Maybe that's an invalid argument. I wouldn't know.


--Preformatted text can be justified if it's wrapped (-moz-pre-wrap).

--If the author wishes to suppress character-spacing justification, they can 
simply specify 'letter-spacing: 0'. According to the CSS spec, the UA may only 
adjust the letter-spacing if it is set to 'normal' (the default).

--Single word lines (when 'letter-spacing' is set even if extending 
character-spacing is implemented) and lines without text need to be right 
justified if the text direction is rtl.


Oh, and you really should take up word-spacing issues in bug 1046. :)

um.. Did the priority change or is it just my imagination?
> I thought of giving Line Layout the number of spaces and characters during
> reflow, but I figured, why waste function calls when most text isn't justified
> anyway?

The impact of that is minimal. There should be basically no impact on the 
performance of laying out non-justified text. Laying out justified text is 
slower than normal text. It could be sped up a little by caching the value of 
each text frame's "justifcation extra space" somewhere, but that would require 
significant additional space (if you put it in nsTextFrame) or complexity (if 
you put it in an extension record in nsLineBox) --- not worth it in my opinion.

> --Preformatted text can be justified if it's wrapped (-moz-pre-wrap).

Is there a consensus on whether this should be justified? What's it used for? 
It's easy to tweak the patch to handle these cases in whichever way is desired.

RTL support is a good point. It's probably broken. How can I test that? There 
seem to be a lot of lurking RTL problems in this area, I'd rather not be the one 
to have to fix them all.

Browsing around, I have noticed that some sites look bad because the existing 
ShouldJustifyLine code is inadequate. E.g.
<p>Hello kitty<br>
gets justified. We can address that issue separately.

This bug has been P1 for a while, which is why I'm here.
-moz-pre-wrap is used for <PRE width="x">. It keeps whitespace intact, but 
allows wrapping. I don't see any reason why it shouldn't be justified, if full 
justification was specified on it.


As for RTL.. your ApplyFrameJustification returns an nscoord value. Subtract 
this from remainingWidth and delete the 'else' before the RTL check. I did the 
same thing in mine--see the code under "// Apply Justification" in txj.txt.


I didn't see any record for a priority change, so I was wondering..
If you're going to allow justify on PRE elements, I think you should probably
add a rule to ua.css so that PRE can't inherit 'text-align: justify', but it
would have to be specified explicitly.  I think generally justify goes against
the meaning of PRE.
I have updated the patch. It fixes the bug with BR-terminated lines; they are no 
longer justified. I have disabled justification for PRE and -moz-pre-wrap text; 
you can easily see where to turn this on again if desired. I have also brought 
the patch up to date with the tip. I also fixed an existing bug in nsTextFrame 
in the "MeasureTextRuns" code for Win32, where some lines were not being 
correctly wrapped. All the examples I've looked at, including all the examples 
from the duplicates of this bug, are looking good.

I tried some RTL tests, but the existing RTL handling already has bugs and it's 
hard to tell whether I've introduced any new ones.

I decided not to justify -moz-pre-wrapped. The reason is that trailing 
whitespace is not stripped from -moz-pre-wrapped text, so wrapped lines almost 
always have a trailing space. This means that justification will not produce a 
straight right margin. You could argue for stripping trailing whitespace from 
-moz-pre-wrapped text, but that's a separate issue.
Robert:  On first review, your new patch looks great.  I'll spend some time 
reviewing it in greater detail this afternoon.  If all looks good, I'll check it 
in early tomorrow morning.  Or, would you rather have the honors?  Makes no 
difference to me. Do you have cvs access rights?  If I do the checkin, it's 
slightly easier for me because I can merge your changes with my own for 12+ 
other bugs I've been holding and get it all in at once.

Many thanks to fantasia for the first cut, and as always to david for his 
insights.
I don't have CVS access, so you'll have to check it in.
I've done a thorough read-through, and it all looks great.  The only additions I 
plan on making are a few comments, a few assertions (checking for null pointers 
passed into new methods), and some cosmetic changes to bring it up to our coding 
standard (method arguments take the form "aArgName".)

In a second pass, it would be better if the justification-specific data items 
(mTextJustificationNumSpaces, etc.) were off in their own struct, and that 
struct was dynamically allocated only when needed.  This would save us 16 bytes 
per PerFrameData object in the case where justification isn't needed.  We can 
look into that as an optimization later.  similarly, I've already moved all the 
booleans in that data structure into a single PRInt32 bitfield, so I'll just 
go ahead and do the same for mIsBullet now.

I'll run some tests, and if it all is well and if the tree is in good shape 
tomorrow early am PST, it'll be in tomorrow's build.
Status: NEW → ASSIGNED
> method arguments take the form "aArgName"

Oops, thanks for reminding me.

I didn't realize the size of PerFrameData was important. They only exist while 
their line is being reflowed, right? Actually, those fields are costing 8 bytes 
per PerFrameData, and 8 bytes per nsLineLayout. (So having a dynamic structure 
within PerFrameData would only save 4 bytes for the non-justifying case.) It 
probably wouldn't be too hard to get to 0 bytes per PerFrameData and 16 bytes 
per nsLineLayout, by getting rid of ComputeJustificationWeights and keeping 
running totals of the total space and letter counts in nsLineLayout::PlaceFrame. 
Alternatively, I could get rid of all the space overhead if I could just call a 
method in nsTextFrame to fetch the space and letter counts during 
ComputeJustificationWeights, but I'm not sure how to do the cast in a totally 
safe way.
At some point, now that nsIFrame::AdjustFrameSize isn't going to be used, it 
should be deleted.

For optimization purposes, maybe we could add a method to nsIFrame that 
retrieves space and letter counts. The default implementation would return 0s, 
and nsTextFrame would override with the snippet that I added to 
nsTextFrame::Reflow. This would be called during the ComputeJustificationWeights 
traversal, and nothing would need to be recorded in the PerFrameData --- zero 
space overhead. It would also get rid of the need to correct overestimated space 
counts after whitespae trimming, because whitespace would already be trimmed by 
the time we compute the counts.

I'll try this out once the basic patch is in.
Keywords: patch
Most tests passed perfectly.  But I had to make a small change to your logic to 
get mail to run correctly.  I don't fully understand this, because the way you 
had the code looked fine. Basically, I had to revert a change you made, as shown 
below.  There are two adjacent code snippets marked with comments starting with
O'Callahan XXX:


      for (lastSegment = 0; textRun.mBreaks[lastSegment] < numCharsFit; 
lastSegment++) ;
      NS_ASSERTION(lastSegment < textRun.mNumSegments, "failed to find 
segment");
      // now we have textRun.mBreaks[lastSegment] >= numCharsFit
      /* O'Callahan XXX: This snippet together with the snippet below prevents 
mail from loading
         Justification seems to work just fine without these changes.
         We get into trouble in a case where lastSegment gets set to -1

      if (textRun.mBreaks[lastSegment] > numCharsFit) {
        // NOTE: this segment did not actually fit!
        lastSegment--;
      }
      */
    }

    /* O'Callahan XXX: This snippet together with the snippet above prevents 
mail from loading

    if (lastSegment < 0) {        
      // no segments fit
      break;
    } else */
    if (lastSegment == 0) {
      // Only one segment fit
      prevColumn = column;
      prevOffset = aTextData.mOffset;
    } else {


I plan on checking it in this way because it passes all my tests, plus the 
justification works fine.  Maybe you could spend a little time figuring out why 
your code creates some bad state that triggers an assertion in nsBlockFrame:

    NS_ASSERTION(aState.IsImpactedByFloater(),
                 "redo line on totally empty line");
OK. I made that change to resolve a rather obscure bug that I believe is exposed 
by (rather than caused by) justification. Assuming I can reproduce it after 
justification lands, I will file a separate bug and try to resolve the 
situation.

The bug involves some lines not being broken correctly. Basically, the Win32 
GetWidth using textRun will occasionally return a numCharsFit that is in the 
middle of a word. MeasureText simply can't handle that; it computes a value for 
"lastSegment" where the end of the line actually falls somewhere inside the 
segment, and then assumes that all the text in lastSegment will actually fit on 
the line, which is wrong. My changes simply try to do the right thing in this 
case.
fix checked in for Robert O'Callahan <roc+moz@cs.cmu.edu>.  Should be able to 
verify against 4/17/00 build.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Whiteboard: Cannot be done for M14
Looks good in 2000041708.

I can't reproduce the GetWidth-related bug I was seeing. Let's forget about it
for now :-).

I will file a bug regarding the excessive space consumption, and provide a patch
when I get around to it.

I will also file a bug regarding the minor display glitches in Composer. It is
probably a layout problem and might come up in non-Composer contexts (dynamic
content).

Tested on Windows, Mac and Linux. Windows and Linux work fine but justification 
on Mac is buggy. Separate bug written up for that. Verifying this bug fixed.
Status: RESOLVED → VERIFIED
Assignee: buster → roc
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: