Closed Bug 312586 Opened 16 years ago Closed 15 years ago

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

Categories

(Core Graveyard :: Widget: Mac, defect)

1.8 Branch
PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mark, Assigned: mark)

References

Details

(Keywords: fixed1.8.1)

Attachments

(3 files, 1 obsolete file)

ddd dd 


dd
Attached file 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.
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.
Attachment #199708 - Flags: superreview?(sfraser_bugs)
Attachment #199708 - Flags: review?(bugs.mano)
Flags: blocking1.8rc1?
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.
Attachment #199708 - Flags: superreview?(sfraser_bugs)
Attachment #199708 - Flags: review?(bugs.mano)
Implements the case I just caught, and gets rid of some of the static cast
ugliness by inheriting GetChildSheet from nsPIWidgetMac.
Attachment #199708 - Attachment is obsolete: true
Attachment #199735 - Flags: superreview?(sfraser_bugs)
Attachment #199735 - Flags: review?(bugs.mano)
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.
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.
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.
Flags: blocking1.8rc1? → blocking1.8rc1-
Attachment #199735 - Flags: review?(bugs.mano) → review?(joshmoz)
Comment on attachment 199735 [details] [diff] [review]
v2, support multiple sheets per window

Note, the iid in nsPIWidgetMac needs to be bumped.
Attachment #199735 - Flags: review?(joshmoz) → review+
Attachment #199735 - Flags: superreview?(sfraser_bugs) → superreview+
smfr suggests making GetChildSheet return an interface and addref it.
Attached patch As checked in Splinter Review
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.
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
Attachment #207741 - Flags: approval-branch-1.8.1?(mark)
(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.
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.
Blocks: 337646
New bug is bug 339359 duped to bug 337646.
No longer blocks: 337646
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).
Attachment #207741 - Flags: approval-branch-1.8.1?(mark) → approval-branch-1.8.1+
Keywords: fixed1.8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.