368554 - [quirks] Image inside table inside table-cell with small width is wrapped

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, this used to display a 200*200px green box, but that doesn't happen anymore with current trunk builds.
This regressed between 2006-12-07 and 2006-12-08, so a regression from the reflow branch landing.

Comment 1

[Eli Friedman]

I'm pretty sure the cause of this is simply that the quirk got removed.  So to fix this, the quirk needs to be reimplemented.

I think it would be sufficient to override nsFrame::AddInlineMinWidth in nsImageFrame with a specialization that doesn't allow breaking in quirks mode if the nearest block's parent is a table cell with an auto width.  Nice and unobtrusive quirk.  I haven't tried that, though, so I'm not certain that's enough.

Comment 2

[Eli Friedman]

See also Bug 25679?

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase from bug 395347

Comment 5

[Mats Palmgren (inactive)]

FWIW, Firefox 1.5/2, Opera, IE7 and Safari/Webkit displays the images on the same line.

Comment 6

[Martijn Wargers (dead)]

I think I see this bug also here: http://www.yourfuturepctv.nl/wat_op_tv/tvgids.html
Although that page is in Standards compliance mode.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Moving this to P2 for drivers -- wouldn't block b2 for it, but we should fix for future/final.  However, if the patch would be very risky/intrusive, please bump back up to P1 and comment.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I made it P1 because any fix is likely to carry risk of layout regressions on real sites.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch - override nsImageFrame::AddInlineMinWidth()

