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

VERIFIED FIXED in mozilla1.0

Status

()

Core
Layout: View Rendering
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Robert Kieffer, Assigned: Kevin McCluskey (gone))

Tracking

({topembed+})

Trunk
mozilla1.0
x86
Windows 2000
topembed+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: DIGBug WG [adt2] [fixed on trunk])

Attachments

(9 attachments, 7 obsolete attachments)

1012 bytes, text/html
Details
995 bytes, text/html
Details
403 bytes, text/html
Details
364 bytes, text/html
Details
402 bytes, text/html
Details
445 bytes, text/html
Details
476 bytes, text/html
Details
381 bytes, text/html
Details
16.06 KB, patch
roc
: review+
kinmoz
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
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)
(Reporter)

Comment 1

16 years ago
Created attachment 79529 [details]
Test of mixed absolute/relative element event handling
(Reporter)

Updated

16 years ago
Whiteboard: DIGBug

Updated

16 years ago
Target Milestone: --- → mozilla1.0

Comment 2

16 years ago
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.
Keywords: nsbeta1

Updated

16 years ago
Keywords: nsbeta1 → nsbeta1+, topembed+

Comment 3

16 years ago
David, do you have any thoughts on this one?
Summary: Event targeting is wrong with absolute/mixed position element styles → [EVENTTARG] Event targeting is wrong with absolute/mixed position element styles
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.
(Assignee)

Comment 5

16 years ago
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.
Assignee: joki → kmcclusk
(Assignee)

Comment 6

16 years ago
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.
(Assignee)

Comment 7

16 years ago
Created attachment 81772 [details] [diff] [review]
Always create views for the GfxScrollFrame and GfxTextControlFrames
(Assignee)

Comment 8

16 years ago
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)
Created attachment 81850 [details]
Testcase showing that this problem can be reproduced using only DIVs
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".]
(Assignee)

Comment 11

16 years ago
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
I suggest punting this bug to post-1.0. Authors can work around it by explicitly
assigning z-indexes.
(Assignee)

Comment 13

16 years ago
Created attachment 86858 [details] [diff] [review]
patch which renders position:absolute, fixed, relative elements above non-positioned elements
(Assignee)

Updated

16 years ago
Attachment #81772 - Attachment is obsolete: true
(Assignee)

Comment 14

16 years ago
Created attachment 86859 [details]
test case (1) absolutely positioned div should render above non-positioned div
(Assignee)

Comment 15

16 years ago
Created attachment 86860 [details]
testcase (2) - Absolutely positioned div should appear above both the testfield background and content
(Assignee)

Comment 16

16 years ago
Created attachment 86864 [details]
testcase (3) - Both textfield and div are absolutely positioned but div appears first in content order so it should be below the textfield
(Assignee)

Comment 17

16 years ago
Created attachment 86865 [details]
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
(Assignee)

Comment 18

16 years ago
Created attachment 86866 [details]
testcase (5) - The green div is above because it is relatively positioned and appears after the red div in content order
(Assignee)

Comment 19

16 years ago
Created attachment 86867 [details]
testcase (6) - Relative positioned span is rendered on top of other content
(Assignee)

Comment 20

16 years ago
Created attachment 86904 [details] [diff] [review]
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.
Attachment #86858 - Attachment is obsolete: true
Maybe this should be quirks mode only... requesting Ian's input
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?
What exactly is the question I should be answering?
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.
I'll ask the WG
Whiteboard: DIGBug → DIGBug WG
(Assignee)

Updated

16 years ago
Whiteboard: DIGBug WG → DIGBug WG [adt2]
(Assignee)

Comment 26

16 years ago
Created attachment 87276 [details] [diff] [review]
solution which stores z-index's internally in PRInt64 

Patch based on Roberts suggestions
Attachment #86904 - Attachment is obsolete: true
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 :-).
(Assignee)

Comment 28

16 years ago
Created attachment 87381 [details] [diff] [review]
re-order the child list only.
(Assignee)

Comment 29

16 years ago
Created attachment 87427 [details] [diff] [review]
re-order the child list only
Attachment #87381 - Attachment is obsolete: true
(Assignee)

Comment 30

16 years ago
Created attachment 88674 [details] [diff] [review]
Reorder child list using topmost flag when view is z-index:auto
Attachment #87276 - Attachment is obsolete: true
Attachment #87427 - Attachment is obsolete: true
(Assignee)

Comment 31

16 years ago
patch (attachment 88674 [details] [diff] [review]) is ready for reviews.
Whiteboard: DIGBug WG [adt2] → DIGBug WG [adt2] waiting for r/sr
(Assignee)

Comment 32

16 years ago
*** Bug 118474 has been marked as a duplicate of this bug. ***
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.
(Assignee)

Comment 34

16 years ago
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>
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.
(Assignee)

Comment 36

16 years ago
Created attachment 89796 [details] [diff] [review]
patch incorporating Robert's feedback
Attachment #88674 - Attachment is obsolete: true
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
Attachment #89796 - Flags: review+

Comment 38

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

sr=kin@netscape.com
Attachment #89796 - Flags: superreview+
(Assignee)

Comment 39

16 years ago
Checked in fix to the trunk (attachment 89796 [details] [diff] [review])
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
Keywords: adt1.0.1
(Assignee)

Updated

16 years ago
Keywords: approval, mozilla1.0.1
Whiteboard: DIGBug WG [adt2] waiting for r/sr → DIGBug WG [adt2] [fixed on trunk]
(Assignee)

Updated

16 years ago
Component: DOM Events → Views
(Assignee)

Comment 40

16 years ago
petersen: Chris can you verify that this bug has been fixed on the trunk? Thanks
QA Contact: vladimire → petersen

Updated

16 years ago
Blocks: 143047
Whiteboard: DIGBug WG [adt2] [fixed on trunk] → DIGBug WG [adt2] [fixed on trunk] [ETA 07/10]

Comment 41

16 years ago
petersen: the window is closing on us. can you pls verify this fix on the trunk
today? thanks!
Whiteboard: DIGBug WG [adt2] [fixed on trunk] [ETA 07/10] → DIGBug WG [adt2] [fixed on trunk] [ETA 07/13]

Comment 42

16 years ago
Verified on 07-11-08-trunk, all the attached testcases work as expected.
Status: RESOLVED → VERIFIED

Comment 43

16 years ago
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.
No longer blocks: 143047
Keywords: adt1.0.1 → adt1.0.1-
Whiteboard: DIGBug WG [adt2] [fixed on trunk] [ETA 07/13] → DIGBug WG [adt2] [fixed on trunk]
You need to log in before you can comment on or make changes to this bug.