122795 - [INLINE-H] line-breaking bug caused by +ve padding-right on inline elements [LINE] {ll}

Status: NEW

(Core :: Layout: Block and Inline, defect, P3)

Description

[Hixie (not reading bugmail)]

STEPS TO REPRODUCE
   See section 2 paragraphs 2 and 3 of:
      http://www.webstandards.org/css/macie/inlinedemo1.html

   Basically, end padding on an inline element is being treated as an end margin 
   on every line.

This hopefully will be easily fixed using ideas from bug 46918.

Comment 1

[Chris Waterson]

Urp. Did I regress this with my checkin for bug 46918?

Comment 2

[Kevin McCluskey (gone)]

Reassigning to Alex

Comment 3

[Greg K.]

Reconfirmed using FizzillaCFM/2002070913. Setting All/All and updating URL.

Comment 4

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

->Layout: Block&Inline

Comment 5

[Loek]

This bug still exists in FF2.0
To be precise, padding-right on an inline element adds both the inline padding on the last line, and the same amount of margin-right.

See http://www.altereco.nl/paddingbug-ff2.html

Comment 7

[Boris Zbarsky [:bzbarsky]]

So what's the expected rendering when you have, say, 4em of text that can break at the 2em mark, 3em of right padding, and a total width of 5 em?  I assume in that situation we'd want to break the text at the 2em mark, right?

Also note that bug 386604 has some testcases (using padding and nbsp) that could be used as a reftest for this...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Also, setting a border-right-width has the same problem.

Comment 9

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

(In reply to comment #7)
> So what's the expected rendering when you have, say, 4em of text that can break
> at the 2em mark, 3em of right padding, and a total width of 5 em?  I assume in
> that situation we'd want to break the text at the 2em mark, right?

Yes.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This could be fixed thanks to the backup-and-retry line reflow logic we have now. We'd just stop subtracting inline padding and border-width from the available width.

Comment 11

[Peter Bright]

Who do I have to bribe to get this fixed? Or even _confirmed_! It's _still_ a problem. Gecko _still_ adds weird right padding (though it doesn't extend the background into the fake padding, so it's more like a kind of padding/margin hybrid) to inline elements when they break over a line, and it's still not correct.

It's a bug for CSS 1: http://www.w3.org/TR/CSS1/#inline-elements
It's a bug for CSS 2: http://www.w3.org/TR/CSS2/visuren.html#inline-formatting

http://dl.dropbox.com/u/4619191/firefox-getting-it-wrong.png

Padding is being applied at each and every linebreak.

Comment 12

[Gérard Talbot]

Reduced test based on the original www.webstandards.org/css/macie/inlinedemo1.html

http://www.gtalbot.org/BrowserBugsSection/css21testsuite/Bug-122795-padding-right-on-inline-elements.html

Comment 13

[Gérard Talbot]

Related test in CSS 2.1 test suite:
http://test.csswg.org/suites/css2.1/latest/html4/c5507-ipadn-r-003.htm

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: testcase for experimenting with padding on inline element

Here's a testcase I've been using to play with this a bit; it allows you to dynamically vary the block width and the padding on a simple inline <span>, to see how line-breaking behaves. The example with padding-left works as expected; padding-right shows clear breakage.

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: bad line-breaking caused by padding-right on inline element

Screenshot showing an example of bad line-breaking as demonstrated with the testcase from attachment 8434908 [details].

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> This could be fixed thanks to the backup-and-retry line reflow logic we have
> now. We'd just stop subtracting inline padding and border-width from the
> available width.

I've been poking at this a bit. Simply removing the addition of the end padding at
  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsInlineFrame.cpp#512
does improve things substantially, but it's not quite sufficient.

We get good line-breaks within the bulk of the <span> that has the padding, but if it ends near the right margin (such that there isn't room for its padding to fit), it fails to take the last potential breakpoint within the span, and instead the padding will project into the margin. I think that's probably wrong: the padding at the end of the span is, in effect, an addition to the width of the last character, and so if it doesn't fit, that character also doesn't fit, and we need to take an earlier breakpoint. (See following screenshot.)

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: projecting padding when using the "simple" fix, as per comment 17

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: possible fix for line-breaking with padding-right on inline element

Here's an attempt to go a bit further, and fix the "projecting padding" described above by backtracking if we find that we're trying to place a frame when we've already overflowed the line width. This behaves nicely with the testcase above, and passes existing unit tests on try: https://tbpl.mozilla.org/?tree=Try&rev=2c6ccdf06cad.

I'm pretty sure there will still be edge cases that are less than perfect, but this seems a substantial improvement on our current behavior.

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: screenshot showing fixed line-breaking

Here's the result of the above patch, using the same test as shown in attachment  8434911 [details] with the partial fix; this time, we get all the correct line-breaks.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8434915 [details] [diff] [review]
possible fix for line-breaking with padding-right on inline element

Review of attachment 8434915 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a little confused by this patch. In your testcase, what is the zero-width frame for which CanPlaceFrame is incorrectly returning true?

::: layout/generic/nsLineLayout.h
@@ +537,5 @@
>                         bool aCanRollBackBeforeFrame,
>                         nsHTMLReflowMetrics& aMetrics,
>                         nsReflowStatus& aStatus,
> +                       bool* aOptionalBreakAfterFits,
> +                       bool aAlreadyOverflowed);

Document aAlreadyOverflowed? Maybe the other parameters too if you feel up to it :-)

Comment 22

[Jonathan Kew [:jfkthame]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)

> I'm a little confused by this patch. In your testcase, what is the
> zero-width frame for which CanPlaceFrame is incorrectly returning true?

The text following the <span> with the padding starts with a space. There's a possible line-break after this initial space (before "world"), which gives us a frame for the single space character - which is then trimmed, so the frame ends up zero-width. But at this point the padding from the <span> has pushed us out beyond the allowed width.

That's how the case shown in attachment 8434911 [details] arises, if we only make the change in nsInlineFrame.cpp.

Comment 23

[Ryan Hales]

Attachment: same problem

Thanks from saviltiosup1971.