Closed Bug 134706 Opened 18 years ago Closed 12 years ago

{inc}[FLOAT]Table is over-covered


(Core :: Layout, defect, P3)






(Reporter: kevin.breit, Assigned: dbaron)


(Depends on 5 open bugs)


(Keywords: testcase, top100)


(6 files, 3 obsolete files)

I noticed that the search results overlap with the Download Center table at right.
does your Mozilla have a build ID?  please report it.

worksforme with linux build 20020401.  probably a dupe of 129350.
I see the bug with builds up to 20020329, so not a dupe of bug 129350.  but
build 20020401 works fine. 
Confirming bug, 2002-04-02-11 on Linux.
Assignee: asa → attinasi
Component: Browser-General → Layout
Ever confirmed: true
QA Contact: doronr → petersen
Priority: -- → P2
Target Milestone: --- → Future
win98 2002040503, I see this behaviour but a local copy of the page is displayed
See also bug 120107 (fixed)
Keywords: top100
OS: Linux → All
Hardware: PC → All
*** Bug 146686 has been marked as a duplicate of this bug. ***
Taking. amar: could you try to minimize if you've got time?
Assignee: attinasi → waterson
Keywords: qawanted
Whiteboard: [testcase]
Definitely an incremental reflow problem. Resizing the window gets rid of the
overlap. Probably a floater problem, given the right menu.
Summary: Table is over-covered → {inc}[FLOAT]Table is over-covered
 Its happening on todays branch build too. I dont see the two horozontal lines
overlapping when I loaded the source from my local drive. I see the text overlaping.
Working on minimising further.
I tried to reduce this testcase further but even if I take out one <td> I
could not reproduce the problem.
 need to refresh the above testcase to reproduce the problem.
Keywords: testcase
*** Bug 148644 has been marked as a duplicate of this bug. ***
*** Bug 151997 has been marked as a duplicate of this bug. ***
*** Bug 158489 has been marked as a duplicate of this bug. ***
*** Bug 137095 has been marked as a duplicate of this bug. ***
*** Bug 155301 has been marked as a duplicate of this bug. ***
*** Bug 167819 has been marked as a duplicate of this bug. ***
i think, there has to be mentioned that the window should be shrinked to see the
table overlapped by the results in the testcase
i do not se any overlap problem with the 05/05/2003 trunk build on xinXP and linux. 
Could this issue have gotten fixed with some checkin ?
Both Testcases WFM using Mozilla Windows Nightly 2005012804 on Windows 2000. 
Desirable layout achieved regardless of window width.
Broken in firefox 1.0.3 WXP 
Though the microsoft page contains something that is wide and stops shrinking,
the testcase can be broken by displaying in a very narrow window.
As the window is shrinked nearly to the width of the table in the right, the
text is drawn over it.
Top testcase, 

Bizarrely, with:
- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/2006120606 Minefield/3.0a (pre-reflow branch)
The text does not overwrite the right hand side column.

But with
- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/2006120804 Minefield/3.0a1 (post-reflow branch)
The text does overwrite the right hand side column.

with current trunk, the TABLE and DIV overlap (and shouldn't)

I narrowed down attachment 88738 [details] with a post-reflow branch to this, which looks like bug 14984, although this testcase worked fine before the reflow branch landed.
Assignee: waterson → nobody
QA Contact: chrispetersen → layout
shouldn't this block bug 300030 ?
Depends on: 14984
Keywords: qawanted
Whiteboard: [testcase]
This is a top100 issue and IE7 renders the "post-reflow branch" testcase correctly. Is this worth examining for 1.9 still?

Also, I'm removing the URL since it no longer points the version of the MS site search shown in the testcases.
Flags: blocking1.9? → blocking1.9+
Priority: P2 → P3
Here are how various browsers handle this testcase:

IE and Opera: 
 - Add linebreak between floated div and the inner "some text" table.

WebKit (Safari 3.04) and Firefox 2:
 - No added linebreak
 - Outer table is made wide enough to fit the full string "x y zsome text"
 - No overlapping text

Firerfox 3:
 - No added linebreak
 - Outer table is not stretched -- it's just as wide as the string "some text"
 - 'float:left' has no overlap, but "text" is pushed outside the outer table
 - 'float:right' has "text" overlapping "x y z"
Daniel, hoping you can take this.
Assignee: nobody → dholbert
I think we should match IE and Opera here.

I'll actually take this one...
Assignee: dholbert → dbaron
So what needs to happen here is that in or around nsBlockFrame::ReflowBlockFrame we need to clear anything that won't fit, because its min-width for container is wider than the available space, into the next band, in any case where nsBlockReflowState::ComputeBlockAvailSpace returns a width narrower than mContentArea.width.  (Although I'm not entirely sure about the float-edge cases.)

