stylo: support ::first-letter

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
8 months ago
28 days ago

People

(Reporter: heycam, Assigned: bz)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 3 obsolete attachments)

59 bytes, text/x-review-board-request
heycam
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
59 bytes, text/x-review-board-request
emilio
: review+
Details | Review
Comment hidden (empty)
(Reporter)

Updated

8 months ago
Blocks: 1324636
(Reporter)

Updated

8 months ago
Blocks: 1324646
(Reporter)

Updated

8 months ago
Blocks: 1324657
(Reporter)

Updated

8 months ago
Blocks: 1324658
Blocks: 1324619
bz and I discussed this a bit, in particular how to handle the stylo restyle case where we're crawling the content tree (and not the frame tree), which means that it's harder to find the ::first-letter frame for a given originating element.

[1] Shows how there can be an arbitrarily-large separation in the content tree between the the originating element for the pseudo and the element whose inline frame actually inherits from the ::first-line style. However, there are a few interesting things to note.

First, ::first-letter and ::first-line only apply to block frames per spec. So we just ignore them if they show up on anything that's display:inline.

Second, Gecko's existing behavior (which differs from Chrome's, and both of which differ from the spec) is that ::first-letter and ::first-line do not apply into nested block elements (see [2]). This makes our job easier.

So the proposed implementation strategy is as follows:
* When servo cascades ::first-line and ::first-letter and stashes them on HashMap hanging off the originating element, it computes the styles with no style parent, since the precise parent to inherit from is heavily frame-constructor-dependent (and in the ::first-line case, can be different styles depending on which frame we're resolving for).
* When we resolve those styles from gecko, we pass in the parent for a lazy cascade, similar to what we do currently for anonymous boxes.
* To find the appropriate frame to restyle during the content tree traversal in RecreateStyleContexts, we first check whether a given block-level element has the appropriate pseudo. If it does, then we do a left-handed depth-first search of the frame tree from the primary frame, stopping when we either (a) hit a block frame or (b) find a frame for that pseudo.

One tricky thing we'll need to handle is the possibility that there might be a RECONSTRUCT_FRAME change hint lurking further down the content tree between the originating element and the affected frame. We don't want to reach through these and do extra work, and they also could affect the block-vs-inline nature of the descendants, which affects how we'd propagate the pseudo style. It might make sense to do this processing postorder, and return some bitmask indicating which pseudos we've already handled further down the tree. Or something.

[1] data:text/html,<style>div::first-letter { color: red; } </style><div><span><span><span>text</span></span></span></div>
[2] data:text/html,<style>div::first-letter { color: red; }</style><div><p>text</p></div>
> (a) hit a block frame

More precisely, when you hit anything that's not a non-replaced inline.  You might hit an image frame, or inline-table, or whatever; if you do, you stop.  

In Gecko the search basically stops when it finds either a textframe (which is where the first-letter frame then goes) or a brFrame or a frame that is not eLineParticipant.
Assignee: nobody → bzbarsky
Priority: -- → P1
(Assignee)

Updated

4 months ago
Blocks: 1360424
(Assignee)

Updated

3 months ago
Blocks: 1363549
(Assignee)

Updated

3 months ago
Depends on: 1352743
Blocks: 1324348
Depends on: 1369187
(Assignee)

Updated

2 months ago
Depends on: 1375315
(Assignee)

Updated

2 months ago
Blocks: 1375338
(Assignee)

Updated

2 months ago
Blocks: 1375971
(Assignee)

Updated

2 months ago
Blocks: 1376071
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 months ago
mozreview-review
Comment on attachment 8881005 [details]
Bug 1324618 part 1.  Fix dynamic restyling that changes whether our element may generate a pseudo to clear out stale pseudo styles.

https://reviewboard.mozilla.org/r/152364/#review157420

::: servo/components/style/matching.rs:1219
(Diff revision 1)
>  
>              if !self.may_generate_pseudo(&pseudo, data.styles.primary()) {
> +                // We might have been able to generate this pseudo before this
> +                // restyle.  Clear the relevant bits.
> +                // FIXME(bz): Does this clearing need to be done if
> +                // !data.restyle.is_restyle() and is checking that very cheap?

It's cheap (like, a bitflag check), and it's presumably not needed.

::: servo/components/style/matching.rs:1221
(Diff revision 1)
> +                // We might have been able to generate this pseudo before this
> +                // restyle.  Clear the relevant bits.
> +                // FIXME(bz): Does this clearing need to be done if
> +                // !data.restyle.is_restyle() and is checking that very cheap?
> +                let pseudos = &mut context.thread_local.current_element_info
> +                                          .as_mut().unwrap()

nit: `context.thread_local.cascade_inputs_mut().pseudos` instead.

::: servo/components/style/matching.rs:1223
(Diff revision 1)
> +                // FIXME(bz): Does this clearing need to be done if
> +                // !data.restyle.is_restyle() and is checking that very cheap?
> +                let pseudos = &mut context.thread_local.current_element_info
> +                                          .as_mut().unwrap()
> +                                          .cascade_inputs.pseudos;
> +                pseudos.remove_rules(&pseudo, visited_handling);

Should this also set the matches_dfferent_pseudos, and do the damage calculation stuff?

If so, maybe it's just easier to convert the block below where we grab the `map`, etc from:

```
{
    let map = ...;
    stylist.push_applicable_declarations(..)
}
```

To:

```
if self.may_generate_pseudo(&pseudo, data.styles.primary()) {
    let map = ..;
    stylist.push_applicable_declarations(..);
```

What do you think?

Comment 14

2 months ago
mozreview-review
Comment on attachment 8881006 [details]
Bug 1324618 part 2.  Add some comments to may_generate_pseudo; no real behavior changes for now.

https://reviewboard.mozilla.org/r/152366/#review157422

::: servo/components/style/dom.rs:442
(Diff revision 1)
> -    }
> +            }
> +            PseudoElement::FirstLetter | PseudoElement::FirstLine => {
> +                // Only supported for block-inside things.  Unfortunately, Gecko
> +                // has block-inside things that might have any computed display
> +                // value due to things like fieldsets, legends, etc.  Need to
> +                // figure out xhow this should work.

nit: xhow/how.
Attachment #8881006 - Flags: review?(emilio+bugs) → review+

Comment 15

2 months ago
mozreview-review
Comment on attachment 8881007 [details]
Bug 1324618 part 3.  Assert that compute_style_difference is only called with eager pseudos.

https://reviewboard.mozilla.org/r/152368/#review157424
Attachment #8881007 - Flags: review?(emilio+bugs) → review+

Comment 16

2 months ago
mozreview-review
Comment on attachment 8881009 [details]
Bug 1324618 part 5.  Add a way to pass a different style context for later continuations to UpdateStyleOfOwnedChildFrame.

https://reviewboard.mozilla.org/r/152372/#review157426

::: layout/generic/nsFrame.cpp:10252
(Diff revision 1)
>  nsChangeHint
> -nsIFrame::UpdateStyleOfOwnedChildFrame(nsIFrame* aChildFrame,
> +nsIFrame::UpdateStyleOfOwnedChildFrame(
> +  nsIFrame* aChildFrame,
> -                                       nsStyleContext* aNewStyleContext,
> +  nsStyleContext* aNewStyleContext,
> -                                       ServoRestyleState& aRestyleState)
> +  ServoRestyleState& aRestyleState,
> +  const Maybe<nsStyleContext*>& aContinuationStyleContext)

nit: I'd normally use just `nsStyleContext&` for `aNewStyleContext`, and `nsStyleContext*` for `aContinuationStyleContext`. Not sure how you feel about that?
Attachment #8881009 - Flags: review?(emilio+bugs) → review+

Comment 17

2 months ago
mozreview-review
Comment on attachment 8881010 [details]
Bug 1324618 part 6.  Change when we update frame pseudo styles to happen after we've dealt with styles for the frame's kids.

https://reviewboard.mozilla.org/r/152374/#review157428

::: layout/base/ServoRestyleManager.cpp:461
(Diff revision 1)
>        }
>      }
>    }
>  
> +  // We want to update frame pseudo-element styles after we've traversed our
> +  // kids, because some of those updates need to modify the styles of the kids,

