162063 - [FIX]Table-related pseudo-frames do not get removed when DOM/style is changed

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This is a bug to track the various issues caused by the following problem:

During frame construction we create "pseudo" frames when non-table-related
frames are inside a table but not inside a table cell.  Similarly, we create
"pseudo" frames when a cell is not inside a table.

The problem is that if a dynamic modification of some sort happens (changing the
display of the frame that forced the creation of the "pseudo" frames, removing
it from the DOM, etc) the "pseudo" frames do not get destroyed.  They stay in
the frame tree.

Bug 156888 and bug 97506 (last testcase; the one in bug 97506 comment 14) have
testcases that demonstrate the problem.

It seems to me that we want to somehow mark a frame that forced pseudo-frames to
be created and have a way of getting from that frame to the outermost
pseudo-frame it forced.  Then ContentRemoved (which both DOM removal and frame
reconstruction go through) should tear down not just the primary frame, but also
all the pseudos it forced into being.

I tried hacking this together, but have found that getting at the
newly-constructed frame and the outermost pseudo at the same time is not so
easy... ;)

Comment 1

[Boris Zbarsky [:bzbarsky]]

As a note, we have a bunch of "INVALID" bugs that this would actually fix, I
think...  People like to set table rows to "block", apparently.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I've been thinking... it seems to me that the best thing would be for the
nsPseudoFrames to hold a pointer to the "topmost" pseudo frame that has been
created.  Would that make sense?

Comment 3

[karnaze (gone)]

I'm not sure if the (unfinished) patch bug 148810 deals with this, but a pseudo
frame has the same content as its parent, which is a fairly simple check. I
can't remember if there are non-pseudo frames where this is true.

Comment 4

[karnaze (gone)]

mass reassign to default owner

Comment 5

[Boris Zbarsky [:bzbarsky]]

Could we just set a framestate bit on pseudo table/row/cell/whatever frames? 
Then when content is removed we can check that its frame is the only child of
its parent, and if it is walk up the parents till we get to a frame that doesn't
have the bit set, then remove everything below that....

Comment 6

[Bernd]

Attachment: scetch

This tries to implement comment 3, while it really detects the removable
pseudoframes, the usage is horked and I dont yet understand why.

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 319611 has been marked as a duplicate of this bug. ***

Comment 8

[Bernd]

Attachment: patch

This does at least the job, while most of the testcases are rendered now anyway better after the empty ascent bug got fixed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

That doesn't seem to handle correctly the case when the child being removed has previous siblings but not next sibling, does it?

Comment 10

[Bernd]

taking to get this moved

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixing this should watch out for the issues found in bug 326834.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

With Bernd's permission I've made an improved patch that address the
remaining problems:

