Sheet support implemented incorrectly

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: jhp (no longer active), Assigned: jhp (no longer active))

Tracking

Trunk
PowerPC
Mac OS X
Points:
---
Bug Flags:
blocking-aviary1.0 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 157502 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Assignee: jag → jhpedemonte
Status: NEW → ASSIGNED

Comment 2

13 years ago
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) ?
(Assignee)

Comment 3

13 years ago
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.
Flags: blocking-aviary1.0mac?
(Assignee)

Updated

13 years ago
Attachment #157502 - Flags: review?(peterv)
(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)
QA Contact: jrgmorrison → bugs.mano
*** Bug 259343 has been marked as a duplicate of this bug. ***
Created attachment 163009 [details] [diff] [review]
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
Attachment #163009 - Attachment description: proposed addition → proposed addition (diff -u)
Created attachment 163010 [details] [diff] [review]
diff -w

Updated

13 years ago
Blocks: 222364
(Assignee)

Comment 8

13 years ago
mano: Do you have any specific examples of dialogs that your additions change? 
Like a dialog that is now (or no longer) a sheet?
(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.
Flags: blocking-aviary1.0mac? → blocking-aviary1.0?
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).
Attachment #163009 - Flags: superreview?(sfraser)
Attachment #163009 - Flags: review?(sfraser)

Comment 11

13 years ago
this is pretty risky at this stage in the game. how much testing has it had?
(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.
(Assignee)

Comment 13

13 years ago
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 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.
Attachment #157502 - Flags: review?(peterv) → review+
(Assignee)

Comment 15

13 years ago
peterv, I agree completely.  That's on my plate, just haven't gotten around to
it yet.
(Assignee)

Updated

13 years ago
Attachment #157502 - Flags: superreview?(sfraser)

Updated

13 years ago
Attachment #157502 - Flags: superreview?(sfraser) → superreview+
Comment on attachment 163009 [details] [diff] [review]
proposed addition (diff -u)

obselting this one, for now.
Attachment #163009 - Attachment is obsolete: true
Attachment #163009 - Flags: superreview?(sfraser)
Attachment #163009 - Flags: review?(sfraser)
No longer blocks: 222364
Whiteboard: [have patch]
(Assignee)

Updated

13 years ago
Attachment #157502 - Flags: approval-aviary?
Whiteboard: [have patch] → [have patch] - needs approval ben

Comment 17

13 years ago
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
(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

13 years ago
I applied the first patch (sheet.diff) ... perhaps I applied it incorrectly
(patch -p0 < sheet.diff) but I didn't get errors.
(In reply to comment #19)

Well, everyting is still "sheeted", here.
We can't take this kind of thing now. We're shipping on Tuesday. 
Flags: blocking-aviary1.0? → blocking-aviary1.0-
Comment on attachment 157502 [details] [diff] [review]
patch (diff -w)

sigh
Attachment #157502 - Flags: approval-aviary?

Updated

13 years ago
Blocks: 222364
(Assignee)

Comment 23

13 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Whiteboard: [have patch] - needs approval ben
(Assignee)

Updated

13 years ago
Attachment #163010 - Attachment is obsolete: true
No longer blocks: 222364
You need to log in before you can comment on or make changes to this bug.