257546 - Sheet support implemented incorrectly

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[jhp (no longer active)]

Sheet dialog support is implemented incorrectly for MacOSX.  You can see this
best by opening Tools->Extensions in Firefox, and clicking on the 'about' button
for one of the extensions.  This will open a floating window in the top left
corner with no titlebar, no where near the Extensions window.  Same thing
happens for the extensions options/preferences dialog.

The reason for this is that sheet support is implemented as a bit in the
nsBorderStyle member of the nsWidgetData structure.  However, the
eBorderStyle_default enum is defined as '-1', which becomes '0xffff' (all bits
set), meaning we can't tell whether the eBorderStyle_sheet bit has been set at all.

Comment 1

[jhp (no longer active)]

Attachment: patch (diff -w)

The issue is that sheet support was implemented in the wrong place.  It is not
a border style;  sheets are actually a special type of dialog.	So in this
patch I've defined a new nsWindowType to handle sheets.

Comment 2

[Jo Hermans]

Is this the reason that I see almost all pulldown windows appearing in the
left-hand corner, both in todays Seamonkey and Firefox builds (2004083108, on
Mac OS X 10.2.3) ? I haven't seen this in yesterdays builds (2004083008).
Erasing the XUL.mfasl file didn't help.

I see it with :
- location pulldown
- search engine pulldown (in FF)
- right click pulldown on toolbar (icon-pulldowns are ok)
- all right-click popups in the browser area
- most title-popups

Once in a while I see pulldowns that are in the correct place, or that jump back
to the correct place (esp when closing the pulldown again), but that's not
repeatable. Most pulldowns that are attached to buttons (in the mail-interface
f.e.) work ok, except for the location pulldown and the search engine pulldown. 

A major regression caused by bug 223545 (which I love btw) ?

Comment 3

[jhp (no longer active)]

Jo, please post a comment in that bug, and make sure to mention your OS version.
 This bug should be unrelated to what you are seeing.

Comment 4

[Asaf Romano (gone)]

(In reply to comment #2)
> Is this the reason that I see almost all pulldown windows appearing in the
> left-hand corner, both in todays Seamonkey and Firefox builds (2004083108, on
> Mac OS X 10.2.3) ? I haven't seen this in yesterdays builds (2004083008).
> Erasing the XUL.mfasl file didn't help.

This is fixed now (bug bug 257647)

Comment 5

[Asaf Romano (gone)]

*** Bug 259343 has been marked as a duplicate of this bug. ***

Comment 6

[Asaf Romano (gone)]

Attachment: proposed addition (diff -u)

Our sheet impl' is really problematic, your patch does impore it, but the main
problem is still there: no way to create modal-but-non-sheet windows.

This is a very bad user-exp' when it comes to modal-opened-by-modal behavior,
that's one reason that not every document-modal on OS X is a sheet.

So what this one changes/adds:
 1) "modal, centerscreen" is a normal document-modal window, not a sheet
   --This adds an option for window.open() callers to avoid sheet behavior.
     Cases that are fixed without anymore changes are wizards and
     firefox/tb about box.
 2) Disables autosheet of dialogs (that's because not every dialog is
per-current-document)
 3) prompts are just "chrome, modal" on OS X (aka sheets)
 4) cleanup of nsAppShellService::CalculateWindowZLevel

Comment 7

[Asaf Romano (gone)]

Attachment: diff -w

Comment 8

[jhp (no longer active)]

mano: Do you have any specific examples of dialogs that your additions change? 
Like a dialog that is now (or no longer) a sheet?

Comment 9

[Asaf Romano (gone)]

(There ar no new sheets.)

real life examples for no-sheets (after patching):
About Dialog
Import Wizard
Profile Migrator
Some windows in the address book

There are some cases that we may want to "resheet" like the download-properties
dialog, but lets get this one in, so we will be able to contorl what is "sheeted".

btw, 1.0mac was dropped, nominating for 1.0.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 163009 [details] [diff] [review]
proposed addition (diff -u)

we're really short of time for 1.0 (as mentioned no separate 1.0mac anymore).

Comment 11

[Simon Fraser [no longer active]]

this is pretty risky at this stage in the game. how much testing has it had?

Comment 12

[Asaf Romano (gone)]

(In reply to comment #11)
> this is pretty risky at this stage in the game. how much testing has it had?

Well, pedemonte's patch is applied on my build for some time and from what i 
can say, it hasn't regressed anything.

I haven't played enough with the additions from comment 9, but i am not sure 
they're that risky, we don't have many modal,centerscreen windows in Firefox 
(We do have in tbird, but there is much more time until the next release).

After this one is landed we're going to "desheet" some windows, especially 
those that has sub-modals (Firefox's prefs window for example and some wizards).

Anyway, the orginal issue (which is fixed by the patch) is very visible in FF; 
extensions' options panes and some pref windows (e.g. js options) are opened as 
modal in screen's top left corner.

Comment 13

[jhp (no longer active)]

Simon, I don't think the first patch is that risky at all.  It only makes it so
the sheet flag isn't obscured.  As Asaf says, this specifically fixes the issues
in the Firefox Extensions dialog, when selecting Preferences or About.

I'm not sure about Asaf's additions.  I think that one might be too risky; we
might want to wait until after 1.0.  I'll have to talk to him about it.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 157502 [details] [diff] [review]
patch (diff -w)

Looks ok. I do think we should go for a distinction between application-level
dialogs and window-level dialogs in the future (and use that concept in the
enum's names and let other platforms treat them as equal). I don't think
CHROME_OPENAS_DIALOG | CHROME_MODAL is completely correct in that sense. But
this is fine for now I think.

Comment 15

[jhp (no longer active)]

peterv, I agree completely.  That's on my plate, just haven't gotten around to
it yet.

Comment 16

[Asaf Romano (gone)]

Comment on attachment 163009 [details] [diff] [review]
proposed addition (diff -u)

obselting this one, for now.

Comment 17

[Kevin Gerich]

I applied the patch to a fresh Aviary build and it looked to me like
application-modal dialogs are being used instead of sheets. This works for
Wizards, Preferences and the About dialog, but I would hope that if the patch is
considered for 1.0 that it include the use of sheets where appropriate.

Should be sheets (Firefox):

Alert/Auth Dialog (Common Dialog)
Cert Warning Dialog
Add Bookmark dialog
Software Install Allowed Sites (triggered from page info bar)
Software Install
Popup Blocker Allowed Sites (triggered from page info bar)


Should not be sheets:
Preferences
About
Wizards

Comment 18

[Asaf Romano (gone)]

(In reply to comment #17)

what patch did you applied? the first one (which is the nominated one) doesn't 
change the behavior you mentioned (while the last one does).

Comment 19

[Kevin Gerich]

I applied the first patch (sheet.diff) ... perhaps I applied it incorrectly
(patch -p0 < sheet.diff) but I didn't get errors.

Comment 20

[Asaf Romano (gone)]

(In reply to comment #19)

Well, everyting is still "sheeted", here.

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

We can't take this kind of thing now. We're shipping on Tuesday. 

Comment 22

[Asaf Romano (gone)]

Comment on attachment 157502 [details] [diff] [review]
patch (diff -w)

sigh

Comment 23

[jhp (no longer active)]

Checked in first patch to trunk.  I'm going to mark this as fixed, since the
original problem this bug was opened for has been fixed.  I'll open another bug
to handle Asaf's concerns.