668921 - Text disappears after scrolling

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: Test case

In Fennec:
1. Load attached page
2. Scroll

Result: All text besides "s" and "test" disappears.

I was first surprised to see that this caused an invalidation at all, but it seems that text ellipsis overflow is very liberal in its invalidation. What I've discovered so far:

1. InvalidateFixedBackgroundFrames invalidates a region that includes all the text
2. The visibility is correctly calculated for all nsDisplayTexts that happens in InvalidateFixedBackgroundFrames.
3. Later, in FrameLayerBuilder, we are getting ready to paint. The visiblility is properly calculated for all text frames.
4. In this loop the visibility is recalculated, and this is where things go terribly wrong:
http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#2072
5. We enter into this if:
http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#2082
6. RecomputeVisiblity decides the item is invisible. The text in the middle isn't painted.

Comment 1

[Benjamin Stover (:stechz)]

This bug is easily reproduced on html5test.com in Fennec (which is where I first discovered it).

Comment 2

[Benjamin Stover (:stechz)]

I have tracked this down to a problem with the generated mask from cairo-clip.c. The mask for the rounded rectangle path is being correctly generated, but the paths for the other clip regions (which is basically the union of the "s" and "text" frames) is not applied to the mask at all. This means the mask ends up only with a blit for the rounded rectangle.

This call, as I understand, is supposed to further reduce the mask based on the rectangular clip regions, but it does effectively nothing:

http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-clip.c#1049

Comment 3

[Benjamin Stover (:stechz)]

I don't really understand how this code could be correct. Operator IN is probably the right thing to do, but before IN we needs to first create a mask that is just the mask for that one path. After we generate it, we would then combine the two masks through operator IN.

Does that sound right?

Comment 4

[Benjamin Stover (:stechz)]

If I comment out this special case, the problem is gone!
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-clip.c#1040

Comment 5

[Benjamin Stover (:stechz)]

The fallback case here:
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-clip.c#1066

...looks to be what I was attempting to describe as the proper solution in comment #3. Jeff, does this make sense to you?

I know! I can explain with ASCII!

Say I have a mask Ma from a rounded rectangle path:
xxxxxxxxxxx...
xxxxxxxxxxxx..
xxxxxxxxxxxxx.

And say we have another path that came from some nsRegion clip, that (say) looks like this as a mask Mb:
xxxx....xxxxxx
xxxx....xxxxxx
xxxx....xxxxxx

The proper thing to do is to generate the nsRegion mask Mb and apply it to the rounded rectangle mask Ma with operator IN. This shortcut branch thinks it's being clever:
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-clip.c#1040

But this can't possibly be right, because there is nothing trying to change the middle x's into .'s for Ma.

Perhaps a potential optimization is to invert Mb and paint black. In other words, we'd subtract the path that describes Mb from the surface's extent and paint that result as black. However, I think the simplest thing to do is to just delete the special case.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #5)
> The fallback case here:
> http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-clip.
> c#1066
> 
> ...looks to be what I was attempting to describe as the proper solution in
> comment #3. Jeff, does this make sense to you?
> 
> I know! I can explain with ASCII!
> 
> Say I have a mask Ma from a rounded rectangle path:
> xxxxxxxxxxx...
> xxxxxxxxxxxx..
> xxxxxxxxxxxxx.
> 
> And say we have another path that came from some nsRegion clip, that (say)
> looks like this as a mask Mb:
> xxxx....xxxxxx
> xxxx....xxxxxx
> xxxx....xxxxxx
> 
> The proper thing to do is to generate the nsRegion mask Mb and apply it to
> the rounded rectangle mask Ma with operator IN. This shortcut branch thinks
> it's being clever:
> http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-clip.
> c#1040
> 
> But this can't possibly be right, because there is nothing trying to change
> the middle x's into .'s for Ma.

Perhaps, I'm missing something but I don't see why the fill with operator IN will be wrong. 

The fill becomes:

dest IN (src IN mask)

