Last Comment Bug 217604 - using alternative CSS mess the z-order of elements
: using alternative CSS mess the z-order of elements
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: x86 Linux
: -- minor (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
: Hixie (not reading bugmail)
Mentors:
http://sakura.nazsco.com.br
: 217694 (view as bug list)
Depends on:
Blocks: 217694
  Show dependency treegraph
 
Reported: 2003-08-28 07:44 PDT by Gabriel Barros
Modified: 2003-10-16 10:59 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (495 bytes, text/html)
2003-08-28 20:23 PDT, Andrew Schultz
no flags Details
fix (24.14 KB, patch)
2003-09-06 16:03 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review

Description Gabriel Barros 2003-08-28 07:44:11 PDT
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.
Comment 1 Gabriel Barros 2003-08-28 07:48:17 PDT
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.
Comment 2 Andrew Schultz 2003-08-28 20:23:38 PDT
Created attachment 130580 [details]
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.
Comment 3 Andrew Schultz 2003-08-28 20:25:06 PDT
==> stylesystem
Comment 4 Boris Zbarsky [:bz] 2003-08-28 20:49:15 PDT
roc, this is all you, methinks.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-08-31 22:23:51 PDT
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.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-08-31 22:29:13 PDT
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);
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-08-31 22:54:03 PDT
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.
Comment 8 Boris Zbarsky [:bz] 2003-09-02 08:55:19 PDT
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.  :(
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-09-02 09:10:05 PDT
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.
Comment 10 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2003-09-02 09:12:42 PDT
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.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-09-02 09:15:59 PDT
> we should really add NS_WARNINGs to places where we knowingly do the wrong
> thing.  :(

Nah. We would drown in NS_WARNINGs :-)
Comment 12 Boris Zbarsky [:bz] 2003-09-02 09:16:31 PDT
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?
Comment 13 Boris Zbarsky [:bz] 2003-09-02 09:17:18 PDT
> Nah. We would drown in NS_WARNINGs :-)

Bah.  Maybe it would motivate people to do some fixin'?  ;)
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-09-02 09:23:17 PDT
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?
Comment 15 Boris Zbarsky [:bz] 2003-09-02 11:53:54 PDT
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...
Comment 16 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2003-09-02 12:14:54 PDT
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.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-09-03 21:38:29 PDT
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.
Comment 18 Boris Zbarsky [:bz] 2003-09-04 07:25:09 PDT
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...
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-09-04 20:16:18 PDT
Right, and I don't care about documents or attributes or any of the other
non-element cases that CompareDocumentPosition tries to handle.
Comment 20 Boris Zbarsky [:bz] 2003-09-04 20:49:36 PDT
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...
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-09-06 10:35:02 PDT
I have written my own nsIContent version and it's tiny.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-09-06 15:34:39 PDT
*** Bug 217694 has been marked as a duplicate of this bug. ***
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-09-06 16:03:25 PDT
Created attachment 131013 [details] [diff] [review]
fix

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.
Comment 24 Boris Zbarsky [:bz] 2003-09-06 20:28:24 PDT
> 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").
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-09-06 22:28:37 PDT
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?
Comment 26 Boris Zbarsky [:bz] 2003-09-07 19:39:32 PDT
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...)
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-09-10 09:26:33 PDT
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 :-)
Comment 28 Boris Zbarsky [:bz] 2003-09-11 08:51:43 PDT
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.
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-10-14 18:10:29 PDT
Fix checked in.
Comment 30 Philip K. Warren 2003-10-16 10:59:45 PDT
This checkin may have caused Bug 222468.

Note You need to log in before you can comment on or make changes to this bug.