(In reply to comment #1)
> I'm pretty sure the cause of this is simply that the quirk got removed.  So to
> fix this, the quirk needs to be reimplemented.
> 
> I think it would be sufficient to override nsFrame::AddInlineMinWidth in
> nsImageFrame with a specialization that doesn't allow breaking in quirks mode
> if the nearest block's parent is a table cell with an auto width.  Nice and
> unobtrusive quirk.  I haven't tried that, though, so I'm not certain that's
> enough.
> 

It's not quite enought to just check if we're in a block in an auto-width table cell, we also need to check to see if we're beside another image, this patch does that. This passes all the test cases on Mac, Windows, and also SoftPedia.com displays correctly too! Patch includes reftest.

Comment 10

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

A few quick comments:
 * the name of the function "BesideAnotherImage" doesn't reflect what it does; it should be renamed
 * likewise, I'm not sure why "GetDirectPrevSibling" shouldn't just be called "GetPrevSibling"
 * Preceding doesn't have a double-e
 * try to follow local coding style, meaning function opening brace on its own line, function name beginning its own line after the type, etc.
 * does it matter if the previous / next sibling is an image?  Does the quirk in IE connect text to images as well?  Does our old quirk?
 * why does having an image before *or* after cause the break to be omitted on both sides?  Shouldn't where the breaks are skipped correspond to the joins?  Or is that actually what other browsers do?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The last point does seem very weird.

Here's my reverse-engineering of what we were doing:
http://lxr.mozilla.org/mozilla1.8/source/layout/generic/nsLineLayout.cpp#1737

* AccumulateImageSizes just adds up all the image widths in the subtree rooted at the frame
* So "curFrameAccumulates" is set if the current frame is or contains an image, or is not-all-whitespace text whose intrinsic-min-width is equal to its width (which mostly means "non-whitespace text with no break opportunity inside it")
* Disallow breaking for min-width computation purposes between two frames that are direct children of a cell with "unconstrained width", if they both "accumulate"

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Which makes no sense because a table cell containing ten copies of
<span>Hello <img width="50"> Kitty</span>
will get a min-width of 10*50. But that's what happens in FF2. Yay.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The fix for bug 403426 affects testcases here quite a bit, as currently we often allow a break between an image and text where we shouldn't.

Maybe a reasonable quirk implementation would be to have nsImageFrame only do OptionalBreak before itself if it's preceded (in the flattened frame tree) by something that's not an image or textframe. And likewise only do OptionalBreak after itself if it's followed (in the flattened frame tree) by something that's not an image or textframe.

The former condition could be easily implemented with another flag in InlineMinWidthData, but the latter is harder...

Comment 14

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

(In reply to comment #13)
> Maybe a reasonable quirk implementation would be to have nsImageFrame only do
> OptionalBreak before itself if it's preceded (in the flattened frame tree) by
> something that's not an image or textframe. And likewise only do OptionalBreak
> after itself if it's followed (in the flattened frame tree) by something that's
> not an image or textframe.

What other than an image or a textframe would not put a break at its start and end?  (In other words, would the easy half only be sufficient?)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If everything other than images and textframes puts a break at its start and end, then we don't need the easy half either. I.e. we just have images not break and start and end if we're in the unconstrained table cell.

Comment 16

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

Not if we want a break between text and an image.  (Do we?  It's still not clear to me what the quirk we're implementing here is supposed to do.)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

According to my analysis, we don't want to break between text and an image, unless the text has a break opportunity of its own inside it.

> It's still not clear to me what the quirk we're implementing here is supposed
> to do.

Comment #11 is my best summary of what we were doing in Gecko 1.8. But browser behaviour varies considerably so we don't have to implement exactly that. Hence I think we should try my suggestion in comment #13 (hopefully equivalent to comment #15).

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2 - override nsImageFrame::AddInlineMinWidth()

(In reply to comment #13)
> Maybe a reasonable quirk implementation would be to have nsImageFrame only do
> OptionalBreak before itself if it's preceded (in the flattened frame tree) by
> something that's not an image or textframe. And likewise only do OptionalBreak
> after itself if it's followed (in the flattened frame tree) by something that's
> not an image or textframe.
>

This patch implements this, it works, patch includes reftest. Requires the patch for Bug 403426 to correctly work, and to pass the reftest.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 - override nsImageFrame::AddInlineMinWidth() reduced

Similar to previous, but reduced. Still requires 403246 to pass reftests.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

IsInAutoWidthTableCell checks for quirks mode too, so it should probably be called something like ApplyMinWidthQuirk.

+  if (eCompatibility_NavQuirks == aFrame->PresContext()->CompatibilityMode()) {

Slightly cleaner to just exit early if != ... saves indentation.

Need to investigate how far up the frame tree we actually need to scan looking for the auto-width table cell.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

IE7 quirks:
* Images/text inside a div
* Images/text inside a span
* Text in a floating div (images !quirked)
* Images/text inside relatively positioned div
* Images/text inside a div
* Multiple position:relative images (it seems their non-translated size is used to size the table cell).
* Images/text in relatively positioned span.
* Images/text in relatively positioned div.

IE7 does not quirk:
* Images inside a floating div (text is quirked).
* Images/text inside an absolutely positioned div.
* Multiple floating images.

FF2 quirks:
* Images in relatively positioned span (text !quirked).
* Images in relatively positioned div (text !quirked).
* Images inside a relatively positioned div (text !quirked)
* Multiple position:relative images (similar behviour to IE7)

FF2 does not quirk:
* Text in relatively positioned span (images quirked).
* Text in relatively positioned div (images quirked).
* Images/text inside a div
* Images/text inside a span
* Images/text inside floating div
* Multiple floating images
* Text in relatively positioned div (images quirked)
IE7 does not quirk:
* Images inside a floating div (text is quirked).
* Images/text inside an absolutely positioned div.
* Multiple floating images.

FF2 quirks:
* Images in relatively positioned span (text !quirked).
* Images in relatively positioned div (text !quirked).
* Images inside a relatively positioned div (text !quirked)
* Multiple position:relative images (similar behviour to IE7)

FF2 does not quirk:
* Text in relatively positioned span (images quirked).
* Text in relatively positioned div (images quirked).
* Images/text inside a div
* Images/text inside a span
* Images/text inside floating div
* Multiple floating images
* Text in relatively positioned div (images quirked)

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

IE7 also quirks images & text inside a div inside a 100%-width table inside a div inside a fixed width table. FF2 doesn't (neither does my current patch on trunk).

So I think we should at least support the quirk for stuff inside a div/span inside a table cell, and we should handle constrained and unconstrained table cells inside table cells?

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Opera 9 quirks everything except images/text which is floating inside a table cell (which I think makes sense).

Safari quirks on images but not text in spans, and doesn't quirk inside divs. It doesn't quirk the nasty inside-div-inside-table-inside-div-inside-table case (Opera does).

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Monster test case

This shows the behaviour of the quirk under different conditions. Try running it in different browser to see their behaviour...

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

FF2 doesn't really apply the quirk to images that are nested inside a block element in the table cell. Neither does Safari. So I think we can not worry about that situation. We should only apply the quirk when the containing block is the table cell.

So I guess the best approach is to use nsLayoutUtils::FindNearestBlockAncestor to find the nearest containing block, and then check if that is the anonymous block for a table cell frame using frame->GetStyleContext()->GetPseudoType() == something or other.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsGkAtoms::cellContent

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4

With further changes...

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (eCompatibility_NavQuirks == aFrame->PresContext()->CompatibilityMode()) {

Make this "if (!=) return;" for less indenting.

+        grandAncestor->GetType() == nsGkAtoms::tableCellFrame &&

This is not needed.

This is kinda fragile because it assumes table cells do not construct wrapper frames (e.g. nsHTMLScrollFrame or nsColumnSetFrame) for the the anonymous block. This is currently true. Better add a comment to the call to NS_NewTableCellInnerFrame in nsCSSFrameConstructor saying that if this gets changed, we should also check this code here in nsImageFrame :-(.

+      if (grandAncestor &&
+        grandAncestor->GetType() == nsGkAtoms::tableCellFrame &&
+        grandAncestor->GetStylePosition()->mWidth.GetUnit() == eStyleUnit_Auto)
+      {
+        return PR_TRUE;
+      }

This is better written as a simple "return <boolean expression>;"

IsInAutoWidthTableCell should be called IsInAutoWidthTableCellForQuirk or something like that because it's also checking for quirks mode.

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v5

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 289381 [details] [diff] [review]
Patch v5

Note this patch still depends on the patch for Bug 403426, so it should not be checked in before that is, else the reftests added in this patch will fail.

Comment 31

[Reed Loden [:reed]]

Taking this off the checkin-needed list until bug 403426 is fixed. Once bug 403426 has been fixed, please re-add the checkin-needed keyword.

Comment 32

[Reed Loden [:reed]]

Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1436; previous revision: 1.1435
done
Checking in layout/generic/nsImageFrame.cpp;
/cvsroot/mozilla/layout/generic/nsImageFrame.cpp,v  <--  nsImageFrame.cpp
new revision: 1.419; previous revision: 1.418
done
Checking in layout/generic/nsImageFrame.h;
/cvsroot/mozilla/layout/generic/nsImageFrame.h,v  <--  nsImageFrame.h
new revision: 1.106; previous revision: 1.105
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/384322-1-ref.html,v
done
Checking in layout/reftests/bugs/384322-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/384322-1-ref.html,v  <--  384322-1-ref.html
initial revision: 1.1
done
Checking in layout/reftests/bugs/384322-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/384322-1-ref.html,v  <--  384322-1-ref.html
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/384322-1.html,v
done
Checking in layout/reftests/bugs/384322-1.html;
/cvsroot/mozilla/layout/reftests/bugs/384322-1.html,v  <--  384322-1.html
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.245; previous revision: 1.244
done

Comment 33

[Boris Zbarsky [:bzbarsky]]

So why the cast to nsFrame*?  Why not just use nsIFrame*?

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Er yeah, I dunno.

Comment 35

[pal-moz]

regression or intended ?
http://forums.mozillazine.org/viewtopic.php?p=3171929#3171929

Comment 36

[Boris Zbarsky [:bzbarsky]]

I guess I should have been clearer with comment 33.  Please get rid of the cast.  It's not needed.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> regression or intended ?

There are spaces between those images.  So I sure hope it's not intended!  Is there a bug filed?

Comment 38

[u254340]

I don't think so.
I searched and found nothing.

Comment 39

[Steve England [:stevee]]

This bug caused bug 407184.

Comment 40

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch - casting removal as requested by BZ.

(In reply to comment #36)
> I guess I should have been clearer with comment 33.  Please get rid of the
> cast.  It's not needed.
> 

Change as requested.

Comment 41

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Test cases which are still broken

The following cases still render differently in FF2 and Minefield.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 291929 [details] [diff] [review]
Patch - casting removal as requested by BZ.

Thanks!

Comment 43

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

Comment on attachment 289381 [details] [diff] [review]
Patch v5

>+    // Assume direct parent is a table cell frame.

No such thing as a "direct parent".  A node in a tree has a unique parent, so parent needs no qualifier other than which node it's the parent of.  (So it might be good to say "Assume ancestor's parent is a table cell frame.")

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The first two cases in that attachment are OK, I think. We now treat content in a span the same as if it wasn't in a span. I think that's a good thing.

The last one is a problem. Let's file a new bug on that. And fix it pronto! It may well be the cause of bug 407184 and other regressions.

Comment 45

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch - comment change for dbaron, casting change for bz

(In reply to comment #43)
> (From update of attachment 289381 [details] [diff] [review])
> >+    // Assume direct parent is a table cell frame.
> 
> No such thing as a "direct parent".  A node in a tree has a unique parent, so
> parent needs no qualifier other than which node it's the parent of.  (So it
> might be good to say "Assume ancestor's parent is a table cell frame.")
> 

As requested, with BZ's change so we wrap it into a single patch.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Bug 407184 covers the issue from comment 35, and is indeed the same as the third testcase in attachment 291930 [details].

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, well, I already filed bug 407227.