696739 - Cannot change options on Travelocity site using mouse

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Peter Pennington]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111023 Firefox/10.0a1
Build ID: 20111023031047

Steps to reproduce:

http://www.travelocity.com/

Cannot change option from, say, 'Hotel only' to 'Flight only' using mouse. Keyboard works


Actual results:

Option marker didn't move


Expected results:

Should have been able to change option

Comment 1

[Alice0775 White]

Regression window(m-c),
Works:
http://hg.mozilla.org/mozilla-central/rev/60e86b847759
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20110929 Firefox/10.0a1 ID:20110929122038
Fails:
http://hg.mozilla.org/mozilla-central/rev/af3668a89015
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20110929 Firefox/10.0a1 ID:20110929141938
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=60e86b847759&tochange=af3668a89015

Triggered by:
Bug 10209 - problems with absolute or fixed position on table [ABS POS] [FIX POS]

Comment 2

[Alice0775 White]

Attachment: sample html

Comment 3

[Alice0775 White]

Attachment: another sample html

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

The frame tree looks correct.

I would guess that this is actually a regression from http://hg.mozilla.org/mozilla-central/rev/00f422b2cf36 in that the fieldset frame used to be a containing block no matter what, but now whether it is depends on the display value...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  On the other hand, fieldset is not a line participant and neither it nor the block inside it is IsBlockWrapper.

When dumping out the event handling display list for the minimal testcase, it looks like this:

Event handling --- (6240,1560):
Background 0x7400320(HTMLScroll(html)(-1)) (0,0,36600,22920)(0,0,0,0) uniform
Clip 0x0() (0,0,36600,22920)(0,0,0,0)
    CanvasBackground 0x7400148(Canvas(html)(-1)) (0,0,36600,22920)(0,0,0,0) uniform
    Background 0x71342f8(Block(html)(-1)) (0,0,36600,3408)(0,0,0,0) uniform
Clip 0x0() (0,0,36600,22920)(0,0,0,0)
    Background 0x7134838(Block(body)(2)) (480,480,35640,2448)(0,0,0,0) uniform
    Background 0x71348c8(Block(form)(1)) (480,480,35640,2448)(0,0,0,0) uniform
Clip 0x72b0980(Block(div)(1)) (0,0,36600,22920)(0,0,0,0)
    WrapList 0x72b0980(Block(div)(1)) (1260,936,7952,1152)(0,0,0,0)
        Background 0x72b0980(Block(div)(1)) (1320,936,7832,1152)(0,0,0,0) uniform
        Text 0x72b0a78(Text(0)) (1260,1032,7952,960)(0,0,0,0)
Clip 0x0() (0,0,36600,22920)(0,0,0,0)
    FieldSetBorderBackground 0x7134b08(FieldSet(fieldset)(1)) (600,480,9272,2448)(0,0,0,0)
    Background 0x72b0710(Block(fieldset)(1)) (1320,936,7832,1152)(0,0,0,0) uniform

If I make the div not be float:left, I instead get this:

Event handling --- (7320,1380):
Background 0x71cc730(HTMLScroll(html)(-1)) (0,0,36600,22920)(0,0,0,0) uniform
Clip 0x0() (0,0,36600,22920)(0,0,0,0)
    CanvasBackground 0x71cc558(Canvas(html)(-1)) (0,0,36600,22920)(0,0,0,0) uniform
    Background 0x71877b8(Block(html)(-1)) (0,0,36600,3408)(0,0,0,0) uniform
Clip 0x0() (0,0,36600,22920)(0,0,0,0)
    Background 0x7187cf8(Block(body)(2)) (480,480,35640,2448)(0,0,0,0) uniform
    Background 0x7187d88(Block(form)(1)) (480,480,35640,2448)(0,0,0,0) uniform
    Background 0x7188598(Block(div)(1)) (1320,936,7832,1152)(0,0,0,0) uniform
Clip 0x0() (0,0,36600,22920)(0,0,0,0)
    FieldSetBorderBackground 0x7187fc8(FieldSet(fieldset)(1)) (600,480,9272,2448)(0,0,0,0)
    Background 0x7188328(Block(fieldset)(1)) (1320,936,7832,1152)(0,0,0,0) uniform
    Text 0x7188650(Text(0)) (1260,1032,7952,960)(0,0,0,0)