nit: maybe mention first-line/letter explicitly?
Attachment #8881010 - Flags: review?(emilio+bugs) → review+

Comment 18

2 months ago
mozreview-review
Comment on attachment 8881011 [details]
Bug 1324618 part 7.  Set our new style context on all our continuations in ServoRestyleManager.

https://reviewboard.mozilla.org/r/152376/#review157430

::: layout/base/ServoRestyleManager.cpp:401
(Diff revision 1)
> -    // different continuations... (e.g. first-line).
> -    for (nsIFrame* f = styleFrame; f;
> -         f = GetNextContinuationWithSameStyle(f, oldStyleContext)) {
> +    // the earlier ones, so there is no point stopping right at the point when
> +    // we'd actually be setting the right style context.
> +    //
> +    // This does mean that we may be setting the wrong style context on our
> +    // initial continuations; ::first-line fixes that up after the fact.
> +    for (nsIFrame* f = styleFrame; f; f = f->GetNextContinuation()) {

GetNextContinuationWithSameStyle also did ib split stuff. I'm assuming this is not needed because it's taken care on the anon-boxes code, right?
Attachment #8881011 - Flags: review?(emilio+bugs) → review+

Comment 19

2 months ago
mozreview-review
Comment on attachment 8881012 [details]
Bug 1324618 part 8.  Implement restyling for first-letter frames.

https://reviewboard.mozilla.org/r/152378/#review157432

Looks good. If bug 1375674 happens to land before this, you probably need to add `IsLetterFrame()` also to the place `IsBulletFrame()` is in the assertions.
Attachment #8881012 - Flags: review?(emilio+bugs) → review+

Comment 20

2 months ago
mozreview-review
Comment on attachment 8881013 [details]
Bug 1324618 part 9.  Implement FrameForPseudoElement for ::first-letter.

https://reviewboard.mozilla.org/r/152380/#review157434

::: layout/base/ServoRestyleManager.cpp:254
(Diff revision 1)
>    bool mShouldComputeHints;
>    nsChangeHint mComputedHint;
>  };
>  
> +// Find an in-flow first-letter frame, if any, in the given frame subtree.
> +// Should not cross block boundaries..

I think this part of this comment isn't really accurate (maybe leftover?).

::: servo/components/style/matching.rs:1520
(Diff revision 1)
>                  return StyleDifference::new(RestyleDamage::empty(), StyleChange::Unchanged)
>              }
>              return StyleDifference::new(RestyleDamage::reconstruct(), StyleChange::Changed)
>          }
>  
> +        match pseudo {

nit: This won't compile in servo I think (Servo doesn't have `PseudoElement::FirstLetter`).