The relevant section of the CSS spec is in which says:

# The border box of a table, a block-level replaced element, or an element in
# the normal flow that establishes a new block formatting context (such as an
# element with 'overflow' other than 'visible') must not overlap any floats in
# the same block formatting context as the element itself. If necessary,
# implementations should clear the said element by placing it below any
# preceding floats, but may place it adjacent to such floats if there is
# sufficient space. They may even make the border box of said element narrower
# than defined by section 10.3.3. CSS2 does not define when a UA may put said
# element next to the float or by how much said element may become narrower. 

ComputeBlockAvailSpace implements the shrinking, but we need to implement the placing below bit.  We probably want to implement it as though it were a 'clear' (in terms of all the interactions with margin collapsing), but unlike 'clear', we need to do it one spacemanager band at a time, since we may need to clear past only some of the floats rather than all of them.

I need to think a bit more about the cleanest way to do this.

I should also probably pay attention to not implementing bug 25888 for the block-wrapping around floats (a bug that we currently have for inline-wrapping around floats).  However, that might be too much to bite off at once.
Note that I'm saying that (except for not having bug 25888 for this code) I believe we can determine whether we need this clearing before reflow, using nsLayoutUtils::IntrinsicForContainer, so that we don't need to set up a new multi-pass reflow loop (over bands) for this case.  I'm still not sure how I want to handle the later-floats issue.
Attached patch work in progress (obsolete) — Splinter Review
Here's where I am so far on the patch.  This shows the approach I'm taking, but I still need to work on the "XXX" comments and write a lot of tests.
A bit of further analysis of this removed chunk from attachment 298865 [details] [diff] [review]:
>-  // text controls are not splittable
>-  // XXXldb Why not just set the frame state bit?
>-  nsSplittableType splitType = aFrame->GetSplittableType();
>-  if ((NS_FRAME_SPLITTABLE_NON_RECTANGULAR == splitType ||     // normal blocks 
>-       NS_FRAME_NOT_SPLITTABLE == splitType) &&                // things like images mapped to display: block
>-      !(aFrame->IsFrameOfType(nsIFrame::eReplaced)) &&         // but not replaced elements
>-      aFrame->GetType() != nsGkAtoms::scrollFrame)         // or scroll frames
>-  {

I wrote this ocaml program to print the frame classes for which this condition would return true, and the full list is:


of these, the only ones that I think could sensibly have display:block and be the child of a block are nsIsIndexFrame, nsBlockFrame, and nsAreaFrame, and I think those are the three frame classes that the condition I'm replacing it with should return true for.  Or close enough, anyway.
Blocks: 349255
Attached patch patch (obsolete) — Splinter Review
I think this is ready for review, although I still need to write some more tests:  in particular, I need to cover interactions with margin collapsing and incremental reflow.  However, those are the areas where what I did most closely fits in to existing code, so I'm less worried about problems.  (I'd be highly worried about them regressing if this patch were ever redesigned, though, so I do want tests.  And I may yet find bugs when writing them...)