Comment 7

[Boris Zbarsky [:bzbarsky]]

This affects painting too; testcase coming up.  The real issue is that the fieldset background is ending up on top of the float completely, and on top of descendant block backgrounds in general.  So per http://www.w3.org/TR/CSS21/zindex.html somewhere above step 5, instead of around step 2.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing broken painting

Comment 9

[Boris Zbarsky [:bzbarsky]]

For comparison, on aurora for the float:left case from comment 6 the part above the body and form backgrounds looks like this:

Clip 0x0() (0,0,77760,52800)(0,0,0,0)
    FieldSetBorderBackground 0x100a19f28(FieldSet(fieldset)(1)) (600,480,18708,2448)(0,0,0,0)
    Background 0x100f04a60(Block(fieldset)(1)) (1320,936,17268,1152)(0,0,0,0) uniform
    WrapList 0x100efe3b0(Block(div)(1)) (1260,936,17388,1152)(0,0,0,0)
        Background 0x100efe3b0(Block(div)(1)) (1320,936,17268,1152)(0,0,0,0) opaque uniform
        Text 0x100efe560(Text(0)) (1260,1032,17388,960)(0,0,0,0)

so the WrapList is above the fieldset border and background, as it should be....

nsFieldSetFrame::BuildDisplayList does some weird display list moving, but bug 10209 didn't change any of that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Aha!  The containing block change did in fact change some relevant code.  In nsIFrame::BuildDisplayListForChild the diff shows:

   // XXX we REALLY need a "are you an inline-block sort of thing?" here!!!
   if ((aFlags & DISPLAY_CHILD_INLINE) &&
       (disp->mDisplay != NS_STYLE_DISPLAY_INLINE ||
-       aChild->IsContainingBlock() ||
        (aChild->IsFrameOfType(eReplaced)))) {
     // child is a non-inline frame in an inline context, i.e.,
     // it acts like inline-block or inline-table. Therefore it is a
     // pseudo-stacking-context.
     pseudoStackingContext = PR_TRUE;
   }

and nothing replaced that removal.  My fault for not catching that...  In any case, we're hitting that exact situation, because nsBlockFrame passes DISPLAY_CHILD_INLINE when building the display list on an inline line.  And fieldset is of of type eReplaced.... and I'm not really sure it should be.

Seems like we need to address that XXX comment and that will fix things.  roc, tn, do you have a preferred API for that?

Comment 11

[Boris Zbarsky [:bzbarsky]]

And yes, checking for aChild->GetType() == nsGkAtoms::fieldSetFrame in there fixes the testcases in this bug, both painting and event delivery.

In comment 10 I meant to say that fieldset is NOT of type eReplaced, of course.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Looking through the IsContainingBlock change, the other frames that unconditionally returned true (or more to the point could return true while being display:inline) were button controls (which are eReplaced), list controls (also eReplaced), blockframes (but those should not usually be happening for display:inline), page content frames, page frames, viewports, table cells, table colgroups, tables, outer tables, table rowgroups (can't be display:inline), and scrollbars (does this ever matter for them?).

So the only observable breakage should be fieldsets....

Comment 13

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

(In reply to Boris Zbarsky (:bz) from comment #10)
>    if ((aFlags & DISPLAY_CHILD_INLINE) &&
>        (disp->mDisplay != NS_STYLE_DISPLAY_INLINE ||
> -       aChild->IsContainingBlock() ||
>         (aChild->IsFrameOfType(eReplaced)))) {
>      // child is a non-inline frame in an inline context, i.e.,
>      // it acts like inline-block or inline-table. Therefore it is a
>      // pseudo-stacking-context.
>      pseudoStackingContext = PR_TRUE;
>    }

What if we changed that to
  if ((aFlags & DISPLAY_CHILD_INLINE) &&
      !aChild->IsFrameOfType(eLineParticipant))
?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm.  I think that would make sense, yes.

Comment 15

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

Attachment: fix

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 569274 [details] [diff] [review]
fix

r=me

Can that XXX comment go away?

Comment 17

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/b87afa49527d

Comment 18

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/b87afa49527d