Also, this doesn't seem reachable at all for Before and After (because the block before this piece of code always returns).

So perhaps this can just become `if pseudo.map_or(false, |p| p.is_first_letter())`?

Comment 21

2 months ago
mozreview-review
Comment on attachment 8881014 [details]
Bug 1324618 part 10.  Fix style resolution for pseudo-elements to actually pass through the right parent style.

https://reviewboard.mozilla.org/r/152382/#review157436

::: layout/reftests/bugs/reftest.list:39
(Diff revision 1)
>  == 18217-zorder-2.html 18217-zorder-ref.html
>  == 18217-zorder-3.html 18217-zorder-ref-inline.html
>  == 18217-zorder-4.html 18217-zorder-ref-inline-table.html
>  == 18217-zorder-5.html 18217-zorder-ref-inline-table.html
>  fails-if(styloVsGecko) == 23604-1.html 23604-1-ref.html
> -fails-if(styloVsGecko) == 23604-2.html 23604-2-ref.html
> +== 23604-2.html 23604-2-ref.html

nice :)

::: layout/style/ServoStyleSet.cpp:581
(Diff revision 1)
>      computedValues = Servo_ResolveStyle(aPseudoElement, mRawSet.get(),
>                                          mAllowResolveStaleStyles).Consume();
>    } else {
> +    const ServoComputedValues* parentStyle =
> +      aParentContext ? aParentContext->ComputedValues()
> +                     : nullptr;

nit: I think this fits on the previous line.

::: layout/style/ServoStyleSet.cpp:923
(Diff revision 1)
>                                         nsStyleContext* aParentContext,
>                                         Element* aPseudoElement)
>  {
>    UpdateStylistIfNeeded();
>    if (aPseudoElement) {
>      NS_ERROR("stylo: We don't support CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE yet");

Hmm... I thought I had fixed all the CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE stuff... But wait, why do we have a `aPseudoElement` argument to _probe_ the existence of a pseudo-element?

Whatever, I'll fix this up.

::: servo/ports/geckolib/glue.rs:1527
(Diff revision 1)
>      doc_data: &PerDocumentStyleDataImpl,
>      is_probe: bool,
>  ) -> Option<Arc<ComputedValues>> {
>      let style = match pseudo.cascade_type() {
> -        PseudoElementCascadeType::Eager => styles.pseudos.get(&pseudo).map(|s| s.clone()),
> +        PseudoElementCascadeType::Eager => {
> +            match pseudo {

nit: I think (except when they're wrapped in other type or something) we avoid matching directly on references and do:

```
match *pseudo {
    PseudoElement::FirstLine => { ... }
    _ => { ... }
}
```

instead. It's pretty much stylistic.

::: servo/ports/geckolib/glue.rs:1548
(Diff revision 1)
> +                        .compute_pseudo_element_style_with_rulenode(
> +                            rule_node,
> +                            &guards,
> +                            inherited_styles,
> +                            &metrics)
> +                        .map(|s| s.clone())

nit: I think this `map(|s| s.clone())` can just become `cloned()` (feel free to fix up the pre-existing ones in the same function if so).

::: servo/ports/geckolib/glue.rs:2672
(Diff revision 1)
>                      &guard,
>                      element,
>                      pseudo,
>                      rule_inclusion,
>                      styles,
> +                    None,

nit: perhaps `/* inherited_styles = */ None,`, so it's clear what it means.
Attachment #8881014 - Flags: review?(emilio+bugs) → review+
(Reporter)

Comment 22

2 months ago
I'm pretty sure it should work, but can you confirm that this all works correctly for SVG <text> with a ::first-letter on it?
(Reporter)

Comment 23

2 months ago
mozreview-review
Comment on attachment 8881008 [details]
Bug 1324618 part 4.  Implement a way to get the first-letter frame, if any, for a block.

https://reviewboard.mozilla.org/r/152370/#review157468

::: layout/generic/nsBlockFrame.cpp:7551
(Diff revision 1)
> +    if (kid->IsFrameOfType(nsIFrame::eLineParticipant)) {
> +      nsIFrame* found = FindFirstLetterFrameInSubtree(kid);

Maybe mention in a comment we don't need to bother checking for a BRFrame, for readers looking for differences between this and nsCSSFrameConstructor::WrapFramesInFirstLetterFrame. :-)

::: layout/generic/nsBlockFrame.cpp:7576
(Diff revision 1)
> +  // The first-letter frame could be the first float.
> +  if (!mFloats.IsEmpty() && mFloats.FirstChild()->IsLetterFrame()) {
> +    return mFloats.FirstChild();
> +  }

Is it possible for it to be in the PushedFloats list, if we interrupted reflow and flushed style before flushing layout again?
Attachment #8881008 - Flags: review?(cam) → review+
(Reporter)

Comment 24

2 months ago
(In reply to Cameron McCormack (:heycam) from comment #23)
> Is it possible for it to be in the PushedFloats list, if we interrupted
> reflow and flushed style before flushing layout again?

r=me if we know it can't be in that list, or if it can, if you add a search through it.
(Assignee)

Comment 25

2 months ago
mozreview-review-reply
Comment on attachment 8881005 [details]
Bug 1324618 part 1.  Fix dynamic restyling that changes whether our element may generate a pseudo to clear out stale pseudo styles.

https://reviewboard.mozilla.org/r/152364/#review157420

> It's cheap (like, a bitflag check), and it's presumably not needed.

Hmm.  I've been having a hard time proving to myself that it's not needed, because I don't really understand the semantics of the is_restyle() boolean...  I'll just take out the FIXME comment for now.

> nit: `context.thread_local.cascade_inputs_mut().pseudos` instead.

Done, but I had to move the bloom_filter and matching_context bits down past this, because  bloom_filter does an immutable borrow of "context" and we're now doing a mutable borrow of "context" and the borrow checker was not happy.

> Should this also set the matches_dfferent_pseudos, and do the damage calculation stuff?
> 
> If so, maybe it's just easier to convert the block below where we grab the `map`, etc from:
> 
> ```
> {
>     let map = ...;
>     stylist.push_applicable_declarations(..)
> }
> ```
> 
> To:
> 
> ```
> if self.may_generate_pseudo(&pseudo, data.styles.primary()) {
>     let map = ..;
>     stylist.push_applicable_declarations(..);
> ```
> 
> What do you think?

I think any change which changes whether may_generate_pseudo returns true will involve a reframe anyway, so I'm not sure there's much point to damage calculation bits here.

It's hard to say, because I haven't figure out a safe way to have may_generate_pseudo return anything other than `true` so far, so this is all a bit academic for the moment.  :(
(Assignee)

Comment 26

2 months ago
mozreview-review-reply
Comment on attachment 8881008 [details]
Bug 1324618 part 4.  Implement a way to get the first-letter frame, if any, for a block.

https://reviewboard.mozilla.org/r/152370/#review157468

> Maybe mention in a comment we don't need to bother checking for a BRFrame, for readers looking for differences between this and nsCSSFrameConstructor::WrapFramesInFirstLetterFrame. :-)

Actually, we should probably check for a BRFrame.  Otherwise we can keep looking way past the point where we know we can stop, in cases when we have first-letter style but no actual first-letter frame.  I'll add that check:

    if (kid->IsFrameOfType(nsIFrame::eLineParticipant) &&
        !kid->IsBrFrame()) {

> Is it possible for it to be in the PushedFloats list, if we interrupted reflow and flushed style before flushing layout again?

Hmm.  I don't actually know.  For that matter, could it end up in the floats list of one of our continuations?

I want to say that it can't, because we don't really treat this thing as a normal float, but I'm not 100% sure.  Maybe we should just take the bullet approach and store a property pointing to this frame, and then skip this whole search.  :(

Comment 27

2 months ago
mozreview-review-reply
Comment on attachment 8881005 [details]
Bug 1324618 part 1.  Fix dynamic restyling that changes whether our element may generate a pseudo to clear out stale pseudo styles.

https://reviewboard.mozilla.org/r/152364/#review157420

> I think any change which changes whether may_generate_pseudo returns true will involve a reframe anyway, so I'm not sure there's much point to damage calculation bits here.
> 
> It's hard to say, because I haven't figure out a safe way to have may_generate_pseudo return anything other than `true` so far, so this is all a bit academic for the moment.  :(

Hmm... Yeah, I see.

My point was (sorry for not making it clear above), that the early return makes us not call `data.styles.pseudos.take(&pseudo)`, which I think it's important if there was a pseudo before, so my suggestion made it share most of the path (!may_generate_pseudo just makes us skip selector-matching).
(Assignee)

Comment 28

2 months ago
mozreview-review-reply
Comment on attachment 8881009 [details]
Bug 1324618 part 5.  Add a way to pass a different style context for later continuations to UpdateStyleOfOwnedChildFrame.

https://reviewboard.mozilla.org/r/152372/#review157426

> nit: I'd normally use just `nsStyleContext&` for `aNewStyleContext`, and `nsStyleContext*` for `aContinuationStyleContext`. Not sure how you feel about that?

I think in this case it would sadly be confusing: far too many places pass nsStyleContext* to mean "here is a non-null style context".  So until we mass-change nsStyleContext bits I would rather not use "nsStyleContext*" to mean "might be null".

Comment 29

2 months ago
mozreview-review
Comment on attachment 8881005 [details]
Bug 1324618 part 1.  Fix dynamic restyling that changes whether our element may generate a pseudo to clear out stale pseudo styles.

https://reviewboard.mozilla.org/r/152364/#review157614

With that, r=me
Attachment #8881005 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 30

2 months ago
mozreview-review-reply
Comment on attachment 8881011 [details]
Bug 1324618 part 7.  Set our new style context on all our continuations in ServoRestyleManager.

https://reviewboard.mozilla.org/r/152376/#review157430

> GetNextContinuationWithSameStyle also did ib split stuff. I'm assuming this is not needed because it's taken care on the anon-boxes code, right?

Good catch!  This patch did in fact break insertion of text into a non-first-inline of an ib split that got restyled.  I'll fix the ib handling to in fact handle this, and add a test that would have caught it!
(Assignee)

Comment 31

2 months ago
mozreview-review-reply
Comment on attachment 8881014 [details]
Bug 1324618 part 10.  Fix style resolution for pseudo-elements to actually pass through the right parent style.

https://reviewboard.mozilla.org/r/152382/#review157436

> nit: I think this fits on the previous line.

Yep, fixed.

> Hmm... I thought I had fixed all the CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE stuff... But wait, why do we have a `aPseudoElement` argument to _probe_ the existence of a pseudo-element?
> 
> Whatever, I'll fix this up.

Yeah, having an aPseudoElement arg there is kinda dumb....  Is there a bug tracking fixing this up?

> nit: I think (except when they're wrapped in other type or something) we avoid matching directly on references and do:
> 
> ```
> match *pseudo {
>     PseudoElement::FirstLine => { ... }
>     _ => { ... }
> }
> ```
> 
> instead. It's pretty much stylistic.

Fixed.

> nit: I think this `map(|s| s.clone())` can just become `cloned()` (feel free to fix up the pre-existing ones in the same function if so).

Unfortunately, no.  I have an `Option<Arc<ComputedValues>>`.  `cloned` is defined for `Option<&T>`.  So I could replace `map(|s| s.clone())` with `as_ref().cloned()`, but just using `cloned()` doesn't work.  I'll leave this as-is unless you think `as_ref().cloned()` is really better.

> nit: perhaps `/* inherited_styles = */ None,`, so it's clear what it means.

Done.
(Assignee)

Comment 32

2 months ago
mozreview-review-reply
Comment on attachment 8881013 [details]
Bug 1324618 part 9.  Implement FrameForPseudoElement for ::first-letter.

https://reviewboard.mozilla.org/r/152380/#review157434

> I think this part of this comment isn't really accurate (maybe leftover?).

Good catch, removed.

> nit: This won't compile in servo I think (Servo doesn't have `PseudoElement::FirstLetter`).
> 
> Also, this doesn't seem reachable at all for Before and After (because the block before this piece of code always returns).
> 
> So perhaps this can just become `if pseudo.map_or(false, |p| p.is_first_letter())`?

Good catch, fixed.

Comment 33

2 months ago
mozreview-review-reply
Comment on attachment 8881014 [details]
Bug 1324618 part 10.  Fix style resolution for pseudo-elements to actually pass through the right parent style.

https://reviewboard.mozilla.org/r/152382/#review157436

> Yeah, having an aPseudoElement arg there is kinda dumb....  Is there a bug tracking fixing this up?

I already fixed this up in bug 1376077 :)

> Unfortunately, no.  I have an `Option<Arc<ComputedValues>>`.  `cloned` is defined for `Option<&T>`.  So I could replace `map(|s| s.clone())` with `as_ref().cloned()`, but just using `cloned()` doesn't work.  I'll leave this as-is unless you think `as_ref().cloned()` is really better.

As discussed on IRC, only the `pseudos.get(&pseudo)` really need `clone()`, and that one can become `cloned()`. The other two `map(|s| s.clone())` should go away.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

2 months ago
mozreview-review-reply
Comment on attachment 8881008 [details]
Bug 1324618 part 4.  Implement a way to get the first-letter frame, if any, for a block.

https://reviewboard.mozilla.org/r/152370/#review157468

> Hmm.  I don't actually know.  For that matter, could it end up in the floats list of one of our continuations?
> 
> I want to say that it can't, because we don't really treat this thing as a normal float, but I'm not 100% sure.  Maybe we should just take the bullet approach and store a property pointing to this frame, and then skip this whole search.  :(

Looks like we can in fact end up with the float in some random later continuation, in testcases like this:

    <style>
      span { display: block; background: yellow; }
      span::first-letter { color: red; float: left; }
      span.x::first-letter { color: green; }
    </style>
    <div style="column-width: 100px; column-fill: auto; height: 100px; border: 1px solid green;">
      <div style="float: left; width: 98px; height: 181px; background: black"></div>
      <span onclick="this.className = 'x'">
        C<div style="float: left; height: 1px"></div>lick me; this text should turn red.
      </span>
    </div>

so I'm going to change to storing this in a frame property to avoid all that complexity.
(Assignee)

Updated

2 months ago
Attachment #8881013 - Flags: review?(emilio+bugs)
OK, since mozreview won't let me re-request reviews on substantive changes here....

Emilio, could you look over parts 1, 2, 7, 9, 10 again?

Cameron, could you look at part 4 again?
Flags: needinfo?(emilio+bugs)
Flags: needinfo?(cam)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 53

2 months ago
mozreview-review
Comment on attachment 8881013 [details]
Bug 1324618 part 9.  Implement FrameForPseudoElement for ::first-letter.

https://reviewboard.mozilla.org/r/152380/#review157630

r=me. The other patches look great too. Thanks!
Attachment #8881013 - Flags: review?(emilio+bugs) → review+
(Reporter)

Comment 54

2 months ago
I think the new part 4 looks OK to me.  I'm not terribly certain on when exactly we call RemoveLetterFrames -- it seems like we call it in expected places, but I couldn't satisfy myself that it's the only way that a Letter frame can go away without its Block going away also.
Flags: needinfo?(cam)
We basically do RemoveLetterFrames on any insertion/removal/text-change in a block with first-letter.

There's nothing else that I know of that would make a first-letter go away without the block going away, but I agree that proving that is not obvious.  Unfortunately, getting back to the block from a first-letter that is being destroyed is not simple either, nor is ensuring that block itself is _not_ getting destroyed...
Flags: needinfo?(emilio+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8881005 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8881006 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8881007 - Attachment is obsolete: true

Comment 63

2 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/049b20a12429
part 4.  Implement a way to get the first-letter frame, if any, for a block.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/b40d51d1f78e
part 5.  Add a way to pass a different style context for later continuations to UpdateStyleOfOwnedChildFrame.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/d2ac7a671d41
part 6.  Change when we update frame pseudo styles to happen after we've dealt with styles for the frame's kids.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/01deb185c67a
part 7.  Set our new style context on all our continuations in ServoRestyleManager.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/169cafc2d5f9
part 8.  Implement restyling for first-letter frames.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/18cd81f24778
part 9.  Implement FrameForPseudoElement for ::first-letter. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a33ae6fa707d
part 10.  Fix style resolution for pseudo-elements to actually pass through the right parent style.  r=emilio
sorry boris, backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=109995764&repo=autoland
Flags: needinfo?(bzbarsky)

Comment 65

2 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbbdd653ca60
Backed out changeset a33ae6fa707d 
https://hg.mozilla.org/integration/autoland/rev/2e83be4674e1
Backed out changeset 18cd81f24778 
https://hg.mozilla.org/integration/autoland/rev/83508083bd67
Backed out changeset 169cafc2d5f9 
https://hg.mozilla.org/integration/autoland/rev/45045c0a3362
Backed out changeset 01deb185c67a 
https://hg.mozilla.org/integration/autoland/rev/90be2993cbe1
Backed out changeset d2ac7a671d41 
https://hg.mozilla.org/integration/autoland/rev/58d14978f7b3
Backed out changeset b40d51d1f78e 
https://hg.mozilla.org/integration/autoland/rev/a7631339f6bf
Backed out changeset 049b20a12429 for bustage

Comment 66

2 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9605ad692ac
part 4.  Implement a way to get the first-letter frame, if any, for a block.  r=heycam
https://hg.mozilla.org/integration/autoland/rev/a144aba6abe5
part 5.  Add a way to pass a different style context for later continuations to UpdateStyleOfOwnedChildFrame.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/924edce03a64
part 6.  Change when we update frame pseudo styles to happen after we've dealt with styles for the frame's kids.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/7580a5f91d0d
part 7.  Set our new style context on all our continuations in ServoRestyleManager.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/48e17a3c706a
part 8.  Implement restyling for first-letter frames.  r=emilio
https://hg.mozilla.org/integration/autoland/rev/df7dd25305d6
part 9.  Implement FrameForPseudoElement for ::first-letter. r=emilio
https://hg.mozilla.org/integration/autoland/rev/961eb14ca94d
part 10.  Fix style resolution for pseudo-elements to actually pass through the right parent style.  r=emilio

Comment 67

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e9605ad692ac
https://hg.mozilla.org/mozilla-central/rev/a144aba6abe5
https://hg.mozilla.org/mozilla-central/rev/924edce03a64
https://hg.mozilla.org/mozilla-central/rev/7580a5f91d0d
https://hg.mozilla.org/mozilla-central/rev/48e17a3c706a
https://hg.mozilla.org/mozilla-central/rev/df7dd25305d6
https://hg.mozilla.org/mozilla-central/rev/961eb14ca94d
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Flags: needinfo?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.