Closed Bug 1428164 Opened 2 years ago Closed 2 years ago

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

Categories

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

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 + fixed
firefox59 + fixed

People

(Reporter: bgrins, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(6 files)

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.
Attached image 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.
Flags: needinfo?(emilio)
Blocks: stylo
mozregression says this regressed from bug 1390694 (which isn't too surprising, given the presence of :-moz-window-inactive in that bug's title).
Blocks: 1390694
Keywords: regression
Priority: -- → P3
[Tracking Requested - why for this release]: Looks like a possible web compat regression.
Cameron, this bug is a regression from your fix for -moz-window-inactive bug 1390694.
Flags: needinfo?(cam)
Summary: Servo: 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 → 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
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).
Assignee: nobody → emilio
Flags: needinfo?(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 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 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.
Attachment #8940019 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(emilio)
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.
Flags: needinfo?(emilio)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4e2662ebb8b3
followup: Simplify reftest reference. r=me
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...
Attachment #8940018 - Flags: review?(bzbarsky)
Version: unspecified → 58 Branch
> 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!
Flags: needinfo?(cam)
Track 58+/59+ as webcomp regression.
Emilio, as this is a 58 new regression, could you create an uplift patch and request for 58? Thanks.
Attached patch Patch for beta.Splinter Review
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
Attachment #8940671 - Flags: approval-mozilla-beta?
Comment on attachment 8940671 [details] [diff] [review]
Patch for beta.

Fix a web compat regression. Beta58+.
Attachment #8940671 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
See Also: → 1429630
Depends on: 1433591
You need to log in before you can comment on or make changes to this bug.