312586 - Multiple sheets (JS alert boxes) on one window don't work

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Mark Mentovai]

ddd dd 


dd

Comment 1

[Mark Mentovai]

Attachment: Testcase

That was good.	Thanks, Bugzilla.

If a window tries to open a sheet while another one is already exposed, via JS
alert() for example, the new sheet is never displayed.	It appears that its
window object is created, though.  The new sheet should either replace the
existing sheet or be saved up and displayed when the existing sheet disappears.
 My preference is for the latter.

In the testcase, if you hover over the brown square, you'll get a bunch of
alert() attempts, each one is mirrored the addition of some HTML.  There really
should only be one mouseover event firing, this seems to be an artifact of
opening a new window and is a separate bug.  Only the first alert is shown, and
the alert sheet's OK button doesn't have focus so it can't be dismissed by
pressing return.  Focus is probably being stolen by subsequent attempts to open
sheets.

In some rare cases, the sheet that does open doesn't display the fox logo, has
both Cancel and OK buttons and is improperly sized, or both.

The green square is similar, except it shoots a pair of alerts at 100ms and
200ms delays after receivng the mouseover.

The phantom undisplayed sheets are killing scripting in some contexts, which
presumably become blocked waiting for all of those "OK" buttons to be clicked. 
After a round of mouseovers, it may be difficult to get more to fire (at least
on the brown square), and some of the browser chrome (toolbar and bookmarks
toolbar) becomes non-functional and won't respond to clicks.  In some cases,
the command-Q (quit) shortcut also becomes a no-op.

Also, sheets are supposed to be window-modal, but while a sheet is open,
mouseover events are still firing.  Compare to this testcase running on
Windows, which still fires too many events, but displays a modal dialog for
each, and refuses to fire additional events while the dialog is out.  (Windows
also has a cosmetically disgusting focus flicker on the testcase.)

This bug can make certain web applications impossible to use, and interferes
with using Firefox on the Mac as a web development platform, where alert() is
frequently used as a debugging aid.

Comment 2

[Simon Fraser [no longer active]]

See bug 300057.

Comment 3

[Mark Mentovai]

Attachment: Support multiple sheets per window

This works very well.  The existing implementation supports sheets as children
of sheets, an annoying feature used all over the 1.0 prefs UI, but broke down
when sheets were siblings of one another.  This patch introduces support for
sibling  sheets, which the testcases here and in bug 300057 indicate can be
found in the real world.  The heuristics are simple: if a sheet wants to open
but a sibling sheet is already open, the sheet doesn't attach to the window. 
When any sheet hides, it checks for siblings that wanted to display and shows
the first one (in the order they live in the widget tree).  If there are no
siblings waiting, the parent sheet, if any, is restored.

This fixes the testcase here and all of the testcases in bug 300057.  If the
event filtering discussed over there is ever implemented, and this change would
still be necessary to fix all of the testcases.  The event filtering is still
needed, but it apparently ain't gonna happen for 1.8.

I think this is a low-risk approach to avoid browser lockup.

Comment 4

[Mark Mentovai]

Comment on attachment 199708 [details] [diff] [review]
Support multiple sheets per window

+    if (mIsSheet && mShown) {

If !bState && mIsSheet && !mShown, this needs to set mSheetNeedsShow = PR_FALSE
so that sheets that never had an opportunity to show won't spring to life after
they're supposed to be hidden.	It's a rare case considering the modality.

Comment 5

[Mark Mentovai]

Attachment: v2, support multiple sheets per window

Implements the case I just caught, and gets rid of some of the static cast
ugliness by inheriting GetChildSheet from nsPIWidgetMac.

Comment 6

[Scott MacGregor]

Comment on attachment 199735 [details] [diff] [review]
v2, support multiple sheets per window

how did 1.0 handle this case? 

Changes to our core mac window code with days to go until 1.5 is
finished....going to be a tough sell this late in the game.

Comment 7

[Mark Mentovai]

1.0 didn't handle this case.  The browser locks up when you try to open more
than one sheet at a time, often requiring a forced quit-restart.  This renders
script debugging with alert() nearly impossible.  It's the same issue reported
in bug 300057.  The solution here is less invasive than what was proposed there,
solves all of those testcases, and only alters the browser's behavior in cases
where it would have hung.

Comment 8

[Asa Dotzler [:asa]]

not a regression from 1.0, we're five days from lockdown, and this isn't an
obviously safe fix so we're not gonna take it for 1.5.

Comment 9

[Mark Mentovai]

Comment on attachment 199735 [details] [diff] [review]
v2, support multiple sheets per window

Note, the iid in nsPIWidgetMac needs to be bumped.

Comment 10

[Mark Mentovai]

smfr suggests making GetChildSheet return an interface and addref it.

Comment 11

[Mark Mentovai]

Attachment: As checked in

The checked-in version differs from the reviewed version only in that it has a new uuid for nsPIWidgetMac.  I didn't change GetChildSheet, because Get*Child and Get*Sibling don't addref either.

Comment 12

[Mark Mentovai]

Fixed on trunk.

Comment 13

[Mark Mentovai]

Comment on attachment 207741 [details] [diff] [review]
As checked in 

Setting a1.8.1? as a reminder that I want this for 1.8.1 when it reopens.

Comment 14

[Patrick]

(In reply to comment #12)
> Fixed on trunk.
> 

Thanks Mark for pointing me to this bug. This one is listed as fixed on trunk as of January 20056. However, mentioned in bug 337334 comment 17, i'm seeing this again in the latest nightly

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060524
Minefield/3.0a1.

Comment 15

[Mark Mentovai]

Patrick, I suggest finding a regression range and filing a new bug, since it's either a recent re-regression or is subtly different from this bug and wasn't actually fixed by this patch.

Comment 16

[Mark Mentovai]

New bug is bug 339359 duped to bug 337646.

Comment 17

[Mark Mentovai]

Comment on attachment 207741 [details] [diff] [review]
As checked in 

Checked in on MOZILLA_1_8_BRANCH along with bug 337646 (consolidated patch including the branch version of this patch and bug 337646 is on that bug).