522632 - [FIX]cellContent blocks shouldn't be IsContainingBlock

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

This makes some of the code in the reflow state more correct, I think, and simplifies other code there....

Comment 1

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

What does this make more correct?  Percentage widths inside border-collapse cells?  I guess more correct per spec, but also completely useless?

Comment 2

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

Comment on attachment 406620 [details] [diff] [review]
Fix

> nsIFrame*
> nsHTMLReflowState::GetHypotheticalBoxContainer(nsIFrame* aFrame,
>                                                nscoord& aCBLeftEdge,
>                                                nscoord& aCBWidth)
> {
>   do {
>     aFrame = aFrame->GetParent();
>     NS_ASSERTION(aFrame, "Must find containing block somewhere");
>+    // XXXbz why do we want to stop at random non-containing-block
>+    // blockframes here?
>   } while (!(aFrame->IsContainingBlock() ||
>-             (aFrame->IsFrameOfType(nsIFrame::eBlockFrame) &&
>-              IsAnonBlockPseudo(aFrame->GetStyleContext()->GetPseudoType()))));
>+             aFrame->IsFrameOfType(nsIFrame::eBlockFrame)));

I think this change makes the problem you're commenting on worse; before the change, we stopped only at containing blocks or anonymous blocks for block-within-inline splits; after the change we stop at containing blocks or any blockframe.  The latter should always have the same width as their parent, so I *think* the old code was functionally the same as just an IsContainingBlock check, but the new code isn't.


Also, it would be good to have tests for whatever it is you're fixing (is it the border-collapse case I mention in the previous comment?).

Comment 3

[Boris Zbarsky [:bzbarsky]]

> What does this make more correct? 

I just checked the IsContainingBlock() users in the tree, and it makes the following more correct:

1)  Auto-width image with percentage height inside a table cell with non-auto
    height (fixes the intrinsic width computation; will add a test).  The
    relevant IsContainingBlock() callsite is GetPercentHeight in
    nsLayoutUtils.cpp.
2)  Computed style values for various properties for kids of table cells (e.g.
    in the following testcase:

      <td style="height: 200px">
        <span style="height: 0; min-height: 50%" id="foo"></span>
      </td>

    we go from returning something around 52px to 98px for the computed height
    of the span, which is correct given the border-box sizing table cells do).
    I can add tests as desired.  These are calling
    nsHTMLReflowState::GetContainingBlockFor.

Not sure about these (hence the "I think" in comment 0):

1)  The mVResize computation in InitResizeFlags.
2)  NS_FRAME_CONTAINS_RELATIVE_HEIGHT propagation in InitResizeFlags

For the remaining callsites it doesn't matter what the return value is here (since both the cell block and the cell have the same direction and since the cell block never has a block-height-dependent line-height).

> before the change, we stopped only at containing blocks or anonymous blocks
> for block-within-inline splits

Right.  And all blocks that were not anonymous blocks for block-within-inline splits returned true from IsContainingBlock(), so we stopped for all blocks plus any non-blocks that return true from IsContainingBlock, right?  That's exactly what the new code does, except a bit less confusingly.

Now maybe we shouldn't stop at the cell-content block, though we used to.  In practice it doesn't matter right now because cells are never absolute containing blocks yet.  But you're right that we shouldn't be stopping at the cell-content when they do become absolute containing blocks; I'll make this check for just IsContainingBlock.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with tests and better GetHypotheticalBoxContainer

Comment 5

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

Comment on attachment 415198 [details] [diff] [review]
Patch with tests and better GetHypotheticalBoxContainer

r=dbaron.  Sorry for the long delay.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/c99d708e789d