Last Comment Bug 696739 - Cannot change options on Travelocity site using mouse
: Cannot change options on Travelocity site using mouse
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 10 Branch
: x86 Windows 7
: -- normal with 1 vote (vote)
: mozilla10
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
:
Mentors:
Depends on:
Blocks: 10209
  Show dependency treegraph
 
Reported: 2011-10-24 05:26 PDT by Peter Pennington
Modified: 2012-01-05 13:28 PST (History)
7 users (show)
bernd_mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
sample html (1.35 KB, text/html)
2011-10-24 07:03 PDT, Alice0775 White
no flags Details
another sample html (1.69 KB, text/html)
2011-10-24 07:53 PDT, Alice0775 White
no flags Details
Minimal testcase (251 bytes, text/html)
2011-10-24 10:44 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Testcase showing broken painting (533 bytes, text/html)
2011-10-24 11:03 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
fix (2.95 KB, patch)
2011-10-24 20:35 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
bzbarsky: review+
Details | Diff | Splinter Review

Description Peter Pennington 2011-10-24 05:26:36 PDT
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 2011-10-24 07:03:26 PDT
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 2011-10-24 07:03:52 PDT
Created attachment 569056 [details]
sample html
Comment 3 Alice0775 White 2011-10-24 07:53:02 PDT
Created attachment 569067 [details]
another sample html
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-10-24 10:44:53 PDT
Created attachment 569107 [details]
Minimal testcase
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-10-24 10:48:47 PDT
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 [:bz] (still a bit busy) 2011-10-24 10:55:58 PDT
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 [:bz] (still a bit busy) 2011-10-24 11:02:32 PDT
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 [:bz] (still a bit busy) 2011-10-24 11:03:09 PDT
Created attachment 569111 [details]
Testcase showing broken painting
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-10-24 11:12:57 PDT
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 [:bz] (still a bit busy) 2011-10-24 11:27:58 PDT
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 [:bz] (still a bit busy) 2011-10-24 11:32:31 PDT
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 [:bz] (still a bit busy) 2011-10-24 11:40:00 PDT
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) 2011-10-24 19:24:31 PDT
(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 [:bz] (still a bit busy) 2011-10-24 19:51:11 PDT
Hmm.  I think that would make sense, yes.
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-10-24 20:35:50 PDT
Created attachment 569274 [details] [diff] [review]
fix
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-10-24 20:40:08 PDT
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) 2011-10-24 20:54:05 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/b87afa49527d
Comment 18 Marco Bonardo [::mak] 2011-10-25 05:04:59 PDT
https://hg.mozilla.org/mozilla-central/rev/b87afa49527d

Note You need to log in before you can comment on or make changes to this bug.