which is essentially just dest IN mask because of the white source.

I'm I misunderstanding something?

Comment 7

[Benjamin Stover (:stechz)]

> Perhaps, I'm missing something but I don't see why the fill with operator IN
> will be wrong. 
> 
> The fill becomes:
> 
> dest IN (src IN mask)
> 
> which is essentially just dest IN mask because of the white source.
> 
> I'm I misunderstanding something?

The mask could be eliminated if the path was the entire area, which is often not the case.

The "shortcut" tries to eliminate the second mask fill by IN filling a white source onto the first mask using the second path, but as I described in comment 5 I think this optimization is incorrect.

So, the current shortcut is:
1. IN fill using white as source onto the final mask with a path.

And the proper algorithm is:
1. Generate a mask M for a path by starting with a black source and IN filling with a white source using the path.
2. IN fill using M as source onto the final mask.

Does that make sense? I'm having a hard time clearly describing this.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #7)
> And the proper algorithm is:
> 1. Generate a mask M for a path by starting with a black source and IN
> filling with a white source using the path.
> 2. IN fill using M as source onto the final mask.

I'm still missing it. Those seem the same to me.

Comment 9

[Benjamin Stover (:stechz)]

Ah. I did not understand the "unboundedness" of operator IN. The documentation says it is not bounded by the mask, but in this case there is no mask. I assume it means that operator IN is not bounded by mask OR by the specified path?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #9)
> Ah. I did not understand the "unboundedness" of operator IN. The
> documentation says it is not bounded by the mask, but in this case there is
> no mask. I assume it means that operator IN is not bounded by mask OR by the
> specified path?

The first part of the operation is to convert the path into a mask.

cairo_fill(path):
  mask = create_mask(path)
  composite(dst, src, mask, operator)

Comment 11

[Benjamin Stover (:stechz)]

Could you post a link to the code you are talking about?

Comment 12

[Jeff Muizelaar [:jrmuizel]]

This is the gist of it:

http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-image-surface.c#3417

Comment 13

[Benjamin Stover (:stechz)]

(In reply to comment #12)
> This is the gist of it:
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-
> image-surface.c#3417

This code does not happen for the path I am following (because your link is called only from _clip_and_composite_polygon, and my path goes down _clip_and_composite_boxes).

Comment 14

[Benjamin Stover (:stechz)]

So, if operator IN fill is supposed to convert the path into a mask, I have no idea where that's going to happen in _cairo_image_surface_fill. The path seems to be determining what area is filled, but the area to fill needs to be the entire mask.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Attachment: Fix

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Here's what's going on here.

Since OPERATOR_IN is unbounded we need to fixup the area outside of the boxes that we composited. This is done by _cairo_image_surface_fixup_unbounded_boxes().
Currently that function has a special case for <=1 box where it assumes that the
extents tightly bound the box. This isn't true for the case that we're hitting and so we don't fix up the area outside of the box but inside of the extents.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Another way of fixing this is here:

http://lists.cairographics.org/archives/cairo/2011-July/022080.html

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/427f162c761c

Comment 20

[LegNeato]

Approved for aurora and beta. Who reviewed the patch?

Comment 21

[Jeff Muizelaar [:jrmuizel]]

(In reply to Christian Legnitto [:LegNeato] from comment #20)
> Approved for aurora and beta. Who reviewed the patch?

Chris Wilson.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/33cabd034563
http://hg.mozilla.org/releases/mozilla-beta/rev/a5ba68d400da

Comment 23

[Kevin Brosnan [Ex-Mozilla]]

Verified 

Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110825 Firefox/9.0a1 Fennec/9.0a1 ID:20110825030823

Mozilla/5.0 (Android; Linux armv7l; rv:8.0a2) Gecko/20110825 Firefox/8.0a2 Fennec/8.0a2 ID:20110825042004

Mozilla/5.0 (Android; Linux armv7l; rv:7.0) Gecko/20110824 Firefox/7.0 Fennec/7.0 ID:20110824181241