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

RESOLVED FIXED in Firefox 45

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
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?
(Assignee)

Comment 2

3 years ago
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
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8677729 [details] [diff] [review]
flatten-text-opacity
Assignee: nobody → matt.woodrow
Attachment #8677729 - Flags: review?(roc)
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
Attachment #8677729 - Flags: review?(roc) → review+
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
Created attachment 8677739 [details] [diff] [review]
Don't include EventRegions in our limit of 3 items to optimize
Attachment #8677739 - Flags: review?(roc)
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
Attachment #8677739 - Flags: review?(roc) → review+
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
Flags: needinfo?(matt.woodrow)

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fb0f5b430939
https://hg.mozilla.org/mozilla-central/rev/0b903f78f2ce
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

3 years ago
Flags: needinfo?(matt.woodrow)
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.