Boris Zbarsky [:bzbarsky] Assignee
	Description • 8 years ago

This should make bug 1292609 simpler, because we won't need to change the style context of placeholder frames at all.

	David Baron :dbaron:
	Comment 10 • 8 years ago
mozreview-review

Comment on attachment 8842329 [details]
Bug 1343078 part 1.  Give placeholders and first-letter continuations different kinds of anonymous boxes.

https://reviewboard.mozilla.org/r/116204/#review119792

r=dbaron with the following comments

::: layout/style/ServoStyleSet.cpp:260
(Diff revision 1)
> +}
> +
> +already_AddRefed<nsStyleContext>
> +ServoStyleSet::ResolveStyleForPlaceholder(nsStyleContext* aParentContext)
> +{
> +  // The parent context can be null if the placer's element is a root element.

placer -> placeholder

::: layout/style/nsCSSAnonBoxList.h:22
(Diff revision 1)
>   */
>  
>  // OUTPUT_CLASS=nsCSSAnonBoxes
>  // MACRO_NAME=CSS_ANON_BOX
>  
> -// ::-moz-text and ::-moz-other-non-element are non-elements which no
> +// ::-moz-text, ::-moz-placeholder, and ::-moz-first-letter-continuation are

:-moz-placeholder-anon-box

Although I wonder if :-moz-oof-placeholder is better...

::: layout/style/nsCSSAnonBoxList.h:26
(Diff revision 1)
>  
> -// ::-moz-text and ::-moz-other-non-element are non-elements which no
> -// rule will match.
> +// ::-moz-text, ::-moz-placeholder, and ::-moz-first-letter-continuation are
> +// non-elements which no rule will match.
>  CSS_ANON_BOX(mozText, ":-moz-text")
> -// This anonymous box has two uses:
> -// 1. placeholder frames,
> +// placeholder frames.  Note that :-moz-placeholder is already taken by the
> +// corresponding pseudo-element, so we need a different string here.

"corresponding" here is misleading; it's for <input type="placeholder">

::: layout/style/nsCSSAnonBoxList.h:27
(Diff revision 1)
> -// ::-moz-text and ::-moz-other-non-element are non-elements which no
> -// rule will match.
> +// ::-moz-text, ::-moz-placeholder, and ::-moz-first-letter-continuation are
> +// non-elements which no rule will match.
>  CSS_ANON_BOX(mozText, ":-moz-text")
> -// This anonymous box has two uses:
> -// 1. placeholder frames,
> -// 2. nsFirstLetterFrames for content outside the ::first-letter.
> +// placeholder frames.  Note that :-moz-placeholder is already taken by the
> +// corresponding pseudo-element, so we need a different string here.
> +CSS_ANON_BOX(placeholder, ":-moz-placeholder-anon-box")

I'm a little leery of having these two names be so different.  That leads to confusion later.  I'd prefer having the nsCSSAnonBoxes member have a name that more closely matches the textual CSS.

	David Baron :dbaron:
	Comment 11 • 8 years ago
mozreview-review

Comment on attachment 8842330 [details]
Bug 1343078 part 2.  Restrict the "is a root" blockification behavior in nsStyleContext to non-pseudos and a limited set of anonymous boxes that want the behavior.

https://reviewboard.mozilla.org/r/116206/#review119794

::: layout/style/nsStyleContext.cpp:828
(Diff revision 1)
> +  const nsStyleDisplay* disp = StyleDisplay();
> +

I don't follow why you're moving this up.  (Did it belong in another patch?)

::: layout/style/nsStyleContext.cpp:871
(Diff revision 1)
> +      // inherit from anything.  So restrict blockification only to actual
> +      // elements, the viewport (which should be block anyway, but in SVG
> +      // document's isn't because we lazy-load ua.css there), and the ::backdrop

er, yikes.  Does this affect any of the other near-root pseudos?  (e.g., :-moz-canvas, :-moz-page, :-moz-pagecontent, :-moz-page-sequence, :-moz-scrolled-content, :-moz-scrolled-canvas, :-moz-scrolled-page-sequence?)  Or are they all non-root?  (I checked :-moz-viewport-scroll since it seemed most worrying.)

	David Baron :dbaron:
	Comment 12 • 8 years ago
mozreview-review

Comment on attachment 8842331 [details]
Bug 1343078 part 3.  Introduce the concept of non-inheriting anon boxes without adding any yet.

https://reviewboard.mozilla.org/r/116208/#review119796

	David Baron :dbaron:
	Comment 13 • 8 years ago
mozreview-review

Comment on attachment 8842332 [details]
Bug 1343078 part 4.  Add storage for the singleton non-inheriting anon box style contexts on style set.

https://reviewboard.mozilla.org/r/116210/#review119798

::: layout/style/ServoStyleSet.h:269
(Diff revision 1)
>                    nsTArray<RefPtr<ServoStyleSheet>>> mSheets;
>    int32_t mBatching;
>  
> +  // Stores pointers to our cached style contexts for non-inheriting anonymous
> +  // boxes.
> +  RefPtr<nsStyleContext> mNonInheritingStyleContexts[static_cast<nsCSSAnonBoxes::NonInheritingBase>(nsCSSAnonBoxes::NonInheriting::_Count)];

does this actually compile when \_Count is 0?  (If it doesn't... maybe it doesn't matter?)

::: layout/style/nsStyleSet.cpp:313
(Diff revision 1)
> +  // Clear our cached style contexts for non-inheriting anonymous boxes.
> +  ClearNonInheritingStyleContexts();
> +  

I'm a little disturbed that there's no servo equivalent...

::: layout/style/nsStyleSet.cpp:315
(Diff revision 1)
>    // held on to after the rule tree has been reconstructed.
>    PresContext()->PresShell()->ClearArenaRefPtrs(eArenaObjectID_nsStyleContext);
>  
> +  // Clear our cached style contexts for non-inheriting anonymous boxes.
> +  ClearNonInheritingStyleContexts();
> +  

trailing whitespace

::: layout/style/nsStyleSet.cpp:2631
(Diff revision 1)
> +void
> +nsStyleSet::ClearNonInheritingStyleContexts()
> +{
> +  for (RefPtr<nsStyleContext>& ptr : mNonInheritingStyleContexts) {
> +    ptr = nullptr;
> +  }  

trailing whitespace

	David Baron :dbaron:
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8842333 [details]
Bug 1343078 part 5.  Change the restyle manager to handle style contexts with no parent in ReparentStyleContext (by doing nothing with them).

https://reviewboard.mozilla.org/r/116212/#review119800

::: layout/base/GeckoRestyleManager.cpp:869
(Diff revision 1)
> +
> +  if (!newParentContext && !oldContext->GetParent()) {
> +    // No need to do anything here.
> +    return NS_OK;
> +  }
> +

Perhaps some of the continuation-related assertions should be retained here?

Also perhaps an assertion that there aren't any child frames?  Since if we were to use this on something that had child frames, we'd presumably need to go through the code below that traverses the children (which would likely be parented to an ancestor)?


Also -- I'm a little disturbed again that there isn't an equivalent change on the stylo side.

	David Baron :dbaron:
	Comment 15 • 8 years ago
mozreview-review

Comment on attachment 8842334 [details]
Bug 1343078 part 6.  Change nsFrame::CorrectStyleParentFrame to return null if we're dealing with a non-inheriting anon box.

https://reviewboard.mozilla.org/r/116214/#review119802

	David Baron :dbaron:
	Comment 16 • 8 years ago
mozreview-review

Comment on attachment 8842335 [details]
Bug 1343078 part 7.  Make placeholders a non-inheriting anon box.

https://reviewboard.mozilla.org/r/116216/#review119804

::: layout/style/ServoStyleSet.cpp:269
(Diff revision 2)
>  }
>  
>  already_AddRefed<nsStyleContext>
> -ServoStyleSet::ResolveStyleForPlaceholder(nsStyleContext* aParentContext)
> +ServoStyleSet::ResolveStyleForPlaceholder()
>  {
> -  // The parent context can be null if the placer's element is a root element.
> +  RefPtr<nsStyleContext>& existing =

Maybe clearer to call this "cache" rather than "existing"?

::: layout/style/nsStyleSet.cpp:1846
(Diff revision 2)
>  }
>  
>  already_AddRefed<nsStyleContext>
> -nsStyleSet::ResolveStyleForPlaceholder(nsStyleContext* aParentContext)
> +nsStyleSet::ResolveStyleForPlaceholder()
>  {
> -  return GetContext(aParentContext, mRuleTree, nullptr,
> +  RefPtr<nsStyleContext>& existing =

same here

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 17 • 8 years ago

> placer -> placeholder

Er, yes, fixed.

> Although I wonder if :-moz-oof-placeholder is better...

Good idea.  Done.

> "corresponding" here is misleading; it's for <input type="placeholder">
> I'm a little leery of having these two names be so different.

OK.  There is no textual CSS for the placeholder case, but I can certainly make the names more similar.  Specifically, this part now says:

  // placeholder frames for out of flows.  Note that :-moz-placeholder is used for
  // the pseudo-element that represents the placeholder text in <input
  // placeholder="foo">, so we need a different string here.
  CSS_ANON_BOX(oofPlaceholder, ":-moz-oof-placeholder")

> I don't follow why you're moving this up. 

Oh, I had added an assert in the mPseudoTag == nsCSSAnonBoxes::viewport block that used the nsStyleDisplay, but that assert turned out to not hold for SVG documents (which is part of what the comment I added is about).  I forgot to move it back.  Good catch, will do so.

> Does this affect any of the other near-root pseudos?

They should all be unchanged, I think, since being anon boxes they use their parent's style context as parent, and they always have a parent frame.  Also, :-moz-canvas, :-moz-page, :-moz-pagecontent, :-moz-page-sequence all have "display: block !important" in ua.css.  :-moz-scrolled-content, :-moz-scrolled-canvas, :-moz-scrolled-page-sequence have display:block without !important...

What it _did_ affect was the anonymous content container parented to the canvas frame (the one nsIDocument::InsertAnonymousContent inserts into) _and_ its placeholder.  The anon content container itself is "position: absolute", so it didn't matter there, but with this patch its placeholder no longer gets display:block.  Which is probably for the better.

> does this actually compile when \_Count is 0? 

It seems to, yes.

> I'm a little disturbed that there's no servo equivalent...

As well you should be!  There should be one.  I'm adding one, having it called from ServoStyleSet::Shutdown and from ServoStyleSet::RebuildData, before the Servo_StyleSet_RebuildData call.  Good catch!

> trailing whitespace

Fixed, both places.

> Perhaps some of the continuation-related assertions should be retained here?

Done, by moving my new block and the existing assert that newParentContext exists to after those assertions.

> Also perhaps an assertion that there aren't any child frames?

Good idea.  Added this right before the early return:

  #ifdef DEBUG
      // Make sure we have no children, so we really know there is nothing to do.
      nsIFrame::ChildListIterator lists(aFrame);
      for (; !lists.IsDone(); lists.Next()) {
        MOZ_ASSERT(lists.CurrentList().IsEmpty(),
                   "Failing to reparent style context for child of "
                   "non-inheriting anon box");
      }
  #endif // DEBUG

> Also -- I'm a little disturbed again that there isn't an equivalent change on the stylo side.

Yeah, the stylo side has no concept of reparenting yet.  It will need to grow one for ::first-line support....

> Maybe clearer to call this "cache" rather than "existing"?

Agreed, fixed.

	Pulsebot
	Comment 25 • 8 years ago

Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5ddadc9ae6d
part 1.  Give placeholders and first-letter continuations different kinds of anonymous boxes.  r=dbaron
https://hg.mozilla.org/integration/autoland/rev/44750ea79b9f
part 2.  Restrict the "is a root" blockification behavior in nsStyleContext to non-pseudos and a limited set of anonymous boxes that want the behavior.  r=dbaron
https://hg.mozilla.org/integration/autoland/rev/e1855612a5d4
part 3.  Introduce the concept of non-inheriting anon boxes without adding any yet. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/6e85a63d117c
part 4.  Add storage for the singleton non-inheriting anon box style contexts on style set.  r=dbaron
https://hg.mozilla.org/integration/autoland/rev/c13329ddee05
part 5.  Change the restyle manager to handle style contexts with no parent in ReparentStyleContext (by doing nothing with them).  r=dbaron
https://hg.mozilla.org/integration/autoland/rev/930853cae5a4
part 6.  Change nsFrame::CorrectStyleParentFrame to return null if we're dealing with a non-inheriting anon box.  r=dbaron
https://hg.mozilla.org/integration/autoland/rev/afd58f4674d1
part 7.  Make placeholders a non-inheriting anon box.  r=dbaron

	Carsten Book [:Tomcat]
	Comment 26 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e5ddadc9ae6d
https://hg.mozilla.org/mozilla-central/rev/44750ea79b9f
https://hg.mozilla.org/mozilla-central/rev/e1855612a5d4
https://hg.mozilla.org/mozilla-central/rev/6e85a63d117c
https://hg.mozilla.org/mozilla-central/rev/c13329ddee05
https://hg.mozilla.org/mozilla-central/rev/930853cae5a4
https://hg.mozilla.org/mozilla-central/rev/afd58f4674d1