Closed Bug 217604 Opened 21 years ago Closed 21 years ago

using alternative CSS mess the z-order of elements

Categories

(Core :: Web Painting, defect)

Product:
Core
Component:
Web Painting
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: gabriel.barros, Assigned: roc)

References

(
URL
)

Details

(Keywords: testcase)

Attachments

(2 files)

testcase
495 bytes, text/html
Details
fix
24.14 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5a) Gecko/20030718
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5a) Gecko/20030718

this page has two elements that overlaps. One being the girl drawing and the
other being the text "sakura no hana". both with absolut position given by the
style sheet.

When the page loads, with the default style sheet (no matter wich i use) the
z-order of the elements are correct: the text below, and the girl above.

But as soon as i chage to any of the alternate style sheets, that order gets
twisted. The text go over the girl.

Already tried changing the default CSS file and using the actual default as an
alternate, and the problem continues, so it eliminates a bad craft css problem.

I haven't forced z-order on the element's rules to be able to use the site as an
example to that bug.

Reproducible: Always

Steps to Reproduce:
1. open a page with overlaping elements (ex: http://sakura.nazsco.com.br/).
2. change to an alternate CSS that keeps those elements overlaping.

Actual Results:  
the z-order get twisted.

Expected Results:  
keept the document z-order, as it does when rendering the page for the 1st time.

i checked that bug with:
mozilla 1.4 linux
mozilla 1.5a linux
mozilla 1.5a win32
firebird 0.6.1 2003-08-26 win32

also checked on Opera7 linux and it renders the css change just fine.
forgot to mention that it also happens when going back to the default style.

1. open the page. The z-order is OK.
2. chage to alternate style sheet. The z-order gets inverted.
3. chage back to the 1st style. The z-order stays inverted.
Attached file testcase — 
bug shows up with this testcase with linux trunk 20030827 (1.5b)
at first, the red <div> is on top of the blue one
switch style sheets -> blue <div> is on top.
reload -> red <div> is back on top.
==> stylesystem
Assignee: dom_bugs → dbaron
Status: UNCONFIRMED → NEW
Component: DOM Style → Style System
Ever confirmed: true
Keywords: testcase
roc, this is all you, methinks.
Assignee: dbaron → roc+moz
Component: Style System → Layout: View Rendering
All me? Surely not! :-)

What's happening here is that the style switch (I didn't even know you could
have titled style blocks, that's nifty) is causing the frame to be recreated for
the blue DIV. The new frame is placed at the end of the absolute child list, so
it displays on top. I guess we need to whack nsCSSFrameConstructor (HAHA) so
that frame recreation puts the new frame into the right place in its child lists.
Ah yes, check out this comment in nsCSSFrameConstructor:

    // If there are new absolutely positioned child frames, then notify
    // the parent
    // XXX We can't just assume these frames are being appended, we need to
    // determine where in the list they should be inserted...
    if (state.mAbsoluteItems.childList) {
      state.mAbsoluteItems.containingBlock->AppendFrames(aPresContext, *shell,
nsLayoutAtoms::absoluteList, state.mAbsoluteItems.childList);
Finding the correct out-of-flow prev-sibling to insert after in each of the
(absolute, fixed, float) cases is going to be ugly ... obviously the immediate
in-flow prevSibling might not have an out-of-flow frame to insert after.

I guess we can do something like this: find the last frame currently on the
out-of-flow child list for which the placeholder frame is <= prevSibling in
frame tree preorder. Writing the tree order test could be slightly tricky; do we
have any code that already does it? I think not.
Shouldn't it actually be "find the last frame currently on the
out-of-flow child list for which the content node is <= the content node of the
prevSibling in the DOM tree"?  And then we just use CompareTreePosition?

And my apologies for assuming it was a views issue... we should really add
NS_WARNINGs to places where we knowingly do the wrong thing.  :(
It's easier to use the frame tree w/placeholders, I think --- that needs to
reflect the content tree order ... whenever it doesn't, that's another bug.

Anyway, I've implemented the frame ordering fix and it looks OK. But it alone
doesn't fix this bug. Not only do we have to get the frames in the right order,
we also have to get their views in the right order. Right now
nsContainerFrame::CreateViewForFrame just appends the frame's view as the last
child of the parent view. Actually we need the view order to reflect the frame
tree order.

I'm not completely sure what the right way is to achieve this. At frame
construction time we don't know what the previous frame sibling is. We could
propagate that down from nsCSSFrameConstructor but that's a lot of work. We
could wait for frame insertion or frame append (note that even if you're
appending the frame, it might be that your view needs to be inserted into the
middle of its siblings) and then shuffle the view to its correct position. Or we
could shuffle the view in nsContainerFrame::SyncFrameViewAfterReflow.

I need to think about ths some more.
Yeah, I was going to mention that -- since I remember there being another bug
that was just about view tree order, even when frame tree order was OK.
> we should really add NS_WARNINGs to places where we knowingly do the wrong
> thing.  :(

Nah. We would drown in NS_WARNINGs :-)
roc, why is it easier to use the frame tree?  If the frame and content trees are
the same order (as they should be), why not use the already-written content code
for doing tree-order comparisons?
Blocks: 217694
> Nah. We would drown in NS_WARNINGs :-)

Bah.  Maybe it would motivate people to do some fixin'?  ;)
I guess that right after the frame has been put into the frame tree, we can
fairly easily use my new CompareTreePosition code to figure out whether the view
needs to be shuffled and if so where it should go.

The tricky part is making sure that the pure append case is fast so pageload
doesn't get hurt. I'll see how it goes.

> why not use the already-written content code for doing tree-order comparisons?

Because thinking about three parallel trees at once hurts my brain? :-)

