Closed Bug 191830 Opened 22 years ago Closed 19 years ago

views screw up paint order

Categories

(Core :: Web Painting, defect, P2)

Product:
Core
Component:
Web Painting
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: roc)

References

Details

(Keywords: testcase)

Attachments

(2 obsolete files)

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.
Attached file testcase (obsolete) —
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.
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. ***
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
This is particularly nasty in XUL where IIRC you can't set the z-order :-(
*** Bug 221181 has been marked as a duplicate of this bug. ***
Blocks: 257458
(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.
Blocks: 268539
Blocks: 275251
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).
Attached file Updated testcase (obsolete) —
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.
(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.
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?
Blocks: 287940
*** Bug 293521 has been marked as a duplicate of this bug. ***
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: 303813
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.
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
(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.
(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.
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.
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
Closed: 19 years ago
Resolution: --- → FIXED
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: