1216851 - Optimize text with an opacity style set to avoid pushing a group

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

We seem to frequently get (inactive) opacity layers in the front end that contain only text.

We can avoid this by folding the opacity value down into the text display item and including it in the colour we render the text.

We'd probably need to skip this if the text contains complex extra (that require multiple draw passes) like underlines, selection and shadows.

We need to implement CanApplyOpacity() and ApplyOpacity() on the nsDisplayText display item, store the opacity on the item (if we decide its ok), and then pass it into nsTextFrame::DrawText.

Comment 1

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

Are there issues with multiple nsDisplayText display items that might overlap each other, inside a single element with opacity?  Or are you thinking of doing this only for a single nsDisplayText?

Comment 2

[Matt Woodrow (:mattwoodrow)]

The current code [1] will attempt to flatten up to 3 child display items, as long as they all support this, and don't overlap each other.


[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#3947

Comment 3

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

The "don't overlap" test sounds good enough for me.  At least as long as there aren't fun things text does with self-overlapping within a single run of text that wouldn't be handled by drawing with rgba colors.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: flatten-text-opacity

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8677729 [details] [diff] [review]
flatten-text-opacity

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

::: layout/base/nsDisplayList.cpp
@@ +3927,5 @@
>    for (; numChildren < ArrayLength(children) && child; numChildren++, child = child->GetAbove()) {
> +    if (child->GetType() == nsDisplayItem::TYPE_LAYER_EVENT_REGIONS) {
> +      numChildren--;
> +      continue;
> +    }

Wot? Remove this before landing

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 8677729 [details] [diff] [review]
> flatten-text-opacity
> 
> Review of attachment 8677729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsDisplayList.cpp
> @@ +3927,5 @@
> >    for (; numChildren < ArrayLength(children) && child; numChildren++, child = child->GetAbove()) {
> > +    if (child->GetType() == nsDisplayItem::TYPE_LAYER_EVENT_REGIONS) {
> > +      numChildren--;
> > +      continue;
> > +    }
> 
> Wot? Remove this before landing

This was intentional. I don't think we want nsDisplayLayerEventRegions contributing to our count of 3 items, and we don't want to check for intersections with it either.

I'll split it out and put it up as a separate patch though.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't include EventRegions in our limit of 3 items to optimize

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8677739 [details] [diff] [review]
Don't include EventRegions in our limit of 3 items to optimize

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

Right OK

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e8e25c04938
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea2c1e54b0f

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e1a81ef86201 something made the reftest fail like https://treeherder.mozilla.org/logviewer.html#?job_id=16483782&repo=mozilla-inbound

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31fda88c4ae6

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0f5b430939
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b903f78f2ce

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/fb0f5b430939
https://hg.mozilla.org/mozilla-central/rev/0b903f78f2ce

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fb0f5b430939
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0b903f78f2ce

Comment 15

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion