views screw up paint order

RESOLVED FIXED

Status

()

Core
Layout: View Rendering
P2
normal
RESOLVED FIXED
15 years ago
3 years ago

People

(Reporter: bz, Assigned: roc)

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 obsolete attachments)

I just found out (while debugging bug 191567) that we paint views totally
separately from inside the presshell, after normal painting finishes.

This screws up the layering of things that overlap if some of them have views
and others do not....  testcase attached.
Created attachment 113468 [details]
testcase
Yes, this is a bug of the form "content order is not always honoured in the
presence of sibling content with views". I know about it :-). But I don't think
I have a specific bug for it.
Yep, that sums it up.  And now you do.  ;)
The problem is that we can have a frame tree like this:

F1(V1) --> F2 (V2)
       --> F3
       --> F4 (V4)

Assuming no z-index overrides, we will paint view V1, then V2, then V4. But
painting view V1 paints F1 and its children without views, i.e. F3. Then we
paint F2 and F4. So F2 is painted after F3 but it should be before F3.

I think the solution is to restructure nsIViewObserver::Paint so that we can
paint more specific parts of the view tree. E.g., we would paint the above frame
tree using the following callbacks:
"Paint the frames rooted at V1, up to but not including V2" (paints frame F1)
"Paint the frames rooted at V2" (paints frame F2)
"Paint the frames rooted at V1, starting after V2 and ending before V4 (paints
frame F3)
"Paint the frames rooted at V4" (paints frame F4)
"Paint the frames rooted at V1, starting after V4" (paints nothing)

This would require significant restructuring of the frame painting code to
support repainting of fragments of the frame tree, but I think it's doable. Also
some thought would have to be given to minimizing the additional overhead.

Updated

15 years ago
Keywords: testcase
*** Bug 192698 has been marked as a duplicate of this bug. ***
*** Bug 198063 has been marked as a duplicate of this bug. ***
*** Bug 211830 has been marked as a duplicate of this bug. ***

Comment 8

14 years ago
Shouldn't this bug report receive the same priority as the most important 
duplicate? This would be P2 in this case? (I am not accustomed to the rules for 
changing priorities here)
well, it's really up to me to set the priority, but I agree that P2 is fair.
Priority: -- → P2

Comment 10

14 years ago
This is particularly nasty in XUL where IIRC you can't set the z-order :-(

Comment 11

14 years ago
*** Bug 221181 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

13 years ago
Blocks: 257458

Comment 12