1. look for previous siblings (Boris' comment 9)
2. when dealing with out-of-flow's (bug 339388) we need to adjust the
   frame also when removing the placeholder.
3. when dealing with captions (bug 293576) I've added a test for the
   case where we an empty pseudo inner table frame (which shouldn't
   block the removal of the pseudo outer when removing the last caption)
4. As foreseen by the oracle in comment 11, I did indeed bump into 
   problems with regressing bug 326834. I've added a wallpaper for
   that (an early return for XUL).

I've tested all the attached testcases of the dependent bugs and I think
all of them are now fixed. No problems in regression tests etc.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 224550 [details] [diff] [review]
Patch rev. 2

Mats, could you add -p to your diff options, in general?  It'd be nice to know where the second chunk of this patch lives without having to open nsCSSFrameConstructor.  ;)

I'm not sure I follow the "early return for XUL" logic here.  XUL with table frame types should behave like HTML with table frame types, no?

Why not just have GetPseudoAdjustedChild return an nsIFrame*?  That would be better, I think.

We change the parentFrame pointer in ContentRemoved, right?  Do we need to adjust the first-letter stuff accordingly?  I suspect that we do...  If nothing else, if I have something like:

<div>
  <span style="display: table-cell">
    Text
  </span>
  More text
</div>

And the div has a first-letter style, and then I remove the span, I doubt that the "M" becomes styled with the first-letter style with this patch.

>+      // If this is a row group then there should be nothing else than anonymous 
>+      // cellbased column frame.

"nothing other than an"

I'd kinda like bernd to review this part of the code...

>+  else if (parentPseudo == nsCSSAnonBoxes::cellContent) {

Don't you want to set aAdjChildFrame to the table cell in this case?  Or is it ok because we just recurse with the cellContent as aChildFrame and fall right through the if/else cascade to the next recursion level?

>+  else if (parentPseudo == nsCSSAnonBoxes::tableCol) {
>+    NS_ASSERTION(PR_FALSE, "illegal colFrame children detected");

NS_ERROR

>+    return;
>+  }
>+  else if (parentPseudo != nsCSSAnonBoxes::tableRowGroup &&

Why bother with the else?

Looks good other that these issues, but I'd like to see an updated patch.

Comment 14

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

(In reply to comment #13)
> Mats, could you add -p to your diff options, in general?

Oops, sorry, I always do but I forgot it in this case...

> I'm not sure I follow the "early return for XUL" logic here.  XUL with table
> frame types should behave like HTML with table frame types, no?

Yes, what I wanted to say is that appearantly some frame trees for XUL are
sensitive in what you do with them... at least I don't understand how to
deal with this case. But I was probably overly pessimistic and I have
limited the wallpaper to <listboxbody>/<listitem> in this patch.

If you know any details on why it crashes I'd be happy to try to fix it.
I think we can spawn off a separate bug on this case though.


> Why not just have GetPseudoAdjustedChild return an nsIFrame*? 

Fixed


> We change the parentFrame pointer in ContentRemoved, right?

Yes

>Do we need to adjust the first-letter stuff accordingly? 

I'm not sure what you mean... the |RecoverLetterFrames()| near the end of the
patch uses the new value for |parentFrame| and the code looks ok to me...
Testcase #1 makes 'M' the first letter when removing the <span> or setting
its display to 'none'.
I can't think of a case where removing a first-letter/first-line frame could
trigger the removal of a pseudo table frame (if that's what you were
thinking of).


> I'd kinda like bernd to review this part of the code...

Bernd wrote this bit in his original patch. FWIW, I think it looks fine,
but I don't claim to know much about the possible table frame trees.


> >+  else if (parentPseudo == nsCSSAnonBoxes::cellContent) {
> 
> Don't you want to set aAdjChildFrame to the table cell in this case?  Or is it
> ok because we just recurse with the cellContent as aChildFrame and fall right
> through the if/else cascade to the next recursion level?

Yes, we would handle it on the next level, but I changed the code slightly
for this block now, we can test directly if the grand parent is a pseudo and if
not return directly, and in this case the Type should be tableCellFrame, right?
(In other words, a cellContent pseudo is either in a cell or pseudo cell and
it's always the only child and it should never be removed unless the cell is.)


> NS_ERROR

Fixed


> Why bother with the else?

Because if we took any of the previous forks we could fall through and
we don't want to test |parentPseudo| again?

Comment 16

[Mats Palmgren (inactive)]

I should also say that I found a sequence of events in bug 339388 that is not
completely fixed by any of these patches. It's rather complex and I don't
think it is anything we can fix in GetPseudoAdjustedChild(), no matter how
we write it. I'll keep bug 339388 open and explain the details there.
The patch fixes the simple testcases (#2 and #3) in that bug though.

Comment 17

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

(In reply to comment #15)
> (In reply to comment #13)
> > Mats, could you add -p to your diff options, in general?
> 
> Oops, sorry, I always do but I forgot it in this case...

You can create a ~/.cvsrc file with default options (even on windows, although where cygwin thinks your homedir is may be a little interesting).  For example, mine is:

cvs -z3 -q
diff -pNu12
update -P
checkout -P

Comment 18

[Mats Palmgren (inactive)]

Cool, thanks. I'll do that.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> If you know any details on why it crashes I'd be happy to try to fix it.

Kids of listbox don't go through normal frame construction.  See the NotifyListBoxBody call at the top of ContentRemoved.  You want to modify that code to get the updated child frame and pass _that_ to NotifyListBoxBody.  Otherwise we'll first pass in the wrong frame there (so NotifyListBoxBody will ignore it due to the fix for bug 326834) and then will mess with the listbox's kids manually, which is bad.

> the |RecoverLetterFrames()| near the end of the patch uses the new value for
> |parentFrame|

But the old value for |haveFLS|, no?  And the old value for |containingBlock|.  I'll attach a testcase where the latter causes a crash.  If it's fixed, the former causes an incorrect frame model.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing first-letter problem

Comment 21

[Mats Palmgren (inactive)]

(In reply to comment #19)
Ok, I see what you mean, I'll fix those issues. There also a bug in the
existing first-letter code:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1235&root=/cvsroot&mark=9965#9963

In case |childFrame| is an out-of-flow we'd want the containing block of its
placeholder instead... I'll fix that too while I'm here...

Comment 22

[Mats Palmgren (inactive)]

Attachment: More first-letter fun, click remove, M should be green

Comment 23

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4 (diff -w)

Changes since last revision:
  1. pass a pseudo adjusted frame to NotifyListBoxBody()
  2. use the containing block of the placeholder when removing
     first-letter frames when |childFrame| is out-of-flow.
  3. re-calculate |haveFLS| and |containingBlock| if remove a pseudo
     ancestor instead the original frame.
  4. instead of having a "if (childFrame) {" cover most of ContentRemoved()
     I changed it to an early return instead.

Comment 24

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 225377 [details] [diff] [review]
Patch rev. 4 (diff -w)

I just got this when running some evil tests with this patch:
###!!! ASSERTION: can't remove inner frame: 'nsLayoutAtoms::captionList == aListName', file nsTableOuterFrame.cpp, line 268
so this patch still has some problem...

Comment 26

[Mats Palmgren (inactive)]

I guess we should not remove an empty pseudo inner table if the outer table
still has a caption... I'm guessing it's ok for the inner to be empty though?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Yeah, the inner can be empty in that case.

Comment 28

[Bernd]

>I guess we should not remove an empty pseudo inner table if the outer table
>still has a caption...
Its mandatory to have the inner once we have the outer, we need it at least for the bizarre parent style relationship that both have.

>I'm guessing it's ok for the inner to be empty though?
Yep, thats normal you can even size it :-), OK the chance are high that your nick name has somewhere a 'hixie' in it when you do this but its legal.

Comment 29

[Mats Palmgren (inactive)]

Attachment: Patch rev. 5 (diff -w)

Changes since last revision:
  1. don't remove the inner table frame unless the outer is too
  2. include 'bcTableCellFrame' in the assertion that verifies the parent
     of 'cellContent'

Comment 30

[Mats Palmgren (inactive)]

Attachment: Patch rev. 5

Comment 31

[Boris Zbarsky [:bzbarsky]]

Mats, I'm going to try to get to this in the next few days, but if that doesn't happen it'll be after July 13.  :(

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 225694 [details] [diff] [review]
Patch rev. 5 (diff -w)

>Index: layout/base/nsCSSFrameConstructor.cpp
>+    RemoveLetterFrames(presContext, mPresShell, frameManager, containingBlock);

So we remove letter frames on the containing block of parentFrame.

>+  // If we have pseudo frames that can go away when removing |frameToRemove|
>+  // then remove them too. If so, adjust |containingBlock| accordingly for the
>+  // RecoverLetterFrames() call below.

But possibly recover on some other block?  That seems wrong to me...

Other than that, the patch looks good.

Comment 33

[Mats Palmgren (inactive)]

(In reply to comment #32)
> But possibly recover on some other block?  That seems wrong to me...

I was thinking of the case where we have:

<block>
  <pseudos>
    <frame-to-remove>
  <sibling>

If any of the <pseudos> is the containing block then we will do
RemoveLetterFrames() on that and then when it is removed we need to create
first-letter frames in <block> because <pseudos> could have been blocking
us from reaching <sibling>... but you're right, we would need to do a
RemoveLetterFrames() on the new containing block for that to work correctly.

So, we could do both RemoveLetterFrames() + RecoverLetterFrames() when
the containing block changed or we could simply skip RecoverLetterFrames().
I think I'll do the latter for now... although in general, I think that
removing <pseudos> could enable new opportunities for first-letter's
in <block> and we should do more to restore them according to the new frame
tree. But, first-letter is quite broken as is already and I'm guessing the
reflow-branch is the solution to that problem?

Comment 34

[Mats Palmgren (inactive)]

Attachment: Patch rev. 6

Comment 35

[Mats Palmgren (inactive)]

Attachment: Patch rev. 6 (diff -w)

Comment 36

[Boris Zbarsky [:bzbarsky]]

Reflow branch won't affect any of this code, actually.  I'll try to get to this review in the next few days.  Thanks for beating on this, Mats!

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 228591 [details] [diff] [review]
Patch rev. 6 (diff -w)

I'm sorry I'm being so slow with these.  :(  Next one should be a lot faster; I've put this on my "get this done quick" list...

>Index: layout/base/nsCSSFrameConstructor.cpp
>+GetPseudoAdjustedChild(nsIFrame* aParentFrame,

>+  nsIFrame* firstChild = aParentFrame->GetFirstChild(nsnull);
>+  if (aChildFrame->GetNextSibling() ||
>+      (aChildFrame != firstChild &&
>+       aChildFrame->GetType() != nsLayoutAtoms::tableCaptionFrame)) {
>+    return aChildFrame; // The child has a sibling (caption is handled below).

So consider the following markup:

<div style="display: table-row">
  <div style="float: right">
  </div>
</div>

This will create an anonymous cell and put the float in it.  Now say I remove the float.  We'll get into GetPseudoAdjustedChild, discover that firstChild (the placeholder) is not aChildFrame, and bail out.  That seems wrong.

>+  else if (parentPseudo == nsCSSAnonBoxes::table) {
>+    if (nsLayoutAtoms::tableColGroupFrame == aChildFrame->GetType()) {
>+      if (firstChild) { 
>+        return aChildFrame; // There is still a row group frame.

How can we hit this case?  Wouldn't we have bailed earlier because firstChild (which is a row group in this case) would not equal aChildFrame (which is a colgroup)?  Maybe just assert !firstChild here (and make this whole if body #ifdef DEBUG)?

>@@ -9887,41 +9969,50 @@ 
>+    nsIFrame* pseudoAdjustedChildFrame = childFrame;
>+    if (childFrame) {

So I've been wondering... Instead of adding the several calls to GetPseudoAdjustedChild(), wouldn't it make more sense to do:

      childFrame = mPresShell->GetPrimaryFrameFor(aChild);
      if (!childFrame) {
        frameManager->ClearUndisplayedContentIn(aChild, aContainer);
        return NS_OK;
      }
      childFrame = ::GetPseudoAdjustedChild(childFrame->GetParent(), childFrame);
      NS_ASSERTION(childFrame, "Shouldn't get null");

in the two places where we currently get childFrame (here, and after we remove letter frames), and then leave all the other code in this method as-is?  It seems like that would do closest to what we want (call DeletingFrameSubtree on the right frames, do IsFrameSpecial checks on the right frames, etc).  It would also be a lot easier to check correctness, I think...

Is there some reason we can't do this?

>+  // then remove them too. In that case, avoid doing RecoverLetterFrames()
>+  // below if |containingBlock| is one the pseudos.

And this part could be nixed, I think, if we just used the pseudo-adjusted frame as childFrame...

If this code does stay, "one of the".

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 228591 [details] [diff] [review]
Patch rev. 6 (diff -w)

r- per comment 37.

Comment 39

[Eli Friedman]

What exactly happened to this patch?  It's been sitting around for over a year.

Comment 40

[Bernd]

>What exactly happened to this patch?

The patch was r-'ed and nobody did work on this. If you have spare cycles please go ahead, I can assist with comments and bz probably is also happy to get this moved but its only a matter of work (testing is the interesting part).

Comment 41

[Jesse Ruderman]

The code in bug 373610 might help with testing a patch for this bug.

Comment 42

[Bernd]

I am taking this, if I can't get it done I will give it back.

Comment 43

[Bernd]

Attachment: last patch updated to tip

Comment 44

[Bernd]

Attachment: patch to address review comments

Comment 45

[Bernd]

Attachment: reftest against about:blank

we show a magenta and a orange square with this patch

Comment 46

[Bernd]

Attachment: patch that passes attachment 363507

Comment 47

[Bernd]

Attachment: revised testcase

that shows that attachment 363542 [details] [diff] [review] does not remove the pseudo when a -moz-popup was wrapped.

Boris what else can one stuff into the pseudos and then remove it?

Comment 48

[Bernd]

The -moz-popup failure is caused by the code in nsCSSFrameConstructor::ConstructFrameInternal where we first create a pseudo frame based on the display type inside AdjustParentFrame and later do not create a frame for it.

  if (data) {
#ifdef MOZ_XUL
    if ((data->mBits & FCDATA_IS_POPUP) &&
        adjParentFrame->GetType() != nsGkAtoms::menuFrame &&
        !aState.mPopupItems.containingBlock) {
      return NS_OK;
    }
#endif /* MOZ_XUL */

The same might be triggered by the svg check

above that code.

Later we go through the cleanup of undisplayed content and since there is no frame to remove there will be no pseudo removal.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Yeah, now that we get the FrameConstructionData in all cases before we call AdjustParentFrame (we used to do the by-display datas after when I did the initial FrameConstructionData landing) we should hoist the popup and SVG checks to before AdjustParentFrame, I think.  File a separate bug for that (blocking this one) and either assign to me or take yourself as you prefer?  It should even be possible to write reftests for it against trunk, and certainly against trunk with this patch, as you demonstrated.

Comment 50

[Boris Zbarsky [:bzbarsky]]

OK, there's no issue with SVG here.  For the popup issue I filed bug 480017.

Comment 51

[Bernd]

Attachment: patch

Comment 52

[Boris Zbarsky [:bzbarsky]]

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

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>+GetPseudoAdjustedChild(nsIFrame* aParentFrame,
>+                        nsIFrame* aChildFrame)

Fix the indent.

>+  // if the childFrame gets removed all the pseudo frames that are between
>+  // childframe and the real parent can be removed
>+  // so we seek upwards a frame which is either not a table pseudo frame

  "So we look up until we get to a frame which is either ..."

>+  // or is table pseudo frame which has more valid children than the one needed

  "or is a table pseudo ..."

>+    return aChildFrame; // The child has a sibling  or the parent has definetely

  "definitely"

>+    // XXXbernd should check all childlists of the block frame if there any other
>+    // children than the childframe or is the entrance test for only one children
>+    // on the main childlist enough??

The main childlist check is enough, since all other childlists involve having an in-flow child (placeholder).

In fact, checking other childlists would be wrong, since we could have lots of floats on the float list whose placeholders are all descendants of the same in-flow inline child, say.

>+  if (parentPseudo == nsCSSAnonBoxes::table && result == aParentFrame) {
>+    // The inner table should not be removed unless the outer is too.
>+    result = aChildFrame;
>+   }
>+  return result;

Weird indent on the '}' there.

>@@ -8458,17 +8560,21 @@ nsCSSFrameConstructor::ContentRemoved(ns
>+    return NS_OK;
>+  }
>+   childFrame = ::GetPseudoAdjustedChild(childFrame->GetParent(),
>+                                         childFrame);
>+   NS_ASSERTION(childFrame, "Shouldn't get null");

Weird indent.

>@@ -8595,19 +8704,29 @@ nsCSSFrameConstructor::ContentRemoved(ns
>+      nsIFrame* adjustedPlaceholder = 
>+        ::GetPseudoAdjustedChild(placeholderParent, placeholderFrame);

Hmm....  Is this needed?  If so, why?

How do we handle this testcase:

  <div style="display: table-row">
    <span>abc<span style="float: left"></span></span>
  </div>

and then removing the floating span?  It seems like GetPseudoAdjustedChild() on the floating block will return the cell in this case, so we'll remove that... and then the text will disappear.  Can you double-check?

In general, it seems like we should not be calling GetPseudoAdjustedChild() on out-of-flow frames...

>+      PRBool removePseudo = (adjustedPlaceholder != placeholderFrame);
>       ::DeletingFrameSubtree(frameManager, placeholderFrame);
>       rv |= frameManager->RemoveFrame(placeholderParent,
>                                       nsnull, placeholderFrame);
>+      if (removePseudo) {
>+         placeholderParent = adjustedPlaceholder->GetParent();
>+         ::DeletingFrameSubtree(frameManager, adjustedPlaceholder);
>+         rv |= frameManager->RemoveFrame(placeholderParent,
>+                                          nsnull, adjustedPlaceholder);
>+      }

This part doesn't make much sense to me.

>+++ b/layout/reftests/bugs/reftest.list
>+== 280708-1a.html 280708-1-ref.html # bug 473824

Don't need the comment any more, do you?

Comment 53

[Boris Zbarsky [:bzbarsky]]

I was thinking some more.  It seems to me that it's never correct to pass an out-of-flow to GetPseudoAdjustedChild.  So the right control flow should be:

1)  Get primary frame.
2)  If it's out of flow, get its placeholder.
3)  Pass the placeholder, or the primary frame if it's in-flow to
    GetPseudoAdjustedChild.
4)  If GetPseudoAdjustedChild is different from the frame passed to it, use its
    result as the child frame.

(the check in item 4 is needed to keep from using the placeholder instead of the primary frame when no adjustment is needed).

Alternately, GetPseudoAdjustedChild could get the placeholder and use it for its various checks.  Either way, I guess...

Comment 54

[Boris Zbarsky [:bzbarsky]]

And thinking about it even more, we have another problem with removals that this patch doesn't address.  Consider this testcase:

<!DOCTYPE html>
<html>
<script>
  window.onload = function() {
    var n = document.getElementById("n");
    n.parentNode.removeChild(n);
  }
</script>
<body>
  a<span style="display: table-cell"></span>
  <span style="display: table-cell" id="n"></span>b
</body>
</html>

When loaded, this should presumably look like:

  a b

whereas with this patch it'll look like "ab".  This didn't use to be a problem back when we shipped whitespace out to before the table, but it really is an issue.

So maybe what we should do is reframe the parent content any time a frame is removed which has a pseudo parent and is its first or last primary child list child (with some extra hackery for captions).  That would remove a lot of the complexity in the patch, since that's aimed at removing as little as possible and ends up not reframing enough, imo.

Thoughts?

Comment 55

[Boris Zbarsky [:bzbarsky]]

If we do take an approach more similar to that, then the right place to do it is probably in MaybeRecreateContainerForIBSplitterFrame (which we should rename accordingly).

Comment 56

[Boris Zbarsky [:bzbarsky]]

I've spun off the first-letter issue that Mats rolled into his original patch into bug 484400.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch per comment 54

Bernd suggested that we should switch the patch and review roles, so here goes.  This patch just switches to what I propose in comment 54.  It includes bernd's reftest, reenables the one existing reftest that was marked "fails" due to this bug, and adds some tests exercising each clause of IsTablePseudo (with the exception of the "tableCell" clause, which can't be tested using kids; I need that for my patch for bug 148810) as well as the clauses for captions and colgroups in MaybeRecreateContainerForFrameRemoval.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Oh, and I plan to add more tests based on the testcases of the various bugs this bug blocks; I just figured I'd start the review rolling first.

Comment 59

[Bernd]

Comment on attachment 368566 [details] [diff] [review]
Proposed patch per comment 54

This is much more elegant than what was attempted before. And OMG this looks so damned easy we could have done this 7 years ago.

+// Return whether the given frame is a table pseudo-frame
Please expand the comment and explain that table cell content and outer table anon boxes are always created and only in a few cases are relally pseudo related. Also the special case for a single caption where we create outer and a empty inner frame should be mentioned. The code will handle this correctly but it is far from obvious. 

// table pseudo-frames, recreate the relevant frme subtree

s/frme/frame/

I would move the testcase that I wrote to the same directory as the other tests the directory was not present when I worked on this. Although that's a proposal not a instruction.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Expanded that comment.  Fixed typo.  Moved tests into the table-anonymous-boxes directory, and put tests for various bugs this blocks in there too.

We couldn't have done this 7 years ago, because the function I'm changing got added a lot later than that (by roc) to fix {ib} issues similar to this.  ;)

Comment 61

[Boris Zbarsky [:bzbarsky]]

OK, pushed <http://hg.mozilla.org/mozilla-central/rev/d0d980778f01>.  Seems to have stuck with no test failures.

Comment 63

[cmtalbert]

Is this something we want to take on 1.9.1 as well?

Setting wanted-? to get the driver's opinion.

Comment 64

[Boris Zbarsky [:bzbarsky]]

Hmm.  This might be reasonably safe, actually.  It doesn't seem to depend on the other parts of frame construction that I changed.  Some of the tests might, of course.

The fix for bug 148810 does, on the other hand.

David, do you think this is enough worth it, without the more-correct handling of insertion from bug 148810, that it's worth taking this patch on branch?  I think it might be, given how long it'll be at this point until we ship gecko > 1.9.1.x

Comment 65

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

I suspect there are a bunch of cases that it would help, e.g., toggling something between 'none' and some other (incorrect?) value of 'display', so I think I'd be in favor of taking it, although I wouldn't call it a blocker.

Comment 66

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch merge

I included some tests that landed before this bug that we're mostly passing on branch; I had to only mark one of those failing.  I did have to mark a bunch of the tests I added for this bug (though none from the dependent bugs) failing too, because we screw up ContentInserted if the parent is a pseudo frame and we did the weird "ship whitespace out to before it" thing.  This patch does fix all cases of people toggling between none and some weird display type in actual tables.

Comment 67

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

Comment on attachment 376969 [details] [diff] [review]
Branch merge

r=dbaron

Comment 68

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 376969 [details] [diff] [review]
Branch merge

It might be worth taking this on branch.  It fixes a number of web compat issues, and we expect more to appear as more sites start using this stuff because IE8 supports it.  There are no known regressions from this fix, and conceptually it's reasonably safe...

Comment 69

[Boris Zbarsky [:bzbarsky]]

Attachment: Actual branch merge (helps to attach from the right mq)

Comment 70

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b4f9568dd47b