137853 - [EVENTTARG] Event targeting is wrong with absolute/mixed position element styles

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Robert Kieffer]

Win2K
Build 2002041603

- Open the attached file.
- Roll around the orange div
* Notice that the rollover effect behaves correctly at all points (in and out)
of the div
- Roll around the cyan div
* Notice that where it intersects the textarea behind it, the textarea is the
event target!

This appears to be a result of the event targeting assuming the element z-order
is defined by the order in which the elements are defined in the HTML document.
 However, in this test, the 1st and 3rd elements (the two divs) are positioned
absolutely, which *should* give them a different z-index in the event handling
system (but doesn't).

(This also shows in how the textarea renders, but I'll report that as a separate
bug)

Comment 1

[Robert Kieffer]

Attachment: Test of mixed absolute/relative element event handling

Comment 2

[Kirk Parsons]

Adding nsbeta1, this is a fundamental problem with event handling, when we 
cannot rely on the correct element receiving the event.  We should get this 
fixed for Mach V.  Bob, I'm adding you in on the CC list - comments are welcome.

Comment 3

[joki (gone)]

David, do you have any thoughts on this one?

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Absolute positioning by itself doesn't establish a new stacking context.  See
http://www.w3.org/TR/REC-CSS2/visuren.html#q30 .  I think the painting here is a
bit weird, too, though.

Comment 5

[Kevin McCluskey (gone)]

This appears to only affect textarea/textfields. If I replace the <textarea>
with a <div> in the testcase it behaves as expected. 

Taking this bug for further investigation.

Comment 6

[Kevin McCluskey (gone)]

The textarea content is rendering on top of the cyan box while its background is
below the cyan box because the textarea does not create a view for the
GfxTextControlFrame. This results in strange rendering because the text area's
background in rendered into the documents view while its contents are rendered
into scrolling views. The absolutely positioned views for the cyan and orange
boxes are displayed relative to scrolling views so we end up with something like
this:

document view ----> GfxTextControlFrame2 doesn't have a view so it
           |        renders text area border and background in the doc's view
           |
         view1 ---> renders cyan box
           |
         scrolling view
           |      |-->scrolled view
           |               |--- Renders text area contents
           | 
         view3----> renders orange box         


Any frame which will have its content rendered into a scrolling view should
create a view for itself so its content is always rendered with its background.
 Other frames which have this problem include text fields, password fields and
scrolling div's. All of these frames create scrolling views so these frames
should create a view which will be the parent of the scrolling views.

Comment 7

[Kevin McCluskey (gone)]

Attachment: Always create views for the GfxScrollFrame and GfxTextControlFrames

Comment 8

[Kevin McCluskey (gone)]

Note: The patch I attached combines the textarea's background with its content
so it is rendered as follows:

bottom: cyan box 
middle: textarea background and content 
top: orange box 

Events behave as expected. (i.e. they accurately reflect the rendered order)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Testcase showing that this problem can be reproduced using only DIVs

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think Kevin's analysis isn't entirely correct. The problem is quite general as
the testcase shows. Let me edit his explanation:

Give a frame tree with associated views:

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

Assuming no z-index is explicitly set, we will render F1 and F3 first, then F2,
F4 and F5 in that order.

Basically the problem is that we're not fully making z-order match content
order. We only do it for frames that have views. So when you have a sibling
frame without a view that has child frames with views, the child frames are
dissociated from the parent frame in the z-order. The only way to fix this is to
make sure that we always have z-order following content order. I recommend not
using Kevin's patch because it will only fix a few cases and leave most of the
cases wide open, while adding complications that won't be necessary once we have
a real fix.

Even if we can fix it easily, making z-order following content order always
seems risky to me even though it's clearly what CSS2 demands ... I suspect a lot
of people have absolutely-positioned content sprinkled around their pages and
will be surprised to find it floating under non-positioned content that comes
after the positioned content in the document. dbaron, what say you? I'd say that
in quirks mode (maybe always) we should put absolutely positioned children after
all other children in z-order (assuming no z-index is set, of course). It would
be great if we could get CSS WG feedback before we make such a decision.

If we just do that -- put absolutely-positioned content after all other children
in z-order -- then this particular manifestation of the problem will go away
without having to fix the general z-order-follows-content problem. Whaddaya think?

[If we want to fix the z-order-follows-content problem then one way to do it
would be to create a view for every frame which has a sibling with a view. I
think that would be rather complex and inefficient. A better way would be to
extend the painting and event handling interfaces on nsIViewObserver so that
instead of the view manager saying "paint view V" it could say "paint view V's
content that lies in the document between the content for child views V1 and V2".]

Comment 11

[Kevin McCluskey (gone)]

More data:

Roc's div example: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=81850
-------------------
IE 6 behaves *exactly* the same is Mozilla. The text is placed on top of the
cyan box and below the orange box
Opera 6 puts the cyan and orange box on top.


Original test case: http://bugzilla.mozilla.org/attachment.cgi?id=79529&action=view
-------------------
IE 6. cyan and orange boxes are place on top of the text area 
Opera 6. cyan and orange boxes are place on *below* the text area 


When a <input type=text> is used instead of a text area

Mozilla: Behaves the same as in textarea example 
IE 6: Behaves the same as in the textarea example
Opera 6: input type=text element is always above the absolutely positioned elements

It appears that Opera 6 has issues related to using native form control elements
which prevents it from doing anything but placing the form control elements
above all other elements.


This is also an issue for fixed position elements. See bug
http://bugzilla.mozilla.org/show_bug.cgi?id=77607
Test case in http://bugzilla.mozilla.org/show_bug.cgi?id=77607
-------------------

Mozilla: The content of textareas, texfields, and password elements are rendered
over the top of the fixed position element while their border is rendered below.
Opera 6: All form controls: Textfields, textareas and passwords, radio buttons
are rendered above the fixed element. Other content is rendered below the fixed
position element.
IE 6: Does *not* support fixed positioned elements

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suggest punting this bug to post-1.0. Authors can work around it by explicitly
assigning z-indexes.

Comment 13

[Kevin McCluskey (gone)]

Attachment: patch which renders position:absolute, fixed, relative elements above non-positioned elements

Comment 14

[Kevin McCluskey (gone)]

Attachment: test case (1) absolutely positioned div should render above non-positioned div

Comment 15

[Kevin McCluskey (gone)]

Attachment: testcase (2) - Absolutely positioned div should appear above both the testfield background and content

Comment 16

[Kevin McCluskey (gone)]

Attachment: testcase (3) - Both textfield and div are absolutely positioned but div appears first in content order so it should be below the textfield

Comment 17

[Kevin McCluskey (gone)]

Attachment: testcase (4) - The div should be above the textfield because the div specifies z-index:0 and the textfield does not specify a z-index

Comment 18

[Kevin McCluskey (gone)]

Attachment: testcase (5) - The green div is above because it is relatively positioned and appears after the red div in content order

Comment 19

[Kevin McCluskey (gone)]

Attachment: testcase (6) - Relative positioned span is rendered on top of other content

Comment 20

[Kevin McCluskey (gone)]

Attachment: updated to the current trunk - patch which renders position:absolute, fixed, relative elements above non-positioned elements

With this patch installed all of the test cases render the same in Mozilla and
IE6.   It also renders pages the same as older Mozilla and N6.x versions except
for testcases which have scrolling views (textfields, textareas, scrolling
divs) which now render with both the background and content areas at a coherent
Z position.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe this should be quirks mode only... requesting Ian's input

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 86904 [details] [diff] [review]
updated to the current trunk - patch which renders position:absolute, fixed, relative elements above non-positioned elements

This is definitely the right approach.

I think that SetViewTopMost should be merged into SetZIndex: just add another
parameter to SetZIndex to represent the new low-order bit.

In fact, instead of keeping around this special bit, we could just expand
zindex values in the view system to be 64 bits. nsIView::SetZIndex and
nsIView::GetZIndex can translate into and out of the 64-bit representation.
This would require minimal changes and keep the code clean. Whaddaya think?

Comment 23

[Hixie (not reading bugmail)]

What exactly is the question I should be answering?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The CSS2 spec clearly states that absent z-index, content is rendered in
document order, with content earlier in the document rendered below content
appearing later in the document.

Consider this fragment:

<div>
  <p>Blah blah blah
  <div style="position: absolute; ...">Fee Fi Fo</div>
  <p>Foo Bar Baz
</div>

Here "Blah blah blah" should be under the positioned div, and "Foo Bar Baz"
should be on top of it. Yet all current browsers will put the positioned div on
top of all the other content. But we don't do it quite consistently so we're
considering a patch to make it "official" and consistent.

I strongly suspect that CSS2-correct ordering would wreck many legacy pages and
therefore I believe this approach is essential for quirks mode. The question is
whether we should apply this to strict mode also or whether we should press on
and do the CSS2-correct thing, even if it surprises almost everyone who uses
positioning.

Comment 25

[Hixie (not reading bugmail)]

I'll ask the WG

Comment 26

[Kevin McCluskey (gone)]

Attachment: solution which stores z-index's internally in PRInt64

Patch based on Roberts suggestions

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm afraid I may have lead you astray in suggesting this approach... The thing
is that when z-index is "auto", the topmost bit is ignored in the z-index
sorting code (because explicitZIndex is completely ignored).

The reason that your patch works for z-index: auto is only because InsertChild
always orders the children taking into account the topmost bit; elements with
the topmost bit set get moved to the front of the child view list (i.e. the
topmost views). In fact that's all we really need to do! Just make sure that
InsertChild always puts topmost views at the front of the child list. We
shouldn't need to use the topmost bit anywhere else. So probably it would be
easier to go back to your original approach and store the topmost bit separately.

I'll give it some thought. Let me chew on it for a day or two in case I change
my mind again :-).

Comment 28

[Kevin McCluskey (gone)]

Attachment: re-order the child list only.

Comment 29

[Kevin McCluskey (gone)]

Attachment: re-order the child list only

Comment 30

[Kevin McCluskey (gone)]

Attachment: Reorder child list using topmost flag when view is z-index:auto

Comment 31

[Kevin McCluskey (gone)]

patch (attachment 88674 [details] [diff] [review]) is ready for reviews.

Comment 32

[Kevin McCluskey (gone)]

*** Bug 118474 has been marked as a duplicate of this bug. ***

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the implementation of CompareZIndex is wrong. If the first z-index is
"auto" and the other one isn't, you'll return "greater than" no matter what.
E.g., if the first z-index is auto and the other one is, say, 2, then the second
one is greater.

I suggest just replacing it with this:

if (aZIndex1 != aZIndex2) {
  return aZIndex1 - aZIndex2;
} else {
  return aTopMost1 - aTopMost2;
}

This relies on the facts that
-- whenever isAuto is true, the corresponding ZIndex is 0
-- whenever isAuto is false, the corresponding aTopMost is false (0)
You probably want to assert these at the top:
NS_ASSERTION(aIsAuto1 ? aZIndex1 == 0 : !aTopMost1);
NS_ASSERTION(aIsAuto2 ? aZIndex2 == 0 : !aTopMost2);

The rest looks fine.

Comment 34

[Kevin McCluskey (gone)]

The ASSERTIONS will fire because the folowing rule is *not* true

-- whenever isAuto is false, the corresponding aTopMost is false (0)

aTopMost is false only if the view was not created for the purpose
of absolutely, relative, or fixed positioning. All positioned
elements will set their view's aTopMost flag to true regardless of whether 
they are z-index:auto or z-index:<explicit value>

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well, I lied. The code I suggested doesn't actually rely on "whenever isAuto is
false, the corresponding aTopMost is false". So just add the assertions

NS_ASSERTION(!aIsAuto1 || aZIndex1 == 0);
NS_ASSERTION(!aIsAuto2 || aZIndex2 == 0);

and go ahead and use it.

Comment 36

[Kevin McCluskey (gone)]

Attachment: patch incorporating Robert's feedback

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 89796 [details] [diff] [review]
patch incorporating Robert's feedback

You need to fix the comment to say

// > 0 if first z-index is greater than the second
// < 0 if first z-index is less than the second

With that, r=roc+moz

Comment 38

[kinmoz]

Comment on attachment 89796 [details] [diff] [review]
patch incorporating Robert's feedback

sr=[e-mail]

Comment 39

[Kevin McCluskey (gone)]

Checked in fix to the trunk (attachment 89796 [details] [diff] [review])

Comment 40

[Kevin McCluskey (gone)]

petersen: Chris can you verify that this bug has been fixed on the trunk? Thanks

Comment 41

[Jaime Rodriguez, Jr.]

petersen: the window is closing on us. can you pls verify this fix on the trunk
today? thanks!

Comment 42

[Sivakiran Tummala]

Verified on 07-11-08-trunk, all the attached testcases work as expected.

Comment 43

[Jaime Rodriguez, Jr.]

i believe we are in good shape with dig bugs, and appears the author of the page
has a viable workaround for this issue. let's get it in the next release.