{inc}[FLOAT]Table is over-covered

RESOLVED FIXED in Future

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
17 years ago
9 years ago

People

(Reporter: Kevin Breit, Assigned: dbaron)

Tracking

(Depends on: 5 bugs, {testcase, top100})

Trunk
Future
testcase, top100
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
I noticed that the search results overlap with the Download Center table at right.

Comment 1

17 years ago
does your Mozilla have a build ID?  please report it.

worksforme with linux build 20020401.  probably a dupe of 129350.
(Reporter)

Comment 2

17 years ago
Gecko/20020326

Comment 3

17 years ago
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
Status: UNCONFIRMED → NEW
Component: Browser-General → Layout
Ever confirmed: true
QA Contact: doronr → petersen

Updated

17 years ago
Priority: -- → P2
Target Milestone: --- → Future

Comment 5

17 years ago
win98 2002040503, I see this behaviour but a local copy of the page is displayed
properly

Comment 6

16 years ago
See also bug 120107 (fixed)
Keywords: top100
OS: Linux → All
Hardware: PC → All

Comment 7

16 years ago
*** Bug 146686 has been marked as a duplicate of this bug. ***

Comment 8

16 years ago
Taking. amar: could you try to minimize if you've got time?
Assignee: attinasi → waterson
Keywords: qawanted
Whiteboard: [testcase]

Comment 9

16 years ago
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

Comment 10

16 years ago
 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.

Comment 11

16 years ago
Created attachment 85526 [details]
some what reduced testcase


 I tried to reduce this testcase further but even if I take out one <td> I
could not reproduce the problem.

Comment 12

16 years ago
 need to refresh the above testcase to reproduce the problem.

Updated

16 years ago
Keywords: testcase

Comment 13

16 years ago
*** Bug 148644 has been marked as a duplicate of this bug. ***

Comment 14

16 years ago
*** Bug 151997 has been marked as a duplicate of this bug. ***

Updated

16 years ago
Status: NEW → ASSIGNED

Comment 15

16 years ago
Created attachment 88738 [details]
Attaching the testcase again.

Comment 16

16 years ago
*** Bug 158489 has been marked as a duplicate of this bug. ***

Comment 17

16 years ago
*** Bug 137095 has been marked as a duplicate of this bug. ***

Comment 18

16 years ago
*** Bug 155301 has been marked as a duplicate of this bug. ***
*** Bug 167819 has been marked as a duplicate of this bug. ***

Comment 20

16 years ago
i think, there has to be mentioned that the window should be shrinked to see the
table overlapped by the results in the testcase

Comment 21

15 years ago
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 ?

Comment 22

14 years ago
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.

Comment 25

12 years ago
Created attachment 248103 [details]
post-reflow branch testcase

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.

Updated

12 years ago
Assignee: waterson → nobody
Status: ASSIGNED → NEW
QA Contact: chrispetersen → layout
shouldn't this block bug 300030 ?

Updated

12 years ago
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?
Flags: blocking1.9? → blocking1.9+
Priority: P2 → P3
Created attachment 292485 [details]
post-reflow branch testcase #2

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 http://www.w3.org/TR/CSS21/visuren.html#floats 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.
Created attachment 298865 [details] [diff] [review]
work in progress

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.
Created attachment 299329 [details]
program to show which frames pass a condition I'm removing

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:

nsTableCaptionFrame
nsTableColFrame
nsLegendFrame
nsIsIndexFrame
nsSelectsAreaFrame
nsComboboxDisplayFrame
nsMathMLForeignFrameWrapper
nsMathMLmtdInnerFrame
nsMathMLmathBlockFrame
nsFrame
nsAreaFrame
nsPageBreakFrame
nsDirectionalFrame
nsLeafFrame
nsBlockFrame
nsBulletFrame
BRFrame
SpacerFrame
nsHTMLFramesetBorderFrame
nsHTMLFramesetBlankFrame
nsPlaceholderFrame
nsSVGLeafFrame
nsSVGStopFrame
nsSVGImageFrame
nsSVGPathGeometryFrame
nsSVGGeometryFrame
nsSVGGlyphFrame

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.
Created attachment 299497 [details] [diff] [review]
patch

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)
Created attachment 299851 [details] [diff] [review]
patch

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]
patch

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...
Created attachment 299955 [details] [diff] [review]
patch

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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 414558
I think this caused bug 414558

Updated

11 years ago
Depends on: 417914
Depends on: 421758

Updated

11 years ago
Depends on: 424546

Updated

11 years ago
Depends on: 424301

Updated

10 years ago
Depends on: 426288

Updated

10 years ago
Depends on: 425524
Depends on: 426925

Updated

10 years ago
Depends on: 427782

Updated

10 years ago
Depends on: 430056
Duplicate of this bug: 240029
Blocks: 240029
Depends on: 488581
You need to log in before you can comment on or make changes to this bug.