Closed Bug 668921 Opened 13 years ago Closed 13 years ago

Text disappears after scrolling

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla9
Tracking Status
firefox7 + fixed
firefox8 --- fixed
firefox9 --- fixed
fennec + ---

People

(Reporter: stechz, Assigned: jrmuizel)

References

Details

(Keywords: regression, verified-aurora, verified-beta)

Attachments

(2 files)

Attached file 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.
Assignee: nobody → ben
This bug is easily reproduced on html5test.com in Fennec (which is where I first discovered it).
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
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?
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
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.
(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?
> 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.
tracking-fennec: --- → ?
(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.
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?
(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)
Could you post a link to the code you are talking about?
(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).
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.
tracking-fennec: ? → 7+
Blocks: 669566
tracking-fennec: 7+ → +
No longer depends on: 678505
Attached patch FixSplinter Review
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.
Assignee: ben → jmuizelaar
http://hg.mozilla.org/mozilla-central/rev/427f162c761c
Target Milestone: --- → mozilla9
Version: unspecified → Trunk
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #553839 - Flags: approval-mozilla-beta?
Attachment #553839 - Flags: approval-mozilla-aurora?
Attachment #553839 - Flags: approval-mozilla-beta?
Attachment #553839 - Flags: approval-mozilla-beta+
Attachment #553839 - Flags: approval-mozilla-aurora?
Attachment #553839 - Flags: approval-mozilla-aurora+
Approved for aurora and beta. Who reviewed the patch?
(In reply to Christian Legnitto [:LegNeato] from comment #20)
> Approved for aurora and beta. Who reviewed the patch?

Chris Wilson.
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
Status: RESOLVED → VERIFIED
Blocks: 679569
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: