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

RESOLVED FIXED in Firefox 51

Status

()

Core
Layout: Floats
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: Claude Pache, Assigned: TYLin)

Tracking

(Blocks: 1 bug)

49 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

10 months ago
Created attachment 8787126 [details]
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.
(Reporter)

Updated

10 months ago
Blocks: 1245021
(Assignee)

Updated

10 months ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 1

10 months ago
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: 1226455
(Reporter)

Updated

10 months ago
Blocks: 1226455
No longer depends on: 1226455
(Assignee)

Comment 2

10 months ago
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
(Reporter)

Comment 3

10 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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+
(Assignee)

Comment 8

10 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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+
(Assignee)

Comment 14

10 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 18

10 months ago
mozreview-review
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)
Comment hidden (obsolete)
> 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.
(Assignee)

Comment 21

10 months ago
> 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+
(Assignee)

Comment 24

10 months ago
mozreview-review-reply
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.
(Assignee)

Comment 25

10 months ago
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+
(Assignee)

Comment 27

10 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 31

10 months ago
All review comments addressed. Boris, thank you for the review.

Comment 32

10 months ago
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

Comment 33

10 months ago
bugherder
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
Last Resolved: 10 months 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.