Closed Bug 1324619 Opened 7 years ago Closed 7 years ago

stylo: support ::first-line

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: heycam, Assigned: bzbarsky, NeedInfo)

References

(Blocks 3 open bugs)

Details

Attachments

(6 files)

Bug 1324619 part 1. Skip reparenting style contexts in the frame constructor when it's not needed.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1324619 part 2. Stop unnecessarily trying to reparent the style contexts of our kids in nsFirstLetterFrame::SetInitialChildList.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1324619 part 3. Implement ReparentStyleContext in ServoRestyleManager, for ::first-line use.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1324619 part 4. Add a Servo API for reparenting a given style.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1324619 part 5. Implement FrameForPseudoElement for ::first-line.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1324619 part 6. Handle dynamic restyles of ::first-line in stylo.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
      No description provided.
Blocks: 1324665
Blocks: 1324673
Blocks: 1324705
So the complexity here includes all the stuff we need to do to support ::first-line (bug 1324618), _plus_ the added complications of how to deal with multiple inline frames inheriting from the same style context.

So we'll probably want to look at bug 1324618 first (or at least simultaneously).
Depends on: 1324618
There's also the complexity of how the kids of first-letter inherit.

Given this document:

<!DOCTYPE html>
<style>
  div { border-color: purple; color: yellow }
  div::first-line { border-color: green; color: blue; }
  span { color: inherit; border-color: inherit; border-style: solid;
         border-width: 5px}
</style>
<div><span>Text</span></div>

per spec and in Gecko the span has a purple border and blue color.  (Chrome doesn't implement the spec here, but it does other weird stuff; for example "display" in the <span> _does_ inherit from the <div>.  The spec's setup was decided on as the sanest thing to produce something kinda like observable behavior in important cases and not special-case any particular properties.)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> There's also the complexity of how the kids of first-letter inherit.

Do you mean first-line?

> 
> Given this document:
> 
> <!DOCTYPE html>
> <style>
>   div { border-color: purple; color: yellow }
>   div::first-line { border-color: green; color: blue; }
>   span { color: inherit; border-color: inherit; border-style: solid;
>          border-width: 5px}
> </style>
> <div><span>Text</span></div>
> 
> per spec and in Gecko the span has a purple border and blue color.

Huh. Why the different inheritance sources for border-color and color?
> Do you mean first-line?

Yes.

> Why the different inheritance sources for border-color and color:

Because color inherits by default and border-color does not.  Per spec, props that inherit by default inherit from first-line and props that do not inherit by default inherit from the DOM parent.  Or at least that _used_ to be the case when Gecko implemented it.  Looking at https://www.w3.org/TR/css3-selectors/#first-formatted-line last paragraph it says:

  During CSS inheritance, the portion of a child element that occurs on the first line only inherits
  properties applicable to the ::first-line pseudo-element from the ::first-line pseudo-element. For
  all other properties inheritence is from the non-pseudo-element parent of the first line pseudo
  element. (The portion of a child element that does not occur on the first line always inherits
  from the parent of that child.) 

