{inc} dynamic changes to br clear don't reflow enough

ASSIGNED
Assigned to

Status

()

Core
Layout: Block and Inline
ASSIGNED
11 years ago
10 years ago

People

(Reporter: Eli Friedman, Assigned: Eli Friedman)

Tracking

(Blocks: 1 bug)

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 obsolete attachments)

(Assignee)

Description

11 years ago
See testcase in URL; changing the selection in the dropdown should take effect immediately, but it doesn't do anything until the window is resized.
(Assignee)

Comment 1

11 years ago
Note: fix possibly at  http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsBlockFrame.cpp&rev=3.835&mark=1698#1698

The issue is that when the br's clear state changes, the clearance for the next line needs to be recalculated.  It's slightly complicated to fix, though, because currently there's no way to tell if a line was previously impacted by clearance.
(Assignee)

Comment 2

11 years ago
Created attachment 270377 [details] [diff] [review]
Patch
Assignee: nobody → sharparrow1
Status: NEW → ASSIGNED
Attachment #270377 - Flags: review?(roc)
(Assignee)

Comment 3

11 years ago
Comment on attachment 270377 [details] [diff] [review]
Patch

Bleh, this isn't right; I need to study this a bit more.
Attachment #270377 - Flags: review?(roc)
(Assignee)

Comment 4

11 years ago
Created attachment 270408 [details] [diff] [review]
Patch v2

Hmm, I guess this patch isn't especially useful, but it was a good learning experience.
Attachment #270377 - Attachment is obsolete: true
Attachment #270408 - Flags: review?(roc)
+        NS_NOTREACHED("This is impossible: it would require a block line and"
+                      "an inline line with content to be children of the"
+                      "same block");

Why do you think this is impossible? It's completely possible for an nsBlockFrame to have both block and inline children. The CSS spec talks about anonymous block wrappers for inlines but we don't implement it that way.

The other hunk of the patch looks OK, but can you explain why it's needed --- in a comment in the code? Superficially it seems to me that the line after the BR can move due to clearance changes but lines after it should be handled just fine by the regular reflow path's line-sliding.
(Assignee)

Comment 6

11 years ago
(In reply to comment #5)
> +        NS_NOTREACHED("This is impossible: it would require a block line and"
> +                      "an inline line with content to be children of the"
> +                      "same block");
> 
> Why do you think this is impossible? It's completely possible for an
> nsBlockFrame to have both block and inline children. The CSS spec talks about
> anonymous block wrappers for inlines but we don't implement it that way.

Oops; I don't know what I was thinking.

> The other hunk of the patch looks OK, but can you explain why it's needed ---
> in a comment in the code? Superficially it seems to me that the line after the
> BR can move due to clearance changes but lines after it should be handled just
> fine by the regular reflow path's line-sliding.

The issue is that if we don't check, we don't notice the case where a previous line had an inline clear style, but doesn't anymore.

This patch is broken anyway; sorry about that. I need to figure out if there's a better way to do this.
(Assignee)

Updated

11 years ago
Attachment #270408 - Attachment is obsolete: true
Attachment #270408 - Flags: review?(roc)
(Assignee)

Comment 7

11 years ago
Bleh, this is really confusing.  Too many different situations in which the logic needs to trigger and no easy way to check all those conditions.

Checking inlineFloatBreakType != NS_STYLE_CLEAR_NONE is sufficient for most cases; however, it is not sufficient for cases when the clear style on the br changes to none.  Checking aState.mY != line->mBounds.y + deltaY is sufficient, but it catches cases where we don't need to clear (I'm not sure exactly which cases, though; probably something to do with margins.).  The check from my patch is not sufficient because it doesn't handle blocks.
I think we can go with checking aState.mY != line->mBounds.y + deltaY.
(Assignee)

Comment 9

11 years ago
(In reply to comment #8)
> I think we can go with checking aState.mY != line->mBounds.y + deltaY.

That isn't correct; margins completely mess this up.
(Assignee)

Updated

10 years ago
Blocks: 429976
You need to log in before you can comment on or make changes to this bug.