Closed Bug 1299753 Opened 7 years ago Closed 7 years ago

<details> with float: left has an incorrect computed height when preceded with other blocks with float: left

Categories

(Core :: Layout: Floats, defect)

Product:
Core
Component:
Layout: Floats
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: claude.pache, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

details-float.html
1.09 KB, text/html
Details
Bug 1299753 Part 2 - Create block formatting context for DetailsFrame if needed.
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1299753 Part 1 - Use NS_NewBlockFormattingContext() in ConstructFieldSetFrame().
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1299753 Part 3 - Reuse ConstructScrollableBlock to build scrollable DetailsFrame.
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Attached file details-float.html — 
See the testcase attached.

A <details> block with float:left is too tall when it is preceded (not necessarily immediately) with other blocks with float: left.
Blocks: details-summary
Status: UNCONFIRMED → NEW
Ever confirmed: true
Since this bug may degrade existing web pages (it did for a web app I'm working on, although I now use a workaround), you should consider if you want to defer shipping <details> until this bug is resolved.
Depends on: ship-details-summary
Blocks: ship-details-summary
No longer depends on: ship-details-summary
Thank you for reporting this bug with a test case. My guess is that your workaround is wrapping the details element in a normal float div instead of declare the details element as a float.
Assignee: nobody → tlin
Status: NEW → ASSIGNED
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #2)
> My guess is that your
> workaround is wrapping the details element in a normal float div instead of
> declare the details element as a float.

You're right.
Comment on attachment 8788096 [details]
Bug 1299753 Part 2 - Create block formatting context for DetailsFrame if needed.

https://reviewboard.mozilla.org/r/76672/#review75134

I see no indication in the spec that `<details>` should be a block formatting context root.  Am I just missing it?  Is it in other browsers?
Attachment #8788096 - Flags: review?(bzbarsky) → review-
Comment on attachment 8788097 [details]
Bug 1299753 Part 1 - Use NS_NewBlockFormattingContext() in ConstructFieldSetFrame().

https://reviewboard.mozilla.org/r/76674/#review75136

r=me
Attachment #8788097 - Flags: review?(bzbarsky) → review+
Comment on attachment 8788096 [details]
Bug 1299753 Part 2 - Create block formatting context for DetailsFrame if needed.

https://reviewboard.mozilla.org/r/76672/#review75134

I'm wrong.  <details> should not be a block formatting context root unconditionally, but it should be one if it has float style, etc. as described in CSS 2.2 section 9.4.1.  Google Chrome renders the float <details> correctly in the reporter's test cases.
https://www.w3.org/TR/CSS22/visuren.html#block-formatting
Comment on attachment 8788096 [details]
Bug 1299753 Part 2 - Create block formatting context for DetailsFrame if needed.

https://reviewboard.mozilla.org/r/76672/#review75470

::: layout/base/nsCSSFrameConstructor.h:1563
(Diff revision 2)
>  
>    /**
>     * Construct a non-scrollable block frame
>     */
>    nsIFrame* ConstructNonScrollableBlock(nsFrameConstructorState& aState,
> -                                        FrameConstructionItem&   aItem,
> +                                        FrameConstructionItem& aItem,

Why the random whitespace changes?

::: layout/base/nsCSSFrameConstructor.h:1569
(Diff revision 2)
> -                                        nsContainerFrame*        aParentFrame,
> -                                        const nsStyleDisplay*    aDisplay,
> -                                        nsFrameItems&            aFrameItems);
> +                                        nsContainerFrame* aParentFrame,
> +                                        const nsStyleDisplay* aDisplay,
> +                                        nsFrameItems& aFrameItems);
> +
> +  /**
> +   * Construct a non-scrollable block for the given aFrameToConstruct.

How about having an overload of ConstructNonScrollableBlock (possibly with a different name) that takes a factory function as an argument?  Then you'd pass NS_NewBlockFrame from the current ConstructNonScrollableBlock and NS_NewDetailsFrame from the details code. You'll need to change NS_NewDetailsFrame to return nsBlockFrame*, but I think that's fine.

I'm probably also OK with your setup, but then I think the function should be called ConstructExistingNonScrollableBlock and the nsBlockFrame* arg should be aBlockToConstruct....

::: layout/base/nsCSSFrameConstructor.cpp:3386
(Diff revision 2)
>  
>      aState.AddChild(scrollFrame, aFrameItems, content, styleContext,
>                      aParentFrame);
>  
>      nsFrameItems scrollFrameItems;
> +    nsContainerFrame* newFrame = detailsFrame;

No, this is wrong.  I'm surprised the tests passed.

When there are column styles on the details, ConstructBlock will mutate its inout param (in this case newFrame).  So the assert that scrollFrameItems.OnlyChild() == detailsFrame should be failing for scrollable columnated details.  Are those just not tested?

Please undo this change (which isn't needed anyway, if you reorder as I suggest below) and add tests that would have caught it.

::: layout/base/nsCSSFrameConstructor.cpp:3399
(Diff revision 2)
>  
>      FinishBuildingScrollFrame(scrollFrame, detailsFrame);
>  
>      frameToReturn = scrollFrame;
>    } else {
> -    ConstructBlock(aState, content, geometricParent, aParentFrame, styleContext,
> +    frameToReturn = ConstructNonScrollableBlockForFrame(

If you're not using the geometric parent and whatnot, then I think you should just have this function start off with:

    if (!aStyleDisplay->IsScrollableOverflow()) {
      nsBlockFrame* detailsFrame = NS_NewDetailsFrame(mPresShell, styleContext);
      return ConstructNonScrollableBlockForFrame(aState, aItem, aParentFrame,
      aStyleDisplay, aFrameItems, detailsFrame);
    }
    
    // Now do scrollframe stuff
    
or something.

::: layout/generic/DetailsFrame.cpp:26
(Diff revision 2)
>    NS_QUERYFRAME_ENTRY(DetailsFrame)
>    NS_QUERYFRAME_ENTRY(nsIAnonymousContentCreator)
>  NS_QUERYFRAME_TAIL_INHERITING(nsBlockFrame)
>  
>  DetailsFrame*
> -NS_NewDetailsFrame(nsIPresShell* aPresShell, nsStyleContext* aContext)
> +NS_NewDetailsFrame(nsIPresShell* aPresShell, nsStyleContext* aContext,

Why this change?  The third arg is not used by anyone....

::: layout/generic/nsBlockFrame.h:343
(Diff revision 2)
>     */
>    nsresult SplitFloat(BlockReflowInput& aState,
>                        nsIFrame*           aFloat,
>                        nsReflowStatus      aFloatStatus);
>  
> +  void SetFlags(nsFrameState aFlags) {

Why did this get moved?  I see no reason to do that.

::: layout/reftests/details-summary/reftest.list:53
(Diff revision 2)
>  == details-page-break-after-1.html details-two-pages.html
>  == details-page-break-after-2.html details-two-pages.html
>  == details-page-break-before-1.html details-two-pages.html
>  == details-page-break-before-2.html details-two-pages.html
>  
> +# With 'float' property

I think you should add the following test: a float:left open details containing a float:left block, with the inner block assigned some fixed size.

I expect that without your patch the height of the details will not be large enough to contain the floating child, but with the patch it will (correctly) stretch vertically to contain it.
Attachment #8788096 - Flags: review?(bzbarsky) → review-
Comment on attachment 8788791 [details]
Bug 1299753 Part 3 - Reuse ConstructScrollableBlock to build scrollable DetailsFrame.

https://reviewboard.mozilla.org/r/77158/#review75478

::: layout/base/nsCSSFrameConstructor.cpp
(Diff revision 1)
>                                               const nsStyleDisplay* aStyleDisplay,
>                                               nsFrameItems& aFrameItems)
>  {
> -  nsIContent* const content = aItem.mContent;
> +  DetailsFrame* detailsFrame = NS_NewDetailsFrame(mPresShell, aItem.mStyleContext);
> -  nsStyleContext* const styleContext = aItem.mStyleContext;
> -  nsContainerFrame* geometricParent =

Ah, I see.  The broken code from the previous patch more or less goes away here, which is presumably why you didn't get test failures if you tested the whole queue at once.

::: layout/base/nsCSSFrameConstructor.cpp:4827
(Diff revision 1)
> +  return ConstructScrollableBlockForFrame(aState, aItem, aParentFrame, aDisplay,
> +                                          aFrameItems, frameToConstruct);
> +}
> +
> +nsIFrame*
> +nsCSSFrameConstructor::ConstructScrollableBlockForFrame(

Again, the naming is weird.  Should be more like ConstructScrollableBlockWithGivenScrolledFrame and the last arg should be named aScrolledFrame.

And it needs some comments about how the style context used to construct the frame in the caller is just temporary and this function will mutate the style context or something.

::: layout/base/nsCSSFrameConstructor.cpp:4857
(Diff revision 1)
>    ConstructBlock(aState, content, newFrame, newFrame, scrolledContentStyle,
>                   &scrolledFrame, blockItem,
>                   aDisplay->IsAbsPosContainingBlock(newFrame) ? newFrame : nullptr,
>                   aItem.mPendingBinding);
>  
>    NS_ASSERTION(blockItem.FirstChild() == scrolledFrame,

While you're here, make this:

    MOZ_ASSERT(blockItem.OnlyChild() == scrolledFrame,
               "Scrollframe's frameItems should be exactly the scrolled frame");
               
?

r=me with those bits fixed.
Attachment #8788791 - Flags: review?(bzbarsky) → review+
Comment on attachment 8788096 [details]
Bug 1299753 Part 2 - Create block formatting context for DetailsFrame if needed.

https://reviewboard.mozilla.org/r/76672/#review75470

> Why the random whitespace changes?

I'd like to make the argument alignment consistent with the new function I add, but I can change it back if you prefer not to touch it since it could destroy the blame history.

> How about having an overload of ConstructNonScrollableBlock (possibly with a different name) that takes a factory function as an argument?  Then you'd pass NS_NewBlockFrame from the current ConstructNonScrollableBlock and NS_NewDetailsFrame from the details code. You'll need to change NS_NewDetailsFrame to return nsBlockFrame*, but I think that's fine.
> 
> I'm probably also OK with your setup, but then I think the function should be called ConstructExistingNonScrollableBlock and the nsBlockFrame* arg should be aBlockToConstruct....

I like the idea to passing the factory to ConstructNonScrollableBlock. I'll make the change in the next patchset.

> No, this is wrong.  I'm surprised the tests passed.
> 
> When there are column styles on the details, ConstructBlock will mutate its inout param (in this case newFrame).  So the assert that scrollFrameItems.OnlyChild() == detailsFrame should be failing for scrollable columnated details.  Are those just not tested?
> 
> Please undo this change (which isn't needed anyway, if you reorder as I suggest below) and add tests that would have caught it.

I'll undo this with your suggestion below, and make sure the test pass locally without part 3.

> Why this change?  The third arg is not used by anyone....

This is not needed. Thanks for catching this.

> Why did this get moved?  I see no reason to do that.

SetFlags() is called by ConstructNonScrollableBlockForFrame, which needs to be be public. RemoveStateBits() and AddStateBits() are all public so it should be reasonable to move it.

> I think you should add the following test: a float:left open details containing a float:left block, with the inner block assigned some fixed size.
> 
> I expect that without your patch the height of the details will not be large enough to contain the floating child, but with the patch it will (correctly) stretch vertically to contain it.

Will add the test cases in my next patchset. And yes, with my patch, the details does stretch the height to containing the tall floating child.
Comment on attachment 8788791 [details]
Bug 1299753 Part 3 - Reuse ConstructScrollableBlock to build scrollable DetailsFrame.

https://reviewboard.mozilla.org/r/77158/#review75478

> Again, the naming is weird.  Should be more like ConstructScrollableBlockWithGivenScrolledFrame and the last arg should be named aScrolledFrame.
> 
> And it needs some comments about how the style context used to construct the frame in the caller is just temporary and this function will mutate the style context or something.

I preserve the comment in my next patchset since now the new function accepts a block frame constructor function.
Comment on attachment 8788791 [details]
Bug 1299753 Part 3 - Reuse ConstructScrollableBlock to build scrollable DetailsFrame.

https://reviewboard.mozilla.org/r/77158/#review75810

Part 3 might need a re-review due to a different approach.
Attachment #8788791 - Flags: review?(tlin)
> but I can change it back if you prefer not to touch it since it could destroy the blame history.

Please change it back, yes.

> SetFlags() is called by ConstructNonScrollableBlockForFrame, which needs to be be public.

Why are we calling SetFlags at all?  It's a utility method designed for flag propagation to continuations during block reflow that does some _very_ specific things that may not be appropriate to other situations.  There's absolutely no reason to call it during initial frame construction; just call AddStateBits.
> Why are we calling SetFlags at all?  It's a utility method designed for flag
> propagation to continuations during block reflow that does some _very_
> specific things that may not be appropriate to other situations.  There's
> absolutely no reason to call it during initial frame construction; just call
> AddStateBits.

Some frame construction functions NS_NewXXXFrame call SetFlags() [1], and I just follow them.  I'll change my patch by using AddStateBits.

Now I see SetFlags() is designed for nsBlockFrame::Init() [2], which is the only caller other than initial frame constructions. To avoid other misuses, perhaps we need another patch to expand SetFlags() directly in nsBlockFrame::Init() and change other call sites to AddStateBits.

[1] http://searchfox.org/mozilla-central/search?q=symbol:_ZN12nsBlockFrame8SetFlagsE12nsFrameState&redirect=false
[2] http://searchfox.org/mozilla-central/rev/124c120ce9d7e8407c9ad330a9d6b1c4ad432637/layout/generic/nsBlockFrame.cpp#6794-6796
Comment on attachment 8788096 [details]
Bug 1299753 Part 2 - Create block formatting context for DetailsFrame if needed.

https://reviewboard.mozilla.org/r/76672/#review76088

::: layout/base/nsCSSFrameConstructor.h:1570
(Diff revision 3)
>                                          nsContainerFrame*        aParentFrame,
>                                          const nsStyleDisplay*    aDisplay,
>                                          nsFrameItems&            aFrameItems);
>  
>    /**
> +   * Construct a non-scrollable block frame by the given block frame creation

s/by/using/, please.

::: layout/base/nsCSSFrameConstructor.h:1573
(Diff revision 3)
>  
>    /**
> +   * Construct a non-scrollable block frame by the given block frame creation
> +   * function.
> +   */
> +  nsIFrame* ConstructNonScrollableBlockByConstructor(

s/By/With/ in the function name.

::: layout/base/nsCSSFrameConstructor.cpp:4916
(Diff revision 3)
> -  } else {
> -    newFrame = NS_NewBlockFrame(mPresShell, styleContext);
>    }
>  
> +  nsBlockFrame* blockFrame = aConstructor(mPresShell, styleContext);
> +  blockFrame->SetFlags(flags);

This should use AddStateBits, not SetFlags, per comments in the bug.  And then you don't need the blockFrame thing.  Just assign directly to newFrame.

::: layout/generic/nsBlockFrame.h:342
(Diff revision 3)
>     */
>    nsresult SplitFloat(BlockReflowInput& aState,
>                        nsIFrame*           aFloat,
>                        nsReflowStatus      aFloatStatus);
>  
> +  void SetFlags(nsFrameState aFlags) {

And this shouldn't move.

::: layout/generic/nsBlockFrame.cpp:293
(Diff revision 3)
> +nsBlockFrame*
> +NS_NewBlockFormattingContext(nsIPresShell* aPresShell,
> +                             nsStyleContext* aStyleContext)
> +{
> +  nsBlockFrame* blockFrame = NS_NewBlockFrame(aPresShell, aStyleContext);
> +  blockFrame->SetFlags(NS_BLOCK_FLOAT_MGR | NS_BLOCK_MARGIN_ROOT);

This too can use AddStateBits.

::: layout/reftests/details-summary/float-open-details-contains-float-left.html:30
(Diff revision 3)
> +  </style>
> +  <body>
> +    <details open>
> +      <summary>Summary</summary>
> +      <div id="float"></div>
> +      Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod

I think this test would be more reliable if it didn't have the text block in the details, so that the float would more reliably be "too tall".  Same for the other details-contains-float tests.

r=me with those nits fixed.  Thank you!
Attachment #8788096 - Flags: review?(bzbarsky) → review+
Comment on attachment 8788791 [details]
Bug 1299753 Part 3 - Reuse ConstructScrollableBlock to build scrollable DetailsFrame.

https://reviewboard.mozilla.org/r/77158/#review76096

Did you mean to request review on this part from me or from yourself?
Attachment #8788791 - Flags: review+
Comment on attachment 8788791 [details]
Bug 1299753 Part 3 - Reuse ConstructScrollableBlock to build scrollable DetailsFrame.

https://reviewboard.mozilla.org/r/77158/#review76096

I'd like you to review it again. Thanks.
Comment on attachment 8788791 [details]
Bug 1299753 Part 3 - Reuse ConstructScrollableBlock to build scrollable DetailsFrame.

mozreview won't let me change part 3 to r? ...
Attachment #8788791 - Flags: review?(bzbarsky)
Comment on attachment 8788791 [details]
Bug 1299753 Part 3 - Reuse ConstructScrollableBlock to build scrollable DetailsFrame.

https://reviewboard.mozilla.org/r/77158/#review76104

::: layout/base/nsCSSFrameConstructor.h:1561
(Diff revision 2)
>                                       nsContainerFrame*        aParentFrame,
>                                       const nsStyleDisplay*    aDisplay,
>                                       nsFrameItems&            aFrameItems);
>  
>    /**
> +   * Construct a scrollable block frame by the given block frame creation

s/by/using/

::: layout/base/nsCSSFrameConstructor.h:1564
(Diff revision 2)
>  
>    /**
> +   * Construct a scrollable block frame by the given block frame creation
> +   * function.
> +   */
> +  nsIFrame* ConstructScrollableBlockByConstructor(

s/By/With/

r=me
Attachment #8788791 - Flags: review?(bzbarsky) → review+
Comment on attachment 8788096 [details]
Bug 1299753 Part 2 - Create block formatting context for DetailsFrame if needed.

https://reviewboard.mozilla.org/r/76672/#review76088

> I think this test would be more reliable if it didn't have the text block in the details, so that the float would more reliably be "too tall".  Same for the other details-contains-float tests.

You're right. This makes the tests more robost against the font size.
All review comments addressed. Boris, thank you for the review.
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d8754d431f3
Part 1 - Use NS_NewBlockFormattingContext() in ConstructFieldSetFrame(). r=bz
https://hg.mozilla.org/integration/autoland/rev/31f1b0aa5308
Part 2 - Create block formatting context for DetailsFrame if needed. r=bz
https://hg.mozilla.org/integration/autoland/rev/37577db30368
Part 3 - Reuse ConstructScrollableBlock to build scrollable DetailsFrame. r=bz
https://hg.mozilla.org/mozilla-central/rev/8d8754d431f3
https://hg.mozilla.org/mozilla-central/rev/31f1b0aa5308
https://hg.mozilla.org/mozilla-central/rev/37577db30368
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.