1472897 - Adapt ::before and ::after on <details> to the spec.

Status: RESOLVED DUPLICATE of bug 1308080

(Core :: Layout, defect, P3)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1472897: Adjust <details> pseudos to the spec.

Remove the summary assertion, because it only asserts that the first frame in a
frame list is a summary, and I don't think it's worth complicating it.

Review commit: https://reviewboard.mozilla.org/r/254376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254376/

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 8989316 [details]
Bug 1472897: Adjust <details> pseudos to the spec.

https://reviewboard.mozilla.org/r/254376/#review261498

Well, as I've said before, I consider the HTML spec for <details> being bogus and the rendering in Chrome a bug.  We should argue for a spec change instead.
Please find another reviewer because I don't want to have any part in this regression.

Comment 3

[Mats Palmgren (inactive)]

Attachment: <fieldset> pseudo test

Compare w. <fieldset>.  I see no reason why <details> should be
different to <fieldset>.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8989316 [details]
Bug 1472897: Adjust <details> pseudos to the spec.

Ting-Yu, Boris, WDYT? See the discussion starting from bug 1460158 comment 5, and https://github.com/whatwg/html/pull/3686, for the different reasoning and arguments.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

If we ever fix bug 1308080, we'll end up with this rendering anyway. I don't think this is the same as <fieldset> because <fieldset> moves the legend out of the normal block flow, unlike <details>.

Comment 6

[Mats Palmgren (inactive)]

Attachment: <summary> vs <legend>

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> because <fieldset> moves the legend out
> of the normal block flow, unlike <details>.

That's simply not true.  <legend> is very much in-flow (by default),
as you can see by putting an abs.pos. child inside it.

Both <legend> and <summary> are similar and special in the sense
that their boxes are placed first, before boxes of content that
precede them in the DOM.  That doesn't make them out-of-flow
boxes though.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) (on vacation) from comment #6)
> Created attachment 8989663 [details]
> <summary> vs <legend>
> 
> (In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> > because <fieldset> moves the legend out
> > of the normal block flow, unlike <details>.
> 
> That's simply not true.  <legend> is very much in-flow (by default),
> as you can see by putting an abs.pos. child inside it.
> 
> Both <legend> and <summary> are similar and special in the sense
> that their boxes are placed first, before boxes of content that
> precede them in the DOM.  That doesn't make them out-of-flow
> boxes though.

Isn't it? The way we model frameset is with {legend, anon block}, and we stash the rest of the content in the anon block:

  https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/layout/base/nsCSSFrameConstructor.cpp#6616

Comment 8

[Mats Palmgren (inactive)]

That's an implementation detail.  It doesn't make the legend box
out-of-flow in the CSS sense of the word.  The fieldset reserves
space for it to make it not overlap content before or after it
so it definitely influences the "flow" of the fieldset, even
though that flow is rather odd compared to say block flow.
(Perhaps better to compare it to Grid which also places its items
at static positions, but that doesn't make the items out-of-flow
either.)

Anyway, <fieldset>/<legend> are actually rather similar to
<details>/<summary> both on a semantic level and on a CSS box
level, so I'd argue that their ::before/::after boxes should work
the same for symmetry reasons alone.

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8989316 [details]
Bug 1472897: Adjust <details> pseudos to the spec.

https://reviewboard.mozilla.org/r/254376/#review262108

::: layout/base/nsCSSFrameConstructor.cpp
(Diff revision 1)
> -  if (canHavePageBreak && display->mBreakAfter) {
> -    AddPageBreakItem(aContent, aItems);
> -  }

Can you elaborate why this is moved?

::: layout/base/nsCSSFrameConstructor.cpp
(Diff revision 1)
> -  if (item->mIsAllInline) {
> -    aItems.InlineItemAdded();
> -  } else if (item->mIsBlock) {
> -    aItems.BlockItemAdded();
> -  }

And why is is not needed?

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I was aware that our implementation renders details::before and details::after differently from Google Chrome, so I filed bug 1307679. I tent to think we manipulate <details>'s frame tree in a funny way in order to move the main summary to be the first child. 

I can see Mats points, but if the spec and web-platform test are leaning towards Chrome's behavior, I don't mind change ours for web-compact reasons (if there are such issues raised). I'll leave the review to bz for the final decision.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #9)
> Comment on attachment 8989316 [details]
> Bug 1472897: Adjust <details> pseudos to the spec.
> 
> https://reviewboard.mozilla.org/r/254376/#review262108
> 
> ::: layout/base/nsCSSFrameConstructor.cpp
> (Diff revision 1)
> > -  if (canHavePageBreak && display->mBreakAfter) {
> > -    AddPageBreakItem(aContent, aItems);
> > -  }
> 
> Can you elaborate why this is moved?

Because I moved the InsertItem call to the end once the FCItem is set up, so AddPageBreakItem should go afterwards.

> ::: layout/base/nsCSSFrameConstructor.cpp
> (Diff revision 1)
> > -  if (item->mIsAllInline) {
> > -    aItems.InlineItemAdded();
> > -  } else if (item->mIsBlock) {
> > -    aItems.BlockItemAdded();
> > -  }
> 
> And why is is not needed?

Because InsertItem already takes care of it, unlike AppendItem.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So... As far as I can tell, what we implement is what the spec says, and what Chrome does is not.  The spec for <details> rendering says:

  The details element is expected to render as a block box. The element's shadow tree is
  expected to take the element's first summary element child, if any, and place it in a
  first block box container, and then take the element's remaining descendants, if any,
  and place them in a second block box container.

  The second container is expected to be removed from the rendering when the details
  element does not have an open attribute.

So that corresponds to a shadow DOM with two slots: the first takes the "summary" and the second takes everything else, including the ::before and ::after, right?  And when "open" is not set the ::before and ::after are not visible, since they're in the second container.

What spec is this attempting to align with, exactly, and what is the behavior it's trying to produce?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

The behavior it's trying to produce is Chrome's.


(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> So that corresponds to a shadow DOM with two slots: the first takes the
> "summary" and the second takes everything else, including the ::before and
> ::after, right?

This is the only wrong bit in what you just said. ::before and ::after are not slotted, and thus should be displayed.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Or is the point that ::before and ::after are not affected by slots in the flattened tree?

If so, then I agree with Mats: the asymmetry in how fieldset and details are defined (the former in terms of actual rendering, the latter in terms of shadow DOM) is just broken, probably because the case of ::before/::after wasn't considered (or because the spec writer expected shadow DOM to handle them differently).  We should keep our behavior and raise a spec issue on this.

Alternately, if we decide we really do want the Chrome/Safari rendering (though I have no idea why we would), we should implement it in terms of the spec's shadow DOM model, not by doing more/different frame munging.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

See also https://bug1460158.bmoattachments.org/attachment.cgi?id=8975527, which is a test-case using our Shadow DOM implementation. I'm fine waiting for bug 1308080 to align with the spec instead of this patch.

If you really think it's that broken a spec issue may be better?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I think it's pretty broken...  I definitely think the asymmetry with fieldset is wrong.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8989316 [details]
Bug 1472897: Adjust <details> pseudos to the spec.

Alright, cancelling for now then. Boris, any chance you could file the spec issue? I think you're in a better position than me to explain why it's broken. Thanks!

Comment 19

[Boris Zbarsky [:bzbarsky]]

I filed https://github.com/whatwg/html/issues/3805

Comment 20

[Emilio Cobos Álvarez (:emilio)]

The discussion in there didn't end up with a spec change and is probably not the hill to die on.

I fixed <details> to follow the spec more closely in bug 1308080, even tho it has this unfortunate side-effect.