1272978 - Split nsLayoutUtils::PaintFrame into two functions, one for build display list, one for paint display list

Status: NEW

(Core :: Layout, defect)

Description

[u459114]

TODO item of bug 1269971
https://hg.mozilla.org/mozilla-central/rev/f71a8d46fdf2#l1.18
+  // TBD: we actually generate display list of aFrame twice here. It's better
+  // to reuse the same display list and paint that one twice, one for selection
+  // background, one for generating text mask.

To fix it, we can separate nsLayoutUtils::PaintFrame into two functions
1. nsLayoutUtils::BuildDisplayListForFrame()
2. nsLayoutUtils::PaintDisplayList()

With these two functions, we can call nsLayoutUtils::BuildDisplayListForFrame to build a display list once, and paint that display list twice via nsLayoutUtils::PaintDisplayList.

Comment 1

[Matt Woodrow (:mattwoodrow)]

I hadn't seen any of this code before, so I have a few questions:

How does invalidation work for background items that are clipped to text? I can't see any way that the nsDisplayBackgroundImage would get marked as invalid if the text frame children changed.

I guess it might work for most cases since we'll also have the nsDisplayText items in the main display list (assuming we don't optimize them away for being invisible), but we can't guarantee that these actually end up in the same layer as the background (background-attachment:fixed?).

nsLayoutUtils::PaintFrame does BuildDisplayListForStackingContext, but I don't think background-clip:text forces a stacking context. I can't think of anything right now that would be broken by that, but the code certainly doesn't expect it.

I see that we skip building nsDisplayTransform when building display lists for text clipping, but what about all the other container items we build there? SVGEffects and BlendModes could change the rendering of the text, do we want that?

Comment 2

[u459114]

Attachment: text animation

Comment 3

[u459114]

Attachment: Blur effect

Comment 4

[u459114]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> I hadn't seen any of this code before, so I have a few questions:
> 
> How does invalidation work for background items that are clipped to text? I
> can't see any way that the nsDisplayBackgroundImage would get marked as
> invalid if the text frame children changed.
Thanks. It's a bug, see attachment 8752644 [details]. Dirty region is not correct. I will file another issue to fix it.

Comment 5

[u459114]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> SVGEffects and BlendModes could change the rendering of the text, do we want that?
We still apply all filters and blendmode, but since most of them will not change the shape of text element, except blur effect, you can not see any visual change before and after add a filter or change blend mode.

Comment 6

[u459114]

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
> nsLayoutUtils::PaintFrame does BuildDisplayListForStackingContext, but I
> don't think background-clip:text forces a stacking context. I can't think of
> anything right now that would be broken by that, but the code certainly
> doesn't expect it.
I will look into it