Closed Bug 696739 Opened 13 years ago Closed 13 years ago

Cannot change options on Travelocity site using mouse


(Core :: Layout, defect)

10 Branch
Windows 7
Not set



Tracking Status
firefox10 - ---


(Reporter: kernow01, Assigned: roc)



(Keywords: regression)


(5 files)

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

Steps to reproduce:

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
Regression window(m-c),
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20110929 Firefox/10.0a1 ID:20110929122038
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20110929 Firefox/10.0a1 ID:20110929141938

Triggered by:
Bug 10209 - problems with absolute or fixed position on table [ABS POS] [FIX POS]
Blocks: 10209
Ever confirmed: true
Keywords: regression
Attached file sample html
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Attached file another sample html
The frame tree looks correct.

I would guess that this is actually a regression from in that the fieldset frame used to be a containing block no matter what, but now whether it is depends on the display value...
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)
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 somewhere above step 5, instead of around step 2.
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.
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?
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.
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....
(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) &&
Hmm.  I think that would make sense, yes.
Attached patch fixSplinter Review
Assignee: nobody → roc
Attachment #569274 - Flags: review?(bzbarsky)
Comment on attachment 569274 [details] [diff] [review]


Can that XXX comment go away?
Attachment #569274 - Flags: review?(bzbarsky) → review+
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.