Last Comment Bug 191699 - Make white-space work on inlines
: Make white-space work on inlines
Status: VERIFIED FIXED
[patch]
: testcase
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.6beta
Assigned To: David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
: Hixie (not reading bugmail)
Mentors:
http://www.hixie.ch/tests/adhoc/css/t...
Depends on: reflow-refactor 343445 393096
Blocks: 178671 101565 184703 225946 261081 277771 332554
  Show dependency treegraph
 
Reported: 2003-02-02 20:01 PST by Hixie (not reading bugmail)
Modified: 2008-05-07 14:45 PDT (History)
12 users (show)
dbaron: blocking1.9+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
the beginning of a patch (54.99 KB, patch)
2003-04-02 19:33 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review
just the nsIFrame::IsEmpty cleanup (35.87 KB, patch)
2003-11-08 18:54 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
roc: review+
roc: superreview+
Details | Diff | Splinter Review
remove BRS_NOWRAP (5.51 KB, patch)
2003-11-10 17:12 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
roc: review+
roc: superreview+
Details | Diff | Splinter Review
nsLineLayout changes merged to trunk (15.30 KB, patch)
2003-11-11 11:39 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review
nsLineLayout changes merged to trunk (17.31 KB, patch)
2004-11-26 01:12 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review
nsLineLayout changes to make white-space apply to inlines (11.52 KB, patch)
2006-12-08 16:59 PST, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
no flags Details | Diff | Splinter Review

Description Hixie (not reading bugmail) 2003-02-02 20:01:30 PST
For CSS2.1 we need to make white-space work on all elements, even inlines.

Testcase:
   http://www.hixie.ch/tests/adhoc/css/text/white-space/mixed/001.html
Comment 1 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2003-04-02 19:33:51 PST
Created attachment 119252 [details] [diff] [review]
the beginning of a patch

This is the beginning of a patch.  The "max-element-size" calculation for
within lines, however, needs to be completely rewritten.  (It's currently in
nsLineLayout::VerticalAlignLine.)  The current code is just all wrong, and this
makes it slightly worse by not considering 'white-space' much.
Comment 2 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2003-11-08 18:54:16 PST
Created attachment 135094 [details] [diff] [review]
just the nsIFrame::IsEmpty cleanup

Considering the state of the current code (partially following 'white-space' on
inlines), this will probably slightly improve our behavior, and it's certainly
movement in the right direction.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-11-10 08:27:24 PST
Comment on attachment 135094 [details] [diff] [review]
just the nsIFrame::IsEmpty cleanup

Nice. Is the comment in nsIFrame correct? "logically empty, i.e., whether the
    * layout would be the same whether or not the frame is present." A
whitespace-only text frame with collapsing whitespace can still affect layout
because it collapses to one space. Right?
Comment 4 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2003-11-10 15:40:56 PST
Comment on attachment 135094 [details] [diff] [review]
just the nsIFrame::IsEmpty cleanup

Checked in, 2003-11-10 15:36 -0800, with improved comment in nsIFrame.h.
Comment 5 Boris Zbarsky [:bz] 2003-11-10 15:44:19 PST
> "Text frames containing only whitespace that does not contribute to the height
> of the line should return false."

Shouldn't that be "true"?
Comment 6 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2003-11-10 17:12:41 PST
Created attachment 135225 [details] [diff] [review]
remove BRS_NOWRAP

This just removes an invalid optimization once 'white-space' applies to inlines
(which we already partly do).  (Actually, I think I thought the whole larger
optimization is invalid.  I should probably remember what the issue was.)
Comment 7 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2003-11-10 17:13:58 PST
This just leaves the nsLineLayout changes from the original patch, which are the
ones that need work...
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-11-10 20:05:18 PST
Comment on attachment 135225 [details] [diff] [review]
remove BRS_NOWRAP

okey doke
Comment 9 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2003-11-11 11:39:34 PST
Created attachment 135263 [details] [diff] [review]
nsLineLayout changes merged to trunk
Comment 10 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2003-11-11 11:40:13 PST
Comment on attachment 135225 [details] [diff] [review]
remove BRS_NOWRAP

