Closed Bug 480452 Opened 16 years ago Closed 16 years ago

[FIX]GetSkipSides on inlines should know about {ib} splits

Categories

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

Product:
Core
Component:
Layout: Block and Inline
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

Proposed patch
19.21 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
We don't seem to have a bug on this... In an {ib} split, the first inline shouldn't have right border/padding, and the last one should not have left border/padding. Except the issue is complicated by bidi and the like... In the absence of bidi, I think I could implement this as follows: treat an inline as not left-most if it's the third part of an {ib} split, and treat it as not right-most of its first continuation is the first part of an {ib} split. This is unfortunately somewhat expensive for the right-most check... not sure what I can do to improve that. With bidi, though, I'm not even quite sure what the right behavior is, much less what it means in terms of our bidi flags, etc. Testcases: data:text/html,<!DOCTYPE html><span style="padding: 0 20px; background: green; border: 2px solid red">aaa<div>bbb</div></span> data:text/html,<!DOCTYPE html><span style="padding: 0 20px; background: green; border: 2px solid red">aaa<div>bbb</div>cc</span> data:text/html,<!DOCTYPE html><html style="direction: rtl"><span style="padding: 0 20px; background: green; border: 2px solid red">aaa<div>bbb</div></span> data:text/html,<!DOCTYPE html><html style="direction: rtl"><span style="padding: 0 20px; background: green; border: 2px solid red">aaa<div>bbb</div>cc</span> Opera's behavior when in rtl mode is pretty weird wrt paddings in general, but they do seem to get the borders right.
This causes issues on msn.com, apparently. See bug 481769 comment 10.
Blocks: 480323
Attached patch Proposed patch — — Splinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #372709 - Flags: superreview?(dbaron)
Attachment #372709 - Flags: review?(dbaron)
Summary: GetSkipSides on inlines should know about {ib} splits → [FIX]GetSkipSides on inlines should know about {ib} splits
Comment on attachment 372709 [details] [diff] [review] Proposed patch >@@ -552,22 +555,32 @@ nsInlineFrame::ReflowFrames(nsPresContex >+ // Make sure to not include our end border and padding if we're not complete, >+ // or if we have a non-fluid continuation, or if we're in the first part of >+ // an {ib} split. >+ if (NS_FRAME_IS_COMPLETE(aStatus) && >+ (!GetNextContinuation() || GetNextInFlow()) && >+ !nsLayoutUtils::FrameIsInFirstPartOfIBSplit(this)) { > aMetrics.width += ltr ? aReflowState.mComputedBorderPadding.right > : aReflowState.mComputedBorderPadding.left; >@@ -1151,20 +1156,24 @@ nsLineLayout::CanPlaceFrame(PerFrameData >+ if ((NS_FRAME_IS_NOT_COMPLETE(aStatus) || >+ (pfd->mFrame->GetNextContinuation() && !pfd->mFrame->GetNextInFlow()) || >+ nsLayoutUtils::FrameIsInFirstPartOfIBSplit(pfd->mFrame)) > && !pfd->GetFlag(PFD_ISLETTERFRAME)) { >- // Only apply end margin for the last-in-flow. Zero this out so >- // that when we compute the max-element-width of the frame we >+ // Only apply end margin for the last continuation (so don't apply it for >+ // incomplete frames or frames which have a non-fluid continuation), and >+ // only in cases when it's not in the first part of an {ib} split. Zero >+ // this out so that when we compute the max-element-width of the frame we > // will properly avoid adding in the end margin. > if (ltr) > pfd->mMargin.right = 0; > else > pfd->mMargin.left = 0; It took me a little while to understand these (existing) checks, and I don't think your comments explain them adequately. It seems to me that what's going on here is that we want to avoid applying the end-margin/border+padding when this isn't going to be the last piece of the frame, but we're at a state where continuations that were lying around from a previous layout involving more continuations haven't been destroyed yet. If that's also your understanding, could you: (1) make the comments reflect it better, and (2) file a followup bug on the case where that undestroyed fluid continuation itself has a fixed continuation (which means that we actually shouldn't apply the margin) r+sr=dbaron with that. Sorry for the delay.
Attachment #372709 - Flags: superreview?(dbaron)
Attachment #372709 - Flags: superreview+
Attachment #372709 - Flags: review?(dbaron)
Attachment #372709 - Flags: review+
> If that's also your understanding It is, yes. Sorry for not documenting it once I figured it out. Fixed the comments and filed bug 492469. You might want to look the comments over. The line layout comment: /* * We want to only apply the end margin if we're the last continuation and * not in the first part of an {ib} split. In all other cases we want to * zero it out. That means zeroing it out if any of these conditions hold: * 1) The frame is not complete (in this case it will get a next-in-flow) * 2) The frame is complete but has a non-fluid continuation. Note that if * it has a fluid continuation, that continuation will get destroyed * later, so we don't want to drop the end-margin in that case. * // FIXME: bug 492469 * 3) The frame is in the first part of an {ib} split. * * However, none of that applies if this is a letter frame (XXXbz why?) */ The inline frame comment: /* * We want to only apply the end border and padding if we're the last * continuation and not in the first part of an {ib} split. To be the last * continuation we have to be complete (so that we won't get a next-in-flow) * and have no non-fluid continuations. * * FIXME the check for non-fluid continuations is not quite correct in the * code (though the comment above describes it correctly); see bug 492469. */
Pushed http://hg.mozilla.org/mozilla-central/rev/de33f2e0b22f
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
And pushed http://hg.mozilla.org/mozilla-central/rev/57a720902b66 to fix resulting test orange.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: