1.55 KB, text/html
53.73 KB, image/png
54.09 KB, image/png
5.65 KB, patch
|Details | Diff | Splinter Review|
53.95 KB, image/png
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.
Keywords: css1, mozilla1.0, testcase
QA Contact: petersen → ian
Whiteboard: [Hixie-P2] (high profile: css1 test suite)
Urp. Did I regress this with my checkin for bug 46918?
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.9
Whiteboard: [Hixie-P2] (high profile: css1 test suite) → [Hixie-P2][CSS1-5.5.7] (high profile: css1 test suite)
Reassigning to Alex
Assignee: waterson → alexsavulov
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.9 → Future
Reconfirmed using FizzillaCFM/2002070913. Setting All/All and updating URL.
Assignee: alexsavulov → block-and-inline
Component: Layout → Layout: Block & Inline
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
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...
Also, setting a border-right-width has the same problem.
(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.
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.
Assignee: layout.block-and-inline → nobody
QA Contact: ian → layout.block-and-inline
8 years ago
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.
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
Related test in CSS 2.1 test suite: http://test.csswg.org/suites/css2.1/latest/html4/c5507-ipadn-r-003.htm
Created attachment 8434908 [details] 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.
Created attachment 8434909 [details] 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].
(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.)
Created attachment 8434911 [details] projecting padding when using the "simple" fix, as per comment 17
Created attachment 8434915 [details] [diff] [review] 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.
Attachment #8434915 - Flags: review?(roc)
Created attachment 8434916 [details] 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 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 :-)
Attachment #8434915 - Flags: review?(roc) → review-
(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.
You need to log in before you can comment on or make changes to this bug.