Cannot change options on Travelocity site using mouse

RESOLVED FIXED in mozilla10

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Peter Pennington, Assigned: roc)

Tracking

({regression})

10 Branch
mozilla10
x86
Windows 7
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox10-)

Details

Attachments

(5 attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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]
Blocks: 10209
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression

Comment 2

6 years ago
Created attachment 569056 [details]
sample html

Updated

6 years ago
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout

Comment 3

6 years ago
Created attachment 569067 [details]
another sample html
tracking-firefox10: --- → ?
Created attachment 569107 [details]
Minimal testcase
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...
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 http://www.w3.org/TR/CSS21/zindex.html somewhere above step 5, instead of around step 2.
Created attachment 569111 [details]
Testcase showing broken painting
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) &&
      !aChild->IsFrameOfType(eLineParticipant))
?
Hmm.  I think that would make sense, yes.
Created attachment 569274 [details] [diff] [review]
fix
Assignee: nobody → roc
Attachment #569274 - Flags: review?(bzbarsky)
Comment on attachment 569274 [details] [diff] [review]
fix

r=me

Can that XXX comment go away?
Attachment #569274 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/b87afa49527d
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/b87afa49527d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10

Updated

6 years ago
Flags: in-testsuite+

Updated

6 years ago
tracking-firefox10: ? → -
You need to log in before you can comment on or make changes to this bug.