Fix checked in to trunk, 2003-11-11 11:24 -0800.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-29 18:04:03 PDT
This may have caused the regression in bug 232540.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-04-29 18:52:04 PDT
More likely it just exposed an existing bug :-)
Comment 13 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2004-11-26 01:12:15 PST
Created attachment 167095 [details] [diff] [review]
nsLineLayout changes merged to trunk
Comment 14 Adam Guthrie 2005-10-13 11:42:24 PDT
*** Bug 312315 has been marked as a duplicate of this bug. ***
Comment 15 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2006-12-08 16:59:14 PST
Created attachment 248028 [details] [diff] [review]
nsLineLayout changes to make white-space apply to inlines

There are still a bunch of trimming/collapsing issues before we pass Hixie's test, but this makes the result at least *resemble* the expected form of output.

I thought this through a bit, but I'd appreciate if you could take a good look at this to make sure it also makes sense to you.
Comment 16 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2006-12-08 17:01:18 PST
(It's the CanPlaceFrame changes that I think deserve some scrutiny.)
Comment 17 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2006-12-08 21:29:29 PST
I think substantial parts of Hixie's test are invalid, though.  I'll refer in this description to the parts of the test as being lines (1) through (8), where the word PASS appears on lines (4) (top) through (8) (bottom).  I think the test is invalid for the following reasons:

 * It requires that the user-agent break at one point where a break is forbidden.  In particular, this is the break that is supposed to separate lines (4) and (5).  Both Mozilla and Opera do not break here.  I believe this break is forbidden because at this break point (between the "x" before it and the "x" after it) there are two spaces:  the first (in an inner span) has 'white-space: nowrap'; the second (in an outer span that is a descendant of the div) has 'white-space: normal'.  According to CSS2.1, section 16.6.1, first ordered list, item 4.2, this second space (with 'white-space: normal') must be ignored.  Therefore there is only a space with 'white-space:nowrap', and that doesn't introduce a break opportunity.

 * Our other problem with the test is that we interpret the spaces with 'white-space:normal' preceding spaces with 'white-space:pre' in line (5) as breaking opportunities.  The latter are supposed to be treated as non-breaking spaces.  I need to have another look at UAX #14 and investigate Web-compatibility issues related to changing our handling of non-breaking spaces, or look into potentially treating these spaces as non-breaking spaces were supposed to be treated and not treating actual non-breaking spaces that way.  In any case, I don't think the behavior here is defined by CSS.
Comment 18 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2006-12-08 21:31:41 PST
Note that I think Opera 9's behavior on this test is probably the most correct behavior, although other behaviors may be valid.  I haven't checked its handling of the early part of line (5) carefully (i.e., I haven't counted spaces), though.
Comment 19 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2006-12-08 21:34:33 PST
(Note that I have a few additional changes in my tree in addition to the patch above -- I'm including those when referencing our behavior.  I need to figure out if any of them are actually helping anything -- perhaps even something not in this test -- once I land the patch above and the patch in bug 363232.)
Comment 20 Hixie (not reading bugmail) 2006-12-10 13:20:22 PST
I don't think a space being collapsed away should reduce its ability to introduce line breaking opportunities. In particular, I think two runs of spaces in <nobr> elements with a normal space in between them should be able to split across two lines. No?
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-12-10 16:20:03 PST
In the new textframe code I'm moving linebreak opportunity calculation to textrun construction time and making it work paragraph-at-a-time, and I'm handling white-space there at the same time because the linebreaking algorithm interacts with CSS 'white-space' in messy ways. In particular, line breaks due to whitespace have to be distinguished from line breaks not due to whitespace because they are controlled by CSS properties on different elements. E.g. given
<span class="A"><span class="B">X</span>Y</span>
If X and Y are a breakable CJK pair then A's white-space property governs, but if X is whitespace then B's white-space property governs.

Now that I'm merging in the reflow branch changes, I think I'll also move whitespace trimming to that phase, out of reflow, because min-width and pref-width calculation need that information too. That would make it easy to code up whatever policy we want here.

So do we really need to solve this bug now, or can it wait for new textframe?
Comment 22 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2006-12-10 16:36:20 PST
Ah, ok, it can wait.
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-14 20:50:24 PDT
I think this works fine now thanks to new-textframe.
Comment 24 Samuel Sidler (old account; do not CC) 2008-05-07 14:45:53 PDT
I'm going to call this verified, with a note that the testcase in the URL field no longer displays properly due to bug 393096. See bug 393096 comment 27 through 33.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre

Note You need to log in before you can comment on or make changes to this bug.