1428164 - stylo: the presence of a :-moz-window-inactive selector in a page stylesheet causes ::first-letter selector on the same page to not match everything it should

Status: RESOLVED FIXED

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

Description

[Brian Grinstead [:bgrins]]

Attachment: firstletter-mozwindow.html

STR:
* Open attached file
* Expected - all of the first letters are green
* Actual - some of the first letters aren't green

Notes:
1) If I flip layout.css.servo.enabled to false then all of the first letters are green as expected
2) The testcase is based off of the 362901-1.html reftest, and I detected the failure first on a try push when debugging Bug 1420229: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aff3cc7a566d267e74644f7669b30abd37571f1&selectedJob=153909079
3) I've also seen this issue happen intermittently on DevEdition 58 even without the extra selector (i.e. just loading https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/bugs/362901-1.html), but I haven't come up with a good STR for that - it just seems to happen occasionally when reloading that page for me.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: first-letter.gif

Here's a gif showing the issue reproducing even without the :-moz-window-inactive selector on the reftest. For whatever reason it's an intermittent problem without the added selector and a permanent problem with it.

Comment 2

[Daniel Holbert [:dholbert]]

mozregression says this regressed from bug 1390694 (which isn't too surprising, given the presence of :-moz-window-inactive in that bug's title).

Comment 3

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Looks like a possible web compat regression.

Comment 4

[Chris Peterson [:cpeterson]]

Cameron, this bug is a regression from your fix for -moz-window-inactive bug 1390694.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

This makes no sense, let me take this, those are the funniest... Chances are that this is just a ::first-line restyling issue uncovered by the moz-window-inactive patches (which can cause a full-doc restyle).

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So this is because we don't update ::first-letter on anon boxes, which I have locally fixed, but that alone doesn't fix all the testcases, so I'm digging into the three that still fail.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1428164: Update first-letter styles of non-block anon boxes.

Also unify the pseudo-element restyling code a bit more.

Review commit: https://reviewboard.mozilla.org/r/210278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210278/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1428164: Restyle owned anon boxes after processing children.

Otherwise the text post-traversal may clobber the first-letter style, and that
causes the ::first-letter style to get lost.

Review commit: https://reviewboard.mozilla.org/r/210280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210280/

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8940018 [details]
Bug 1428164: Update first-letter styles of non-block anon boxes.

https://reviewboard.mozilla.org/r/210278/#review216080

I'm not sure why we need the UpdateFirstLetterIfNeeded call for random non-block anon boxes.  What anon boxes can end up getting restyled _without_ the corresponding block getting restyled?  And what non boxes are inline, such that the first-letter will end up inside them?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8940019 [details]
Bug 1428164: Restyle owned anon boxes after processing children.

https://reviewboard.mozilla.org/r/210280/#review216084

r=me as far as it goes, but I'd still like to understand the actual failure mode here.  What anon boxes are involved?

::: layout/base/ServoRestyleManager.cpp:888
(Diff revision 1)
>                                                             childrenFlags);
>        }
>      }
>    }
>  
>    // We want to update frame pseudo-element styles after we've traversed our

This comment should also talk about why we want to process owned anon boxes after our kids.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: A bit more interactive test-case.

Pretty much all anon-boxes are affected I'd say, looking at the test-case. It's only that the test we had for this changed border color or something like that, which uses the first-letter style, which _is_ updated correctly. It's the underlying text style the one that isn't.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51a2953c8c41
Restyle owned anon boxes after processing children. r=bz
https://hg.mozilla.org/integration/autoland/rev/de5351e9d43f
Reftest. r=me

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e2662ebb8b3
followup: Simplify reftest reference. r=me

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8940018 [details]
Bug 1428164: Update first-letter styles of non-block anon boxes.

You're right it's not needed. I was thinking of ::-moz-fieldset-content when it's not a block or stuff like that, but apparently we don't do first-line bits in those...

Comment 15

[Boris Zbarsky [:bzbarsky]]

> It's the underlying text style the one that isn't.

Ah, so the underlying failure mode here is when we have a _block_ anon box, but we update its style (and that of its first-letter) before we restyle the kids.  OK, that makes sense.  ;)

Thank you for writing the reftest!

Comment 16

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/51a2953c8c41
https://hg.mozilla.org/mozilla-central/rev/de5351e9d43f
https://hg.mozilla.org/mozilla-central/rev/4e2662ebb8b3

Comment 17

[Gerry Chang [:gchang]]

Track 58+/59+ as webcomp regression.

Comment 18

[Astley Chen (inactive)]

Emilio, as this is a 58 new regression, could you create an uplift patch and request for 58? Thanks.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch for beta.

It applies cleanly, so it's just an squash of what landed.

Approval Request Comment
[Feature/Bug causing the regression]: stylo (though bug 1390694 exposes this particular issue, the bug is reproducible without that)
[User impact if declined]: wrong dynamic styling in some cases involving first-letter.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes (just did)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: Changes the order of two operations so one doesn't clobber another. One liner.
[String changes made/needed]: none

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8940671 [details] [diff] [review]
Patch for beta.

Fix a web compat regression. Beta58+.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/528b66320843
https://hg.mozilla.org/releases/mozilla-beta/rev/83b9db0e679d
https://hg.mozilla.org/releases/mozilla-beta/rev/8093f2581e95