1297835 - Figure out why we're reframing scrollcorners on https://drafts.csswg.org/css-values-3/

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Loading https://drafts.csswg.org/css-values-3/ hits:

Assertion failure: aChild->GetProperty(nsGkAtoms::restylableAnonymousNode) (Someone passed native anonymous content directly into frame construction.  Stop doing that!)

aChild is a scrollcorner for a scrollframe corresponding to a <details> element.  I need to figure out why we're trying to reframe such a thing and whether we should really be doing that.

Comment 1

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

(This was split out from bug 1250725.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

We seem to have an undisplayed node for the scrollcorner.  We land under ElementRestyler::RestyleUndisplayedNodes with aDisplay set to NS_STYLE_DISPLAY_NONE (called from mozilla::ElementRestyler::DoRestyleUndisplayedDescendants for the <details> element).  In particular, we have undisplayed nodes for both a scrollbar and a scrollcorner.  The style context stored in the undisplayed node has NS_STYLE_DISPLAY_BOX as mDisplay for both.  We resolve a new style context and that _also_ has NS_STYLE_DISPLAY_BOX.  This does not equal aDisplay, so we try to reframe.

It's a bit weird that we try to reframe even though the display value did not change from the thing we had _last_ time...

Anyway, we already flag scrollbar as being reframeable for totally bogus-ish reasons (theme changes); but the scrollcorner is not thus flagged and hence asserts.

The reason the scrollcorner is undisplayed is that we have this special case for details in nsCSSFrameConstructor::AddFrameConstructionItemsInternal:

  // When constructing a child of a non-open <details>, create only the frame
  // for the main <summary> element, and skip other elements.
  auto* details = HTMLDetailsElement::FromContentOrNull(parent);
  if (details && details->IsDetailsEnabled() && !details->Open()) {
    auto* summary = HTMLSummaryElement::FromContentOrNull(aContent);
    if (!summary || !summary->IsMainSummary()) {
      SetAsUndisplayedContent(aState, aItems, aContent, styleContext,
                              isGeneratedContent);
      return;
    }
  }

Applying that to the scrollbar/details of the scrollframe seems bogus to me.  In particular, consider this testcase:

  <details style="display: block; overflow: scroll; width: 200px; height: 200px;
                  border: 1px solid black">
    <summary style="display: block; width: 300px; height: 300px;
                    border: 2px solid purple">Some summary</summary>
    The details
  </details>

Should this show the scrollbar even while the details is not open?  Seems to be like it should, but we currently do not.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal testcase for the assertion

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't suppress the scroll bits of a non-open <details> that has the appropriate overflow styles

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8784598 [details] [diff] [review]
Don't suppress the scroll bits of a non-open <details> that has the appropriate overflow styles

No, this is wrong, because it lets through ::before and ::after....

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't suppress the scroll bits of a non-open <details> that has the appropriate overflow styles

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8784652 [details] [diff] [review]
Don't suppress the scroll bits of a non-open <details> that has the appropriate overflow styles

Review of attachment 8784652 [details] [diff] [review]:
-----------------------------------------------------------------

Yes. We should display the scrollbar for details with overflow:scroll style even when the details is not open.

This patch looks good to me.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75e8d28abce1
Don't suppress the scroll bits of a non-open <details> that has the appropriate overflow styles.  r=TYLin

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75e8d28abce1