1401317 - stylo: Broken rendering of -moz-selection text when stylo is enabled

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Attachment: Screenshot 1: highlighting of the testcase with stylo DISABLED

The rendering of selected text when -moz-selection is used seems to be significantly broken when stylo is enabled.

Using attachment 610677 [details] (testcase from bug 721750), load the example and then press Cmd-A to Select All (on macOS).

Two issues:

First, the highlighting when stylo is enabled is very different from the non-stylo version (compare screenshots 1 and 2).

Second, pressing Cmd-A a second time (or any number of additional times) changes the stylo highlighting again (compare screenshots 2 and 3). Indeed, the result varies depending exactly how the content is selected. (So selecting individual words or lines may give different results from selecting the whole text.) This inconsistent behavior is clearly unexpected.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Screenshot 2: highlighting with Stylo enabled (first time of Cmd-A only)

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Screenshot 3: highlighting with stylo after pressing Cmd-A a second time

Comment 3

[Jonathan Kew [:jfkthame]]

This does seem to be a Mac-specific issue; I'm not seeing similar behavior on Windows.

Comment 4

[Jonathan Kew [:jfkthame]]

Not Mac-only after all, I'm seeing similar behavior on Linux. Maybe it's a recent regression and my Windows build is too old to show it...?

Comment 5

[Jonathan Kew [:jfkthame]]

Yup. Broken in latest Windows nightly, too.

Comment 6

[Jonathan Kew [:jfkthame]]

mozregression says this started with:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5e5eab26b9dec16c05f70c351d78863f8cb23900&tochange=ff89cb83be0a2a1ba74099c839d4f796981dda67

which points at https://github.com/servo/servo/pull/18496.

Comment 7

[Bobby Holley (:bholley)]

So the basic problem here is that the simplification in bug 1368291 comment 9 doesn't hold anymore.

I'll attach a reftest.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Reftest. r=me

MozReview-Commit-ID: KnGCuU0rcgi

Comment 9

[Bobby Holley (:bholley)]

There are various ways to solve this. Emilio, any preference on which way?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> So the basic problem here is that the simplification in bug 1368291 comment
> 9 doesn't hold anymore.

This doesn't have nothing to do with that simplification, but with the general lazy pseudo style cache mechanism.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> There are various ways to solve this. Emilio, any preference on which way?

I'm not totally sure about what kind of ways are you thinking about.

The lazy pseudo-style cache didn't help memory that much when we land it, so easiest thing is just remove it, and get back that word in the style contexts.

However, if we really really want to keep it, I guess we'd need to resolve the style and just compare the rule node pointers to cache them.

Looking at the list of pseudo-elements, the only ones worth sharing are :-moz-list-bullet and :-moz-list-number... And per bz's comment right above it, maybe we should just convert them to inheriting anonymous boxes (post-57, ofc).

It's not clear the lazy pseudo cache is a great memory win, so I'd propose just getting rid of it.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Looking at the list of pseudo-elements, the only ones worth sharing are
> :-moz-list-bullet and :-moz-list-number... And per bz's comment right above
> it, maybe we should just convert them to inheriting anonymous boxes
> (post-57, ofc).

it == the pseudo-element definition, in particular:

// XXXbz should we really allow random content to style these?  Maybe
// use our flags to prevent that?
CSS_PSEUDO_ELEMENT(mozListBullet, ":-moz-list-bullet", 0)
CSS_PSEUDO_ELEMENT(mozListNumber, ":-moz-list-number", 0)

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1401317: Remove the lazy pseudo-style cache.

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

Comment 13

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> > So the basic problem here is that the simplification in bug 1368291 comment
> > 9 doesn't hold anymore.
> 
> This doesn't have nothing to do with that simplification, but with the
> general lazy pseudo style cache mechanism.

Per IRC discussion, my original comment holds.
 
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> > There are various ways to solve this. Emilio, any preference on which way?
> 
> I'm not totally sure about what kind of ways are you thinking about.

The two that I had in mind where (1) matching before sharing, and (2) disabling caching when the primary style was reused via the second pass.

> 
> The lazy pseudo-style cache didn't help memory that much when we land it

Per bug 1367862 comment 22, the impact isn't negligible. I'd imagine it varies a lot by page, but it basically boils down to an extra CV per <li> element, and it's not such a stretch to think that there are pages out there with a lot of <li> elements.

> , so
> easiest thing is just remove it, and get back that word in the style
> contexts.
> 
> However, if we really really want to keep it, I guess we'd need to resolve
> the style and just compare the rule node pointers to cache them.

I think option (2) above is pretty simple and low-risk. I'll attach a patch for that.

> 
> Looking at the list of pseudo-elements, the only ones worth sharing are
> :-moz-list-bullet and :-moz-list-number... And per bz's comment right above
> it, maybe we should just convert them to inheriting anonymous boxes
> (post-57, ofc).

That sounds like a good plan, filed bug 1401449 for it.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Disable lazy pseudo caching when the originating element's primary style was reused via the rule node. v1

MozReview-Commit-ID: IkBa39E1bR1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Bonus fix: Make second pass sharing sensitive to DISABLE_STYLE_SHARING_CACHE. v1

This makes it easier to detect sharing-related weirdness in the future.

MozReview-Commit-ID: 8U0xekMHGg8

Comment 16

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00e0711be00fcecd2d9598d7f8706c9b3b504224

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Do you really think it's worth to share style across <li> element at the expense of an extra word for every single style context? It doesn't sound like an obvious win at all to me. And honestly the less code that interacts with the ElementData flags the best over all, I think.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

I'd rather take a few numbers here now that we have proper memory reporting, and remove the code if that optimization doesn't look worth it.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8910121 [details] [diff] [review]
Disable lazy pseudo caching when the originating element's primary style was reused via the rule node. v1

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

Per IRC discussion, let's do this, I guess we can always get rid of this later...

::: servo/ports/geckolib/glue.rs
@@ +844,5 @@
>  }
>  
>  #[no_mangle]
> +pub extern "C" fn Servo_Element_IsPrimaryStyleReusedViaRuleNode(element: RawGeckoElementBorrowed) -> bool
> +{

nit: Either brace to the previous line, or alignment of the argument as a block, your call. While you're here you can also fixup the previous function.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

07:43 <emilio> bholley: do you really think it's worth it to sacrifice a word in every single style context to share styles on <li> elements? It doesn't seem obvious to me that it's a win
07:52 <bholley> emilio: hard to say about the average, but the worst-case behavior is better. In any case, it seems weird to use this bug to revive that whole tradeoff analysis two days before the merge. I'd be sympathetic if it were our only easy option, but we have another one
07:52 <bholley> emilio: so I'm more inclined to land the minimal change for now, and then try to ship the anon box thing in 58

Comment 21

[Bobby Holley (:bholley)]

Thanks!

https://github.com/servo/servo/pull/18571

(The servo changes are not interdependent with the gecko ones, so I'll let that land overnight and then push the gecko bits in the morning)

Comment 22

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #21)
> Thanks!
> 
> https://github.com/servo/servo/pull/18571

Servo-side changes landed as https://hg.mozilla.org/integration/autoland/rev/4d6c6ad56429

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef5d46900a24
Disable lazy pseudo caching when the originating element's primary style was reused via the rule node. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cd809c5e79a5
Reftest. r=me

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef5d46900a24
https://hg.mozilla.org/mozilla-central/rev/cd809c5e79a5