263050 - auto sized iframe and object frames report bogus MEW

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Arnout Engelen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040624 Debian/1.7-2
Build Identifier: among others, Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040624 Debian/1.7-2 and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040928 Firefox/0.9.3

The iframe in the left cell overlaps the text in the right cell.

The same problem comes up with objects, see mozbug2.html.

Reproducible: Always
Steps to Reproduce:
1. Go to website :)

Actual Results:  
I saw the iframe overlap the text in the other cell

Expected Results:  
showing the iframe in one cell, and the text in the other, without overlap.

Comment 1

[Bernd]

Attachment: testcase

its always better to attach the testcase to bugzilla than to host it on a some
foreign server, the server goes probably much faster away than the bug

Comment 2

[Bernd]

Attachment: object testcase

Comment 3

[Bernd]

 cell 0254AAE8 r=0 a=UC,UC c=UC,UC cnt=869
  block 0254AB94 r=0 a=UC,UC c=UC,UC cnt=870
   subdoc 0254ADD4 r=0 a=UC,UC c=UC,UC cnt=871
   subdoc 0254ADD4 d=3648,1848 me=48
  block 0254AB94 d=3648,1848 me=48
 cell 0254AAE8 d=3672,1872 me=72

cell 0254AAE8 r=2 a=744,UC c=720,UC cnt=880
 block 0254AB94 r=2 a=720,UC c=720,UC cnt=881
  subdoc 0254ADD4 r=2 a=720,UC c=UC,UC cnt=882
  subdoc 0254ADD4 d=3648,1848 me=48
 block 0254AB94 d=720,1848 me=48 o=(0,0) 3648 x 1848
cell 0254AAE8 d=744,1872 me=72

if a frame reports a mew of 48 it should be possible to shrink the frame to any
size that is greater than the MEW.

Comment 4

[Bernd]

374   if (aDesiredSize.mComputeMEW) {
375     // If our width is set by style to some fixed length,
376     // then our actual width is our minimum width
377     nsStyleUnit widthUnit = GetStylePosition()->mWidth.GetUnit();
378     if (widthUnit != eStyleUnit_Percent && widthUnit != eStyleUnit_Auto) {
379       aDesiredSize.mMaxElementWidth = aDesiredSize.width;
380     } else {
381       // if our width is auto or a percentage, then we can shrink until
382       // there's nothing left but our borders
383       aDesiredSize.mMaxElementWidth = border.left + border.right;
384     }
385   }

is not in sync with 
 if (NS_UNCONSTRAINEDSIZE != aReflowState.mComputedWidth) {
303     aDesiredSize.width = aReflowState.mComputedWidth;
304   }
305   else {
306     aDesiredSize.width = PR_MIN(PR_MAX(NSIntPixelsToTwips(300, p2t),
307                                        aReflowState.mComputedMinWidth),
308                                 aReflowState.mComputedMaxWidth);
309   }

we dont shrink below 300px for an autowidth iframe

Comment 5

[Bernd]

Attachment: patch

Comment 6

[Bernd]

the patch fixes both testcases

Comment 7

[bora123]

Are there any related good bits for a solution for bug 227470 in this fix?

Comment 8

[Bernd]

Comment on attachment 161195 [details] [diff] [review]
patch

switching reviewer (Robert is busy)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 161195 [details] [diff] [review]
patch

>+        if (!mContent->IsContentOfType(nsIContent::eXUL))

Was there supposed to be something after this?	Setting p2t only if we're not
XUL seems highly unusual.


Also, the general structure here is

if (!a && !b) {
} else {
  if (a) {
  } else {
  }
}

That's a bit odd, since they're all testing the same condition.  It would be
clearer as either:

if (is auto) {
} else if (is percent) {
} else {
}

or 

switch (widthUnit) {
  case auto:
    break;
  case percent:
    break;
  default:
}

Also, watch the spacing -- there's an "if(" with no space.

Also, do you want to account for mComputedMaxWidth as well, like the other code
you quoted does?

Comment 10

[Bernd]

Attachment: revised patch

Comment 11

[Bernd]

Comment on attachment 162375 [details] [diff] [review]
revised patch

I am not certain how the correct indent should look like in the
eStyleUnit_Auto: case, but I hope thats a minor issue.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 162375 [details] [diff] [review]
revised patch

>+    case eStyleUnit_Auto:
>+      aDesiredSize.mMaxElementWidth = PR_MIN(PR_MAX(defaultAutoWidth,
>+                                             aReflowState.mComputedMinWidth),
>+                                             aReflowState.mComputedMaxWidth) +
>+                                      border.left + border.right;
>+      break;

min-width should override max-width, so you need to reverse the PR_MIN and
PR_MAX.  Also, indent the second line more., so it should look like

PR_MAX(PR_MIN(defaultAutoWidth,
	       aReflowState.mComputedMaxWidth),
       aReflowState.mComputedMinWidth) +
border.left +border.right;

Comment 13

[Bernd]

Attachment: revised patch

Comment 14

[Bernd]

Comment on attachment 167928 [details] [diff] [review]
revised patch

David could you please rereview, the code that you critized is not my
invention, I think the code should by in sync.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 167928 [details] [diff] [review]
revised patch

r+sr=dbaron.  It's probably worth filing a separate bug on implementing
min/max-width/height on iframes that don't have auto width/height.

Comment 16

[Bernd]

shouldn't that be done inside the ReflowState?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, it seems to work already, so never mind.

Comment 18

[Bernd]

fix checked in