13 years ago
(In reply to comment #1)
> Created an attachment (id=113468)
> testcase
> 

Hmms.  This looks like the same bug which manifested when I added header/footer
support to abiword's xhtml exporter using the example given in 9.6.1 (iirc) of
CSS 2.1, glad to see it's already known about.
(Reporter)

Updated

13 years ago
Blocks: 268539

Updated

13 years ago
Blocks: 275251

Comment 13

13 years ago
Around or just over 20 months I estimate since this bug was last touched by the
assignee, no?  This with 4 dups and 3 being blocked...  I by no means wish to
underappreciate how busy you must be, or the hard work you put in here or the
obligations and employment you have elsewhere, but could we possibly get a quick
30 second update?  If not a status update (for I know not whether progress has
been made), at least a simple ping to let us know that this issue has not been
forgotten?
Again, please don't take this the wrong way, I have a profound appreciation for
your volunteering your time to take care of our problem.  I'd just like to know
if this issue is still being looked at.
Well, the testcase WFM with 2005-04-04 trunk build. (I can see the bug in
Mozilla1.7).
Created attachment 179963 [details]
Updated testcase

The testcase used background-attachment to force a view.  That stopped forcing
a view unless a background image is also specified (see bug 258793).

This new testcase uses opacity instead of background-attachment to force a
view; this bug is alive and well.

That said, Mark, the status is that we're not sure how to best fix this yet...
The proposal in comment 4 is basically a pretty complete rewrite of the way
painting happens; if that's what we need to do we need to find the time to do
it...
Attachment #113468 - Attachment is obsolete: true
There's no way we can pull this off for 1.8.
Oh, ofcourse.  The question is whether we can for 1.9.....
Should be able to, yeah.

Comment 19

13 years ago
(In reply to comment #15)
> That said, Mark, the status is that we're not sure how to best fix this yet...
> The proposal in comment 4 is basically a pretty complete rewrite of the way
> painting happens; if that's what we need to do we need to find the time to do
> it...

Gotcha, thanks!  Glad to see an update, at least the 1.9 part.

Keep up the good work.
Blocks: 289480

Comment 20

13 years ago
The proposal for complete rewrite in comment #4 sounds a lot of work. I assume
tht it will take a pretty long time before this is fixed. Would it be possible
in the mean time to change the paint algorithm to something that works correctly
with a decreased performance?

Given frame tree like
F1(V1) --> F2 (V2)
       --> F3
       --> F4 (V4)

would it be possible to render F1 (View 1), F2 (View 2), F3 (repaint the frame 3
in View 1), F4 (View 4). As a possible tweak, F3 should repaint only if some
other view (View 2 in this case) has been repainted between the time View 1 and
F3 are repainted. A simple implementation could remember just the last view that
has been repainted and check if that is different from the view of current frame.

Would that cause a too big perfomance hit? Is it better to render incorrectly
with better performance?
(Reporter)

Updated

13 years ago
Blocks: 287940
*** Bug 293521 has been marked as a duplicate of this bug. ***

Comment 22

12 years ago
What if a view paints the frame, the children up to the first child with a view,
and siblings up to the first sibling with a view?

F1(V1) --> F2 (V2)
       --> F3
       --> F4 (V4)

V1 paints F1.
V2 paints F2 and F3.
V4 paints F4.
There's actually a much nicer way to fix this, which I thought of a while ago
but forgot to right down. Basically we just make it so that when we're painting
a frame subtree, and we encounter a frame with a view, we call back to the view
manager to paint that view. If the view is at the "right place" in the display
list then the view manager goes ahead and renders the view and crosses it off
the display list. If the view is at some other z-position then the view manager
just ignores the call and paints the view later (or earlier). This will require
much smaller changes.
*** Bug 301603 has been marked as a duplicate of this bug. ***
Blocks: 313190
(Reporter)

Updated

12 years ago
Blocks: 303813
(Reporter)

Updated

12 years ago
Depends on: 317375
*** Bug 319534 has been marked as a duplicate of this bug. ***
Comment on attachment 179963 [details]
Updated testcase

CSS 2.1 actually says that 'opacity' creates a new stacking context that's above normal in-flow content, so this testcase renders correctly before and after the patch in bug 317375.
Attachment #179963 - Attachment is obsolete: true
Actually, opacity isn't in CSS2.1; all I see in CSS2.1 about stacking contexts is that non-auto z-index establishes a new stacking context.

That said, if opacity establishes a stacking context, we do need a new testcase.  I tried using background-attachment instead of opacity, but then the bug went away, so the testcase might need to be sneakier... Or just use one from one of the bugs this blocks, I guess.

Comment 28

12 years ago
With bug 317375 being fixed, the paint order error in Acid2 is gone, although the new testcase still shows up fixed, but that's expected results. I think this bug should be marked as FIXED as fixing bug 317375 also removed the Acid2 problem (the exes are now on the top of everything). Also, this bug is platform "All", not platform "Linux", but I can't set that.
This bug was basically a tracking bug... please feel free to resolve fixed once all the bugs it blocks are tested and resolved.
OS: Linux → All
Hardware: PC → All

Comment 30

12 years ago
(In reply to comment #29)
> This bug was basically a tracking bug... please feel free to resolve fixed once
> all the bugs it blocks are tested and resolved.

I would resolve it, as FIXED, but it requires privileges. Do it for me or grant me the privilege to do it, else it's impossible.

Comment 31

12 years ago
(In reply to comment #29)
> This bug was basically a tracking bug... please feel free to resolve fixed once
> all the bugs it blocks are tested and resolved.

I would resolve it, as FIXED, but it requires privileges. Do it for me or grant me the privilege to do it, else it's impossible.

Comment 32

12 years ago
Oops... double post.
> I would resolve it, as FIXED, but it requires privileges. Do it for me or grant
> me the privilege to do it, else it's impossible.

Which part of "the bugs it blocks are tested and resolved" was missed?  Or do you mean that all of those are fixed now and just need to be resolved accordingly?
So, regarding comment 26 and comment 27, what's the conclusion?
Should opacity create a new stacking context or not?
In the css spec, I found this sentence:
http://www.w3.org/TR/CSS21/visuren.html#z-index
"Stacking contexts are not necessarily related to containing blocks. In future levels of CSS, other properties may introduce stacking contexts, for example 'opacity'."
The outcome decides if bug 275251 is a valid bug or not, I think.
'opacity' definitely creates a new stacking context.

Comment 36

12 years ago
Actually the bug that blocked this (bug 317375) was fixed, so it should be now possible to resolve, but it doesn't even let me accept this bug (assign to self). Only "Leave as NEW" appears for me.
I guess this could be resolved->fixed now.
The remaining open bugs:
Bug 289480 is a meta bug for the acid2 test.
Bug 313190 depends on a fix for bug 130078.

Stefanik, it's always important to look at the bugs that were blocked by this bug before you can resolve this bug.
> Actually the bug that blocked this (bug 317375) was fixed

The next time you feel like ignoring my comments (comment 29), please just don't comment or uncc me before commenting, ok?

Martijn, thanks for looking at the deps!
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.