3490 - [INLINE-H] line-breaking bug caused by -ve margin-right on inline elements [LINE] {ll}

Status: VERIFIED FIXED

(Core :: Layout, defect, P4)

Description

[ekrock's old account (dead)]

This text:

This element is unstyled save for a background color of gray.. It contains an
inline element of class two, giving it an aqua background and a -10px margin

looked like this:

Page margin | This element is unstyled save for a background
            | color of gray.. It contains an
        inline element of class two, giving it an aqua background
            | and a -10px margin

This was with browser window width set to 2/5 screen width.

Comment 1

[ekrock's old account (dead)]

Oh yeah, Seamonkey 3/2 viewer.

Comment 2

[kipp]

Some how the content in this page triggers an interesting line-breaking bug.
depending on the width of the window either the line breaks before the span with
the negative left margin or not...

Comment 3

[kipp]

*** Bug 5469 has been marked as a duplicate of this bug. ***

Comment 4

[leger]

Updating to default Layout Assignee...kipp no longer with us :-(

Comment 5

[troy]

Why are you re-reassing layout bugs? Do NOT touch layout bugs.

The bugs are assigned to Kipp so they can stay neatly organized until we have a
new owner for the block/inline code.

Comment 6

[buster]

mass moving all Kipp's pre-beta bugs to M15.  Nisheeth and I will
prioritize these and selectively move high-priority bugs into M13 and M14.

Comment 7

[Hixie (not reading bugmail)]

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

Comment 8

[buster]

kipp is very unlikely to fix these, since he's not working ont he project any 
longer.  so I'll take a look.

Comment 9

[rickg]

moving all buster m15 bugs to m16.

Comment 10

[buster]

this is at risk for FCS.  very bad, since it's a CSS1 compliance issue

Comment 11

[Hixie (not reading bugmail)]

Note, BTW, that this is a _bug_ and not merely a lack of support for the spec.
IIRC, what we are doing is rather bad/buggy/broken, as opposed to just not what
it says in the spec.

Comment 12

[buster]

redistributing bugs across future milestones, sorry for the spam

Comment 13

[ekrock's old account (dead)]

Nom. nsbeta2, recc. nsbeta2 6/22 with fall-through to nsbeta3+ if not fixed. 
CSS1 non-compliance problem that's showing up on official W3C test suite. Also 
per Ian's comments this is a situation where we break the standard and possible 
render that feature unusable, rather than just displaying in an ugly way.

Comment 14

[Phil Peterson]

[nsbeta2-] since Steve Clark says this is less important than the [P-Margin] 
bugs he's looking at right now.

Comment 15

[buster]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another 
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration. 

Comment 16

[Hixie (not reading bugmail)]

Considering this bug is marked nsbeta2-, with a recommendation of nsbeta3+ from
Eric Krock, are we to assume that this bug being marked Future is an error?

This bug occurs on the Official CSS1 Test Suite too, so is quite high-profile.

Comment 17

[Hixie (not reading bugmail)]

Attachment: Screenshot showing problem and explanation in build 2000061320

Comment 18

[buster]

brought back from the dead

Comment 19

[ekrock's old account (dead)]

Nominating nsbeta2,nsbeta3,rtm. Recc. nsbeta2+, falling through to nsbeta3+ and 
rtm+ if necessary. This is a W3C CSS1 Official Test Suite compliance bug.

Comment 20

[ekrock's old account (dead)]

Sorry, to clarify: this is not an nsbeta2 stopper, but I'd permit checking in 
prior to nsbeta2 if we have a fix. Recc. nsbeta2+[some lenient date-], falling 
through to nsbeta3+ hard stop if not fixed during nsbeta2. This is a W3C CSS1 
Official Test Suite compliance bug. Clearing [nsbeta2-] to trigger 
re-evaluation.

Comment 21

[leger]

Putting on [nsbeta2-] radar. Not critical to beta2. 

Comment 22

[Christine Hoffman]

Ian: has this been fixed? It looks okay using 7_27_04 build on Win 98.

Comment 23

[Hixie (not reading bugmail)]

It doesn't look ok on Win2000 commerical build 6.0.17.2000072811.
It looks exactly the same as shown in the screenshot attached.

Comment 24

[Hixie (not reading bugmail)]

ChrisD found something important: The problem only occurs if the RIGHT margin
(in left-to-right flowing and left aligned text) is negative. The other margins
are irrelevant.

Does this help in the fixing?

Comment 25

[Marc Attinasi]

Approving for beta3: high-profile compliance test site.

Comment 26

[Hixie (not reading bugmail)]

See also related bug 46918, which shows a mundane but more serious problem with
positive right margins on inline elements.

Comment 27

[Chris Waterson]

I'll take in hopes that it might be related to the other bug.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the exact condition when this bug happened is when the line would be
broken a word earlier if the -10px margin would not there.  As you gradually
shrink the window from a wide width, you can see that every time you get to a
point where you should knock one word down to the next line, it knocks down the
whole beginning of the inline element, for about 10 px (I think).

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: in case the previous screenshot wasn't clear enough

Comment 30

[Chris Waterson]

The problem starts here:

http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsLineLayout.cpp#1167

Specifically, we subtract from the space available on the line *both* the left
and right margins. We can't correctly subtract the right margin until we've
decided to fit the entire span onto a single line.

With negative margins set, this causes us to over-estimate the amount of space
available on the line. We panic later in nsLineLayout::CanPlaceFrame() when we
realize that we can't fit the entire span onto the line:

http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsLineLayout.cpp#1270

...which forces the span down to the next line.

Comment 31

[Chris Waterson]

Attachment: half-assed fix

Comment 32

[Chris Waterson]

The above fix makes it so that the right margin does not come into play in
computing the amound of space left on the line. This means that we'll botch some
cases with *positive* margins on inline elements. For example, the second
paragraph in the test case I'm about to attach (when displayed at 620x400,
"viewer's" default size).

Comment 33

[Chris Waterson]

Attachment: test case that illustrates problems at 620x400 in viewer

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Although I can't see the new problem, it sounds worse than the old...

Comment 35

[ekrock's old account (dead)]

In general, I think that the usage of positive margin values will be >> 10x more 
frequent than the usage of negative margin values, so I'd be very reluctant to 
take a fix that improved negative margin handling at the cost of messing up 
positive margin handling in some cases.

Comment 36

[Phil Peterson]

PDT thinks this is a P4

Comment 37

[Jason Eager]

We know exactly where this bug is being triggered, right? All we need is a
better algorithm.

Comment 38

[Chris Waterson]

next train...

Comment 39

[Chris Waterson]

Attachment: work-in-progress for a fix

Comment 40

[Chris Waterson]

Started looking at the problems with inline elements and margins, padding, and
borders. The way it's been written works only if the inline box never needs to
split: the margins, borders, and padding are all subtracted from the "available
width" that's given to the child frame.

The above patch is work-in-progress: since nsTextFrame's are the only frames
that can be split, I tried adding some special case logic that allows the
available width be the the computed available width, but with the right margins,
borders, and padding added back from all the containing inline frames. This
logic is used *unless* the frame is the last unsplittable text. (One of the
problems with this patch is that I don't figure that out quite right currently!)

Comment 41

[Chris Waterson]

Attachment: oops, forgot a file. try this patch instead.

Comment 42

[Chris Petersen]

Requesting a triage on this bug. It's nominated but doesn't have a - or +. Are
we going to fix this for rtm ?

Comment 43

[Hixie (not reading bugmail)]

waterson: if you're at a loss of what to fix next, I would recommend looking at 
this bug and its sister, bug 46918. :-)

Comment 44

[Madhur Bhatia]

I do not see this problem anymore on win98 buildID: 2002-01-02-06trunk.

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I still see it.

Comment 46

[Chris Waterson]

Should be fixed along with bug 46918.

Comment 47

[Hixie (not reading bugmail)]

VERIFIED FIXED.