The basic idea of what's changed here (described in a bit of detail since, although the changes are relatively small, they're sort of on top of each other):

 * rename two variables in nsBlockFrame.cpp to avoid -Wshadow warnings

 * add an eBlockFrame IsFrameOfType argument (something we can replace QueryInterface to kBlockFrameCID with)

 * slightly change (but not for any important cases; see previous comment and the comment in the code) the condition used in nsBlockReflowState::ComputeBlockAvailSpace uses to determine whether a child block's width should depend on the position of floats and refactor the decision into nsBlockFrame::BlockCanIntersectFloats.

 * add code to nsBlockFrame::ReflowBlockFrame (used to reflow a block child) and nsBlockFrame::ReflowDirtyLines to consider clearing of block children that cannot intersect floats, based on their width, as an additional type of clearing, and appropriate calls to nsBlockFrame::BlockCanIntersectFloats (see above) and the new function nsBlockFrame::WidthToClearPastFloats in order to pass that width through to nsBlockReflowState::ClearFloats

 * added code to nsBlockReflowState::ClearFloats, modeled on code in nsBlockFrame::DoReflowInlineFrames (to handle pushing lines down in the case where the first unbreakable unit on a line doesn't fit next to floats) to do the clearing of these blocks.  I made it not call nsSpaceManager::ClearFloats when the float break type is NS_STYLE_CLEAR_NONE, since it used to not be called at all in that case.
Attachment #298865 - Attachment is obsolete: true
Attachment #299497 - Flags: superreview?(roc)
Attachment #299497 - Flags: review?(roc)
Attached patch patch (obsolete) — Splinter Review
I had some mistakes in the table+caption handling:  both triggering assertions and using the wrong width.  This fixes that and adds some tests for it; I still need to write the other tests I mentioned.
Attachment #299497 - Attachment is obsolete: true
Attachment #299851 - Flags: superreview?(roc)
Attachment #299851 - Flags: review?(roc)
Attachment #299497 - Flags: superreview?(roc)
Attachment #299497 - Flags: review?(roc)
+  if (aFrame->GetStylePosition()->mWidth.GetUnit() == eStyleUnit_Percent) {

Cleaner to invert this test and make the early return short.

Why doesn't min-size computation work for the table outer frame?
min-size computation works fine for the table outer frame, but either the table or the caption could independently have a % width.
Comment on attachment 299851 [details] [diff] [review]

okay then
Attachment #299851 - Flags: superreview?(roc)
Attachment #299851 - Flags: superreview+
Attachment #299851 - Flags: review?(roc)
Attachment #299851 - Flags: review+
(In reply to comment #37)
> +  if (aFrame->GetStylePosition()->mWidth.GetUnit() == eStyleUnit_Percent) {
> Cleaner to invert this test and make the early return short.

I actually prefer it the other way, since I like the unusual case being the early return and the normal case going to the end of the function...
Attached patch patchSplinter Review
Here's what I'm going to land.  The changes from the previous patch are, I think:
 * adding reftest 134706-7, to test margin collapsing interactions a bit
 * adding bug number and fixing reftest manifest syntax for the failing test

I'm not quite sure how to add an incremental layout test, in particular because I'm not really sure which cases to test.  I guess I'm thinking that that's better tested by fuzzing our existing tests than by adding tests for specific cases.  (And I think Jesse's been doing this.)
Attachment #299851 - Attachment is obsolete: true
Attachment #299955 - Attachment is patch: true
Attachment #299955 - Attachment mime type: application/octet-stream → text/plain
Checked in to trunk.
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 414558
I think this caused bug 414558
Depends on: 415252
Depends on: 417914
Depends on: 417178
Depends on: 421758
Depends on: 424546
Depends on: 424301
Depends on: 426288
Depends on: 425524
Depends on: 426925
Depends on: 427782
Blocks: 105520
Depends on: 430056
Duplicate of this bug: 240029
Depends on: 488581
You need to log in before you can comment on or make changes to this bug.