Seriously, you may be right. What's the content tree comparator function called?
CompareDocumentPosition -- see nsIDOM3Node.idl

One other thing.  When we move to stylesheets not blocking the parser, then
there is a nontrivial chance that a lot of content will in fact follow the
not-pure-append path, since we will have a good chunk of the DOM tree
constructed before the style sheets come in and trigger a style reresolve...
But it will still be rare that we construct frames without the stylesheets, and
we should use a long enough timeout for the case that we do so that we're
unlikely to be slower than the network.
Hmm. I like the idea of working over the content tree more than using this
actual code (actually nsContentUtils::ComparePositionWithAncestors is the
function I want). Lots of QI and dynamic arrays and refcounting, for something
that's going to get called a lot. Let me try to write a version that uses
nsIContent and is a bit more efficient.
Layout can't get at nsContentUtils last I checked... should we correct that?

Also, be careful when changing from nsIDOMNode to nsIContent -- documents are
nodes, but not contents...
Right, and I don't care about documents or attributes or any of the other
non-element cases that CompareDocumentPosition tries to handle.
True.  I don't think those checks should be a big perf hit, but if we're worried
we may indeed have to hook in at the lower level or write our own version of this...
I have written my own nsIContent version and it's tiny.
*** Bug 217694 has been marked as a duplicate of this bug. ***
Attached patch fix — — Splinter Review 
Here it is. Gets rid of a fair number of XXXs :-).

Using the content tree to do the lookup has the advantage that we can find the
right position for the view in CreateViewForFrame, even though the frame hasn't
been put in the frame tree yet.

The content tree approach fails when some of the content is anonymous. Is there
actually any ordering on anonymous children?

It's unclear what the page load impact might be. InsertOutOfFlowFrames tries to
see if the frames can be appended to the containing block first, before going
back and looping through all the children. That shoud help.
> Is there actually any ordering on anonymous children?

Hmm....  There is with XBL, no (eg abs pos things as part of an XBL binding,
being dynamically manipulated (eg display set to "none" and then "block").
What I mean is this: does our content system actually keep track of the order of
anonymous children in any way?

If yes, how do I get at it?

If no, should it?
Ah.  No, it does not. I'm not quite sure whether it should, to be truthful, or
even whether it could (given the multifarious ways anonymous content can arise...)
That's fine with me, but if the document order of anonymous elements is
undefined, then so is the natural z-order of those elements, so we just have to
remember to mark bugs about incorrect z-ordering of anonymous content as WONTFIX :-)
Attachment #131013 - Flags: superreview?(dbaron)
Attachment #131013 - Flags: review?(dbaron)
Yeah... I guess that's one argument for using the frame tree instead of the
content tree (as long as we are writing a new comparison function)...

I sort of suspect that we won't really run into issues with this in XBL, though
(since I don't think you can easily create new root content nodes in the
binding, and the rest of the binding is not exactly anonymous, and since the
original root content nodes will get ordered correctly by frame construction). 
Other types of anonymous content should really not be getting z-indices.
Attachment #131013 - Flags: superreview?(dbaron)
Attachment #131013 - Flags: superreview+
Attachment #131013 - Flags: review?(dbaron)
Attachment #131013 - Flags: review+
Fix checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
This checkin may have caused Bug 222468.
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: