The default bug view has changed. See this FAQ.

Text disappears after scrolling

VERIFIED FIXED in Firefox 7

Status

()

Core
Graphics
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: stechz, Assigned: jrmuizel)

Tracking

({regression, verified-aurora, verified-beta})

Trunk
mozilla9
regression, verified-aurora, verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox7+ fixed, firefox8 fixed, firefox9 fixed, fennec+)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 543536 [details]
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.
(Reporter)

Updated

6 years ago
Assignee: nobody → ben
(Reporter)

Comment 1

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

Comment 2

6 years ago
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
(Reporter)

Comment 3

6 years ago
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?
(Reporter)

Comment 4

6 years ago
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
(Reporter)

Comment 5

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

Comment 6

6 years ago
(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?
(Reporter)

Comment 7

6 years ago
> 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: --- → ?
status-firefox7: --- → affected
(Assignee)

Comment 8

6 years ago
(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.
(Reporter)

Comment 9

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

Comment 10

6 years ago
(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)
(Reporter)

Comment 11

6 years ago
Could you post a link to the code you are talking about?
(Assignee)

Comment 12

6 years ago
This is the gist of it:

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

Comment 13

6 years ago
(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).
(Reporter)

Comment 14

6 years ago
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+
tracking-firefox7: --- → ?
Keywords: regression, regressionwindow-wanted

Updated

6 years ago
tracking-firefox7: ? → +

Updated

6 years ago
Blocks: 669566
tracking-fennec: 7+ → +
Depends on: 678505
No longer depends on: 678505
Depends on: 678505
(Assignee)

Comment 15

6 years ago
Created attachment 553839 [details] [diff] [review]
Fix
(Assignee)

Comment 16

6 years ago
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)

Comment 17

6 years ago
Another way of fixing this is here:

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

Updated

6 years ago
Assignee: ben → jmuizelaar
Duplicate of this bug: 669566
http://hg.mozilla.org/mozilla-central/rev/427f162c761c
Keywords: regressionwindow-wanted
Target Milestone: --- → mozilla9
Version: unspecified → Trunk

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Attachment #553839 - Flags: approval-mozilla-beta?
Attachment #553839 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #553839 - Flags: approval-mozilla-beta?
Attachment #553839 - Flags: approval-mozilla-beta+
Attachment #553839 - Flags: approval-mozilla-aurora?
Attachment #553839 - Flags: approval-mozilla-aurora+

Comment 20

6 years ago
Approved for aurora and beta. Who reviewed the patch?
(Assignee)

Comment 21

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

Chris Wilson.
(Assignee)

Comment 22

6 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/33cabd034563
http://hg.mozilla.org/releases/mozilla-beta/rev/a5ba68d400da
status-firefox7: affected → fixed
status-firefox8: --- → fixed
status-firefox9: --- → fixed
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
Keywords: verified-aurora, verified-beta

Updated

6 years ago
Blocks: 679569
You need to log in before you can comment on or make changes to this bug.