Cameron McCormack (:heycam) Reporter
	Description • 11 years ago

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.

	Cameron McCormack (:heycam) Reporter
	Comment 1 • 11 years ago

Regression range:

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

Bug 898333 sounds like a likely candidate.

	David Baron :dbaron: Assignee
	Comment 2 • 11 years ago

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.

	David Baron :dbaron: Assignee
	Comment 6 • 11 years ago

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

	David Baron :dbaron: Assignee
	Comment 7 • 11 years ago

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

	David Baron :dbaron: Assignee
	Comment 8 • 11 years ago

(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

	Cameron McCormack (:heycam) Reporter
	Comment 9 • 11 years ago

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.

	David Baron :dbaron: Assignee
	Comment 10 • 11 years ago

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).

	David Baron :dbaron: Assignee
	Comment 11 • 11 years ago

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.

	Cameron McCormack (:heycam) Reporter
	Comment 12 • 11 years ago

(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.)

	Cameron McCormack (:heycam) Reporter
	Comment 13 • 11 years ago

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?

	David Baron :dbaron: Assignee
	Comment 14 • 11 years ago

(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., ...

	David Baron :dbaron: Assignee
	Comment 15 • 11 years ago

(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.

	Cameron McCormack (:heycam) Reporter
	Comment 16 • 11 years ago

(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.

	David Baron :dbaron: Assignee
	Comment 17 • 11 years ago

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

	Carsten Book [:Tomcat]
	Comment 18 • 11 years ago

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

	David Baron :dbaron: Assignee
	Comment 19 • 11 years ago

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

	David Baron :dbaron: Assignee
	Comment 20 • 11 years ago

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

	David Baron :dbaron: Assignee
	Comment 21 • 11 years ago

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

	David Baron :dbaron: Assignee
	Comment 22 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 11 years ago

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