945105 - style changes not handled properly with ::first-line and continuations

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

Attachment: test

Attached is a simpler version of the test from bug 670467.  The change to the <span>'s visibility should cause the continuations to be restyled, and show the entire "This is some text", but only the "This" is shown.

Comment 1

[Cameron McCormack (:heycam)]

Regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68d279364a8b&tochange=e85b0372cece

Bug 898333 sounds like a likely candidate.

Comment 2

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

I think the problem is that the changes to RestyleManager::ComputeStyleChangeFor in https://hg.mozilla.org/mozilla-central/rev/ed9c22ef51e0 assume that all ib-siblings and next-in-flows actually do copy the style; this code instead needs to use GetPrevContinuationWithSameStyle like ElementRestyler::RestyleContentChildren does.

I'll have a go at a patch.

Comment 3

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

Attachment: patch 1: Revert the changes to RestyleManager::ComputeStyleChangeFor from bug 898333, patch 2, since they cause skipping (in addition to the desired skipping) of continuations that do need restyling, in the case of directly restyling an element where con

Comment 4

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

Attachment: patch 2: Convert RestyleManager::ComputeStyleChangeFor from while loops to for loops to make it easier to add continue statements to it.

Comment 5

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

Attachment: patch 3: Replace changes to ComputeStyleChangeFor with a check of GetPrevContinuationWithSameStyle to avoid the duplication in a way that still doesn't break direct restyling of an element whose continuations have different styles.

Comment 6

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

Attachment: patch 4: Add reftest.

I confirmed that the test fails as expected without the patch, and
passes with the patch.

Comment 7

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

https://tbpl.mozilla.org/?tree=Try&rev=d1d6ba480474

Comment 8

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

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #7)
> https://tbpl.mozilla.org/?tree=Try&rev=d1d6ba480474

try: --build d --platform linux64 --unittests crashtest,reftest,mochitests --talos none --no-emails 

all green

Comment 9

[Cameron McCormack (:heycam)]

David, can you explain to me how block-in-inline frames are structured, how the IBSplitSpecialSibling frame properties point to differents parts of the split up frames, which frames get NS_FRAME_IS_SPECIAL, and whether continuations are also used between the parts of the IB split?  I'd like to have a better grasp of that before reviewing patch 2.

Comment 10

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

When we split an inline that contains blocks, we split it into a sequence of alternating inline-parts and block-parts.  These parts might then in turn be split into continuations during line breaking or pagination.  The first continuation of each of these parts has an IBSplitSpecialSibling property that points to the next such part.

That said, I think patch 2 can be verified to change nothing using only local knowledge (i.e., knowing nothing about the world outside of that function).

Comment 11

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

Oh, and we set NS_FRAME_IS_SPECIAL on all of the split parts, and the bit gets gets copied to continuations as well in nsFrame::Init.

Comment 12

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #10)
> That said, I think patch 2 can be verified to change nothing using only
> local knowledge (i.e., knowing nothing about the world outside of that
> function).

(I meant to say patch 1.)

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8360763 [details] [diff] [review]
patch 3:  Replace changes to ComputeStyleChangeFor with a check of GetPrevContinuationWithSameStyle to avoid the duplication in a way that still doesn't break direct restyling of an element whose continuations have different styles.

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

::: layout/base/RestyleManager.cpp
@@ +2869,5 @@
> +  // continuations, and skipping everything else.  However, when we have
> +  // a style change for something inside a style difference (e.g., a
> +  // style change on an element that extends from inside a styled
> +  // ::first-line to outside of that first line), we might restyle more
> +  // than that.

I don't understand "when we have a style change for something inside a style difference", though the parenthetical makes sense.  Could you reword or clarify?

Comment 14

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

(In reply to Cameron McCormack (:heycam) from comment #13)
> Comment on attachment 8360763 [details] [diff] [review]
> > +  // continuations, and skipping everything else.  However, when we have
> > +  // a style change for something inside a style difference (e.g., a
> > +  // style change on an element that extends from inside a styled
> > +  // ::first-line to outside of that first line), we might restyle more
> > +  // than that.
> 
> I don't understand "when we have a style change for something inside a style
> difference", though the parenthetical makes sense.  Could you reword or
> clarify?

Oops.  How about:

  However, when we have a style change targeted at an element inside a context where styles vary between continuations (e.g., ...

Comment 15

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

(In reply to Cameron McCormack (:heycam) from comment #12)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #10)
> > That said, I think patch 2 can be verified to change nothing using only
> > local knowledge (i.e., knowing nothing about the world outside of that
> > function).
> 
> (I meant to say patch 1.)

To be clear:  patch 1 is pure backout of an overzealous optimization in the previous patch; this change replaces it with a valid optimization.

Comment 16

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #14)
> Oops.  How about:
> 
>   However, when we have a style change targeted at an element inside a
> context where styles vary between continuations (e.g., ...

Sounds good.

Comment 17

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/faade79c9165
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb87f9b1fae
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/03bdbaabbfc2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f26cb95f5a56

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0bb87f9b1fae
https://hg.mozilla.org/mozilla-central/rev/03bdbaabbfc2
https://hg.mozilla.org/mozilla-central/rev/f26cb95f5a56

Comment 20

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

Comment on attachment 8360761 [details] [diff] [review]
patch 1:  Revert the changes to RestyleManager::ComputeStyleChangeFor from bug 898333, patch 2, since they cause skipping (in addition to the desired skipping) of continuations that do need restyling, in the case of directly restyling an element where con

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 898333
User impact if declined: incorrect styles (and possibly other symptoms, see above) when a page changes style on an element that starts but does not end in a styled ::first-letter or ::first-line 
Testing completed (on m-c, etc.): been on m-c for a month, passes automated tests
Risk to taking this patch (and alternatives if risky): I think it's pretty low risk; it fixes a relatively obvius regession in the old code.
String or IDL/UUID changes made by this patch: none

Comment 21

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

Comment on attachment 8360762 [details] [diff] [review]
patch 2:  Convert RestyleManager::ComputeStyleChangeFor from while loops to for loops to make it easier to add continue statements to it.

[Approval Request Comment]
same as comment 20

Comment 22

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

Comment on attachment 8360763 [details] [diff] [review]
patch 3:  Replace changes to ComputeStyleChangeFor with a check of GetPrevContinuationWithSameStyle to avoid the duplication in a way that still doesn't break direct restyling of an element whose continuations have different styles.

[Approval Request Comment]
same as comment 20

Comment 23

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

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/1d48512b791c
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/c6bbd04a0bff
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/2101e3c40a1a
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/a27a4ccaafe5

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/1d48512b791c
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/c6bbd04a0bff
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/2101e3c40a1a
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a27a4ccaafe5