Open Bug 1472897 Opened Last year Updated Last year

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

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

People

(Reporter: emilio, Unassigned)

References

Details

Attachments

(3 files)

No description provided.
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.
Attachment #8989316 - Flags: review?(mats)
Attached file <fieldset> pseudo test
Compare w. <fieldset>.  I see no reason why <details> should be
different to <fieldset>.
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.
Attachment #8989316 - Flags: review?(bzbarsky)
Attachment #8989316 - Flags: review?(aethanyc)
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>.
See Also: → 1308080
Attached file <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.
(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
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 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?
Attachment #8989316 - Flags: review?(aethanyc)
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.
Duplicate of this bug: 1307679
(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.
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?
Flags: needinfo?(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.
Flags: needinfo?(emilio)
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.
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?
I think it's pretty broken...  I definitely think the asymmetry with fieldset is wrong.
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!
Attachment #8989316 - Flags: review?(bzbarsky)
Priority: -- → P3
Assignee: emilio → nobody
You need to log in before you can comment on or make changes to this bug.