so they apparently went back to determining where to inherit from on a fairly ad-hoc per-property basis? :(
Flags: needinfo?(dbaron)
So I guess per-spec border-color and background-color should inherit differently (because the former does not apply and the latter does).  In Gecko they inherit identically, since neither inherits by default...
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #4)
> Because color inherits by default and border-color does not.  Per spec,
> props that inherit by default inherit from first-line and props that do not
> inherit by default inherit from the DOM parent.  Or at least that _used_ to
> be the case when Gecko implemented it.  Looking at
> https://www.w3.org/TR/css3-selectors/#first-formatted-line last paragraph it
> says:
> 
>   During CSS inheritance, the portion of a child element that occurs on the
> first line only inherits
>   properties applicable to the ::first-line pseudo-element from the
> ::first-line pseudo-element. For
>   all other properties inheritence is from the non-pseudo-element parent of
> the first line pseudo
>   element. (The portion of a child element that does not occur on the first
> line always inherits
>   from the parent of that child.) 
> 
> so they apparently went back to determining where to inherit from on a
> fairly ad-hoc per-property basis? :(

I don't recall the discussion that led to this.

It's probably worth investigating both:
 * what WG discussion led to that spec text
 * whether other browsers implement it

I'll leave the needinfo? to do that (at least the first) at some point later, although somebody else could also investigate it first...
Assignee: nobody → bzbarsky
Priority: -- → P1
Depends on: 1352743
Depends on: 1375315
Blocks: 1375338
Blocks: 1376071
Depends on: 1382806
Depends on: 1382786
Attachment #8891187 - Flags: review?(emilio+bugs)
Attachment #8891188 - Flags: review?(emilio+bugs)
Comment on attachment 8891183 [details]
Bug 1324619 part 1.  Skip reparenting style contexts in the frame constructor when it's not needed.

https://reviewboard.mozilla.org/r/162388/#review167762

::: layout/base/nsCSSFrameConstructor.cpp:497
(Diff revision 1)
>                nsContainerFrame* aNewParentFrame,
> -              nsIFrame* aFrame)
> +              nsIFrame* aFrame,
> +              bool aForceStyleReparent)
>  {
>    aFrame->SetParent(aNewParentFrame);
> +  if (aForceStyleReparent || aRestyleManager->IsGecko()) {

nit: I think a comment here for the `IsGecko()` check would be worth it.
Attachment #8891183 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8891184 [details]
Bug 1324619 part 2.  Stop unnecessarily trying to reparent the style contexts of our kids in nsFirstLetterFrame::SetInitialChildList.

https://reviewboard.mozilla.org/r/162390/#review167764
Attachment #8891184 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8891185 [details]
Bug 1324619 part 3.  Implement ReparentStyleContext in ServoRestyleManager, for ::first-line use.

https://reviewboard.mozilla.org/r/162392/#review167768

::: layout/base/ServoRestyleManager.cpp:1245
(Diff revision 1)
> +    // We make the first float orange and the second float blue.  On the other
> +    // hand, if the float were within an inline-block that was on the first
> +    // line, arguably it _should_ inherit from the ::first-line...
> +    nsIFrame* outOfFlow =
> +      nsPlaceholderFrame::GetRealFrameForPlaceholder(aFrame);
> +    NS_ASSERTION(outOfFlow, "no out-of-flow frame");

nit: I'd just use MOZ_ASSERT.

::: layout/base/ServoRestyleManager.cpp:1246
(Diff revision 1)
> +    // hand, if the float were within an inline-block that was on the first
> +    // line, arguably it _should_ inherit from the ::first-line...
> +    nsIFrame* outOfFlow =
> +      nsPlaceholderFrame::GetRealFrameForPlaceholder(aFrame);
> +    NS_ASSERTION(outOfFlow, "no out-of-flow frame");
> +    do {

nit: I find slightly easy to read:

for (; outOfFlow; outOfFlow = outOfFlow->GetNextContinuation())

than the do while.

It's one extra null-check, I guess, but not sure it matters enough. Your call anyway, if you prefer do while I'm fine with it.

::: layout/base/ServoRestyleManager.cpp:1249
(Diff revision 1)
> +      nsPlaceholderFrame::GetRealFrameForPlaceholder(aFrame);
> +    NS_ASSERTION(outOfFlow, "no out-of-flow frame");
> +    do {
> +      DoReparentStyleContext(outOfFlow, aStyleSet);
> +    } while ((outOfFlow = outOfFlow->GetNextContinuation()));
> +  } else if (frameType == LayoutFrameType::Backdrop) {

I'd move this to the beginning of the function, so it reads:

if (aFrame->IsBackdropFrame()) {
  return;
}

if (aFrame->IsPlaceholderFrame()) {
  // ...
}

::: layout/base/ServoRestyleManager.cpp:1314
(Diff revision 1)
> +               "How could we get a ::first-line parent style without having "
> +               "a ::first-line provider frame?");
> +    // If newParent is a ::first-line style, use the provider frame's parent
> +    // block's style for the "style ignoring ::first-line".
> +    newParentIgnoringFirstLine =
> +      providerFrame->GetParent()->StyleContext()->AsServo();

Is this wrong in presence of display: contents?

::: layout/base/ServoRestyleManager.cpp:1326
(Diff revision 1)
> +    // No providerFrame means we inherited from a display:contents thing.  Our
> +    // layout parent style is the style of our nearest ancestor frame.
> +    // FIXME(bz) What if that's an anonymous box?  Should we be walking out of
> +    // anonymous boxes here as needed?  Or does that basically not matter in
> +    // practice, because we only use the layout parent in a very limited range
> +    // of situations?

I think we should walk past anon boxes, yeah... Otherwise you may miss blockification for flex items and similar stuff I guess? Though it's unclear if it may matter here...

::: layout/base/ServoRestyleManager.cpp:1360
(Diff revision 1)
> +  }
> +
> +  // Generally, owned anon boxes are our descendants.  The only exceptions are
> +  // tables (for the table wrapper) and inline frames (for the block part of the
> +  // block-in-inline split).  We've already updated our descendants when looping
> +  // over the kids, and we don't want to update the block part of a

I find this somewhat confusing, did you mean to put this comment _after_ looping over the kids?

::: layout/style/ServoStyleSet.h:459
(Diff revision 1)
>     * in a style sheet.
>     */
>    bool HasStateDependency(const dom::Element& aElement,
>                            EventStates aState) const;
>  
> +  /*

nit: Not that we're consistent already, but some comments here use:

```
/**
 *
 *
 */
```

And others just:

```
//
//
//
```

So perhaps peek one of the two so we don't introduce a third comment style in the same file?

::: layout/style/ServoStyleSet.h:463
(Diff revision 1)
>  
> +  /*
> +   * Get a new style context that uses the same rules as the given style context
> +   * but has a different parent.
> +   *
> +   * aIsLink is true if aStyleContext is for a link.

I see no `aIsLink`.
Comment on attachment 8891186 [details]
Bug 1324619 part 4.  Add a Servo API for reparenting a given style.

https://reviewboard.mozilla.org/r/162394/#review167776

r=me. Was somewhat worried about adding yet another entry-point to cascade(), but it's not that terrible.

::: servo/components/style/context.rs:193
(Diff revision 1)
>      pub visited_rules: Option<StrongRuleNode>,
>  }
>  
>  impl CascadeInputs {
>      /// Construct inputs from previous cascade results, if any.
>      pub fn new_from_style(style: &Arc<ComputedValues>) -> Self {

You can just change this to be `&ComputedValues`, afaict, and avoid the two signatures. This should work automatically on the callers.

::: servo/ports/geckolib/glue.rs:184
(Diff revision 1)
>      RefPtr::from_ptr_ref(&DUMMY_URL_DATA)
>  }
>  
> +fn visited_styles_enabled<'a>(per_doc_data: &'a PerDocumentStyleDataImpl)
> +                              -> bool {
> +    return unsafe { bindings::Gecko_AreVisitedLinksEnabled() } &&

nit: no need for `return`. Also maybe worth to just make this a method in `PerDocumentStyleDataImpl`.

::: servo/ports/geckolib/glue.rs:1694
(Diff revision 1)
> +                    // Now in practice this turns out to be OK, because all the
> +                    // cases in which there's a mismatch go ahead and reparent
> +                    // styles again as needed to make sure the ::first-line
> +                    // affects all the things it should affect.  But it makes it
> +                    // impossible to assert anything about the two styles
> +                    // matching here, unfortunately.

ick. Oh well...

::: servo/ports/geckolib/glue.rs:2953
(Diff revision 1)
> +    if let Some(element) = element {
> +        if element.is_link() {
> +            cascade_flags.insert(IS_LINK);
> +            if element.is_visited_link() &&
> +                visited_styles_enabled(&doc_data) {
> +                    cascade_flags.insert(IS_VISITED_LINK);

nit: you doubled indentation here, I think
Attachment #8891186 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8891183 [details]
Bug 1324619 part 1.  Skip reparenting style contexts in the frame constructor when it's not needed.

https://reviewboard.mozilla.org/r/162388/#review167762

> nit: I think a comment here for the `IsGecko()` check would be worth it.

Added comment:

    // We reparent frames for two reasons: to put them inside ::first-line, and to
    // put them inside some wrapper anonymous boxes.
    //
    // The latter shouldn't affect any styles in practice, so only needs style
   // context reparenting in the Gecko backend, to make our style context tree
   // assertions happy.  The former passes aForceStyleReparent == true.
Comment on attachment 8891185 [details]
Bug 1324619 part 3.  Implement ReparentStyleContext in ServoRestyleManager, for ::first-line use.

https://reviewboard.mozilla.org/r/162392/#review167768

> nit: I'd just use MOZ_ASSERT.

Oops, copy/paste fail.  Fixed.

> nit: I find slightly easy to read:
> 
> for (; outOfFlow; outOfFlow = outOfFlow->GetNextContinuation())
> 
> than the do while.
> 
> It's one extra null-check, I guess, but not sure it matters enough. Your call anyway, if you prefer do while I'm fine with it.

Again, copy/paste... I'll change to the for loop.

> I'd move this to the beginning of the function, so it reads:
> 
> if (aFrame->IsBackdropFrame()) {
>   return;
> }
> 
> if (aFrame->IsPlaceholderFrame()) {
>   // ...
> }

Done.

> Is this wrong in presence of display: contents?

I don't think so, because we have an actual first-line frame here, and hence the corresponding block frame.

But it _is_ wrong when the block involved is an anonymous box!  Good catch.  A simple testcase that fails with these patches:

    <style>
      div { color: red; border: 10px solid green; column-count: 2 }
      div::first-line { color: green; border: 10px solid red; }
    </style>
    <div>
     <span style="border: inherit">Some text</span>
    </div>

should have a green border in the inner span, but has none.

I'm going to fix it like so:

    nsIFrame* blockFrame = providerFrame->GetParent();
    nsIFrame* correctedFrame =
      nsFrame::CorrectStyleParentFrame(blockFrame, oldContext->GetPseudo());
    newParentIgnoringFirstLine = correctedFrame->StyleContext()->AsServo();

Boy, I wish we had more first-line tests...

> I think we should walk past anon boxes, yeah... Otherwise you may miss blockification for flex items and similar stuff I guess? Though it's unclear if it may matter here...

Hmm, right.  Going to use CorrectStyleParentFrame here too:

    if (!providerFrame) {
      // No providerFrame means we inherited from a display:contents thing.  Our
      // layout parent style is the style of our nearest ancestor frame.
      providerFrame = nsFrame::CorrectStyleParentFrame(aFrame->GetParent(),
                                                       oldContext->GetPseudo());
    }
    ServoStyleContext* layoutParent = providerFrame->StyleContext()->AsServo();

I _think_ the other thing might have been ok, but it's hard to tell, and this is definitely safer.

> I find this somewhat confusing, did you mean to put this comment _after_ looping over the kids?

I think I started with a copy of the ProcessPostTraversal code, which does anon boxes before kids.

I'll make the comment clearer.

> nit: Not that we're consistent already, but some comments here use:
> 
> ```
> /**
>  *
>  *
>  */
> ```
> 
> And others just:
> 
> ```
> //
> //
> //
> ```
> 
> So perhaps peek one of the two so we don't introduce a third comment style in the same file?

Did the `/**` thing.

> I see no `aIsLink`.

Oops.  It got replaced by aElement.  Good catch!
Comment on attachment 8891186 [details]
Bug 1324619 part 4.  Add a Servo API for reparenting a given style.

https://reviewboard.mozilla.org/r/162394/#review167776

> You can just change this to be `&ComputedValues`, afaict, and avoid the two signatures. This should work automatically on the callers.

Hmm.  I thought I'd tried that, but apparently not.  Good.  ;)

> nit: no need for `return`. Also maybe worth to just make this a method in `PerDocumentStyleDataImpl`.

I actually started with no return there.  That fails to compile, because it thinks the unsafe block is a statement and the `&&` is references or something:

    error[E0308]: mismatched types
       --> glue.rs:184:14
        |
    184 |     unsafe { bindings::Gecko_AreVisitedLinksEnabled() } &&
        |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected (), found bool
        |
        = note: expected type `()`
                   found type `bool`

    error[E0308]: mismatched types
       --> glue.rs:184:57
        |
    184 |       unsafe { bindings::Gecko_AreVisitedLinksEnabled() } &&
        |  _________________________________________________________^ starting here...
    185 | |         !per_doc_data.is_private_browsing_enabled()
        | |___________________________________________________^ ...ending here: expected bool, found &&bool
        |
        = note: expected type `bool`
                   found type `&&bool`

Good idea about making this a method on PerDocumentStyleDataImpl.  Done.

> nit: you doubled indentation here, I think

I just let rust-mode do its thing... Ah, well.
No longer blocks: 1375338
Comment on attachment 8891185 [details]
Bug 1324619 part 3.  Implement ReparentStyleContext in ServoRestyleManager, for ::first-line use.

https://reviewboard.mozilla.org/r/162392/#review167996

Nice, thanks! (And thanks for all the tests!)we

::: layout/reftests/first-line/reftest.list:39
(Diff revision 2)
>  
>  fails-if(styloVsGecko||stylo) == restyle-inside-first-line.html restyle-inside-first-line-ref.html
>  fails-if(styloVsGecko||stylo) == font-styles.html font-styles-ref.html
>  fuzzy-if(OSX==1010,1,2) fails-if(styloVsGecko||stylo) == font-styles-nooverflow.html font-styles-ref.html
> +
> +fails-if(!stylo) == ib-split-1.html ib-split-1-ref.html

heh
Attachment #8891185 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8891187 [details]
Bug 1324619 part 5.  Implement FrameForPseudoElement for ::first-line.

https://reviewboard.mozilla.org/r/162396/#review167998

::: layout/base/ServoRestyleManager.cpp:824
(Diff revision 2)
>    if (aPseudoTagOrNull == nsCSSPseudoElements::firstLetter) {
>      return FindFirstLetterFrameForElement(aElement);
>    }
>  
>    if (aPseudoTagOrNull == nsCSSPseudoElements::firstLine) {
>      // TODO(emilio, bz): Figure out the best way to diff these styles.

remove the `TODO`?

::: layout/generic/nsBlockFrame.cpp:7612
(Diff revision 2)
> +{
> +  // Our ::first-line frame is either the first thing on our principal child
> +  // list, or the second one if we have an inside bullet.
> +  nsIFrame* bullet = GetInsideBullet();
> +  nsIFrame* maybeFirstLine;
> +  if (bullet) {

nit: Not sure if I prefer it or not, but could be:

```
nsIFrame* maybeFirstLine = bullet ? bullet->GetNextSibling() : PrincipalChildList().FirstChild();
```
Attachment #8891187 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8891188 [details]
Bug 1324619 part 6.  Handle dynamic restyles of ::first-line in stylo.

https://reviewboard.mozilla.org/r/162398/#review167994

Yay! You're awesome :). r=mew

::: layout/base/ServoRestyleManager.cpp:50
(Diff revision 2)
>    }
>  
>    return nsLayoutUtils::FirstContinuationOrIBSplitSibling(aFrame);
>  }
>  
> +#ifdef DEBUG

This is already in an ifdef DEBUG block, so you can drop this.

::: layout/base/ServoRestyleManager.cpp:97
(Diff revision 2)
>      parent = FirstContinuationOrPartOfIBSplit(parent);
>    }
>  
>    return parent;
>  }
> +#endif // DEBUG

ditto

::: layout/base/ServoRestyleManager.cpp:740
(Diff revision 2)
> +      //
> +      // FIXME(bz) Could we do better here?  For example, could we keep track of
> +      // frames that are "block with a ::first-line so we could avoid
> +      // IsFrameOfType() and digging about for the first-line frame if not?
> +      // Could we keep track of whether the element children we actually restyle
> +      // are affected by first-line?  Something else?

Yeah, at least avoiding the IsFrameOfType + GetFirstLineFrame for the common case would be nice... Probably worth filing a followup at least?

::: layout/generic/nsBlockFrame.cpp:7595
(Diff revision 2)
> +                                              parentStyle,
> +                                              nullptr);
> +
> +    // FIXME(bz): Can we make first-line continuations be non-inheriting anon
> +    // boxes?
> +    RefPtr<nsStyleContext> continuationStyle = aRestyleState.StyleSet().

nit: You can make these ServoStyleContext's if you want, I suppose. No big deal anyway.
Attachment #8891188 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8891187 [details]
Bug 1324619 part 5.  Implement FrameForPseudoElement for ::first-line.

https://reviewboard.mozilla.org/r/162396/#review167998

> remove the `TODO`?

Er, yes.  Done.  ;)

> nit: Not sure if I prefer it or not, but could be:
> 
> ```
> nsIFrame* maybeFirstLine = bullet ? bullet->GetNextSibling() : PrincipalChildList().FirstChild();
> ```

Yeah, it seemed too noisy and hard to read when I tried to write it that way.
Blocks: 1385443
Comment on attachment 8891188 [details]
Bug 1324619 part 6.  Handle dynamic restyles of ::first-line in stylo.

https://reviewboard.mozilla.org/r/162398/#review167994

> This is already in an ifdef DEBUG block, so you can drop this.

Whoops, I missed that somehow.  Fixed.

> Yeah, at least avoiding the IsFrameOfType + GetFirstLineFrame for the common case would be nice... Probably worth filing a followup at least?

Filed bug 1385443.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6253d87d0734
part 1.  Skip reparenting style contexts in the frame constructor when it's not needed.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/b1aa311b6b42
part 2.  Stop unnecessarily trying to reparent the style contexts of our kids in nsFirstLetterFrame::SetInitialChildList.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/d5d6e006b251
part 3.  Implement ReparentStyleContext in ServoRestyleManager, for ::first-line use.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/e1b802894f33
part 4.  Add a Servo API for reparenting a given style.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/64f8d2059998
part 5.  Implement FrameForPseudoElement for ::first-line.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/ab3c85d4d199
part 6.  Handle dynamic restyles of ::first-line in stylo.  r=emilio
Blocks: 1385656
Blocks: 1385658
Blocks: 520605
Depends on: 1404324
Depends on: 1406222
Depends on: 1409502
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: