Closed Bug 224454 Opened 18 years ago Closed 15 years ago

Prompts should not be application modal but just window modal

Categories

(Core Graveyard :: Embedding: GTK Widget, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mpgritti, Assigned: chpe)

References

Details

(Keywords: fixed-aviary1.0.8, fixed1.7.13)

Attachments

(1 file, 5 obsolete files)

That should be impossible to implement using window groups, exactly like
mozilla-the-browser.
So I tried to implement it. It mostly works as expected but for some reason page
loading is blocked until you close the dialog. Going to attach the diff anyway.
Attached patch Incomplete implementation (obsolete) — Splinter Review
Attachment #134647 - Attachment is obsolete: true
If it's blocking page loading that probably means that the event queue is being
pushed.
The only case where it blocks loading is actually with the dns not found dialog.
The others works fine, odd.
Attached patch another patch (obsolete) — Splinter Review
This has the same effect as the patch above, but without its bugs :)
For a dialogue to be modal to another window, it's sufficient to add it to the
window group, and use gtk_dialog_run().

Of course, this is not of much use without fixing the blockage in the other
windows.
Attached patch this is the right patch (obsolete) — Splinter Review
Attachment #169121 - Attachment is obsolete: true
Are you looking for review for that?

Also, is the group automatically destroyed when the window is destroyed?
The patch looks correct to me. We release our own reference on the group so it
will be destroyed when the window is destroyed.

The problem is that the loading blockage in the other windows could be very
confusing for the users. I guess that's why Christian didnt ask a review yet.
Attachment #134648 - Attachment is obsolete: true
(In reply to comment #9)
> The problem is that the loading blockage in the other windows could be very
> confusing for the users. I guess that's why Christian didnt ask a review yet.
Yes, that's why I didn't seek review on the patch.

Also, in #epiphany when we discussed this we thought it might be better to add
the dialogue only to the window group if the parent window _already has_ a
window group, so that we don't make dialogues non-modal for unsuspecting
embedders. If you want, I'll update the patch accordingly.
Attached patch updated patch (obsolete) — Splinter Review
I've updated the patch to add the prompt to the parent's window group iff that
group already exists. That way we don't break applications' expectations about
prompt modality.

However, there is a problem with apps which DO use window groups: if all
windows are in a window group, but the prompts are in none (i.e. without this
patch), the prompt ends up being NOT MODAL at all, i.e. you can close the its
parent tab (it's destroy-with-parent, but with a multi-tab app like Epiphany,
that doesn't say anything), and the prompt persists, causing a crash when
dismissed [http://bugzilla.gnome.org/show_bug.cgi?id=165130]. Which means that
we need this patch now, even though the problem with the blocking of content
loading and link clicking in other windows isn't resolved :/
Attachment #169126 - Attachment is obsolete: true
Attachment #172294 - Flags: review?(marco)
Comment on attachment 172294 [details] [diff] [review]
updated patch

You probably should use // commments for consistency.

The two GTK_WINDOW(aParentWindow) casts seem unnecessary.
Attachment #172294 - Flags: review?(marco) → review-
Attachment #172294 - Attachment is obsolete: true
Attachment #172346 - Flags: superreview?(blizzard)
Attachment #172346 - Flags: review?(marco)
Attachment #172346 - Flags: review?(marco) → review+
Attachment #172346 - Flags: superreview?(blizzard) → superreview+
Comment on attachment 172346 [details] [diff] [review]
updated patch with reviewer's objections addressed

This is an important fix for embedders which use window groups (Epiphany) (see
comment 11 and the gnome bug referenced therein), and doesn't change anything
if they don't; so it's low risk and should go on branches.
Attachment #172346 - Flags: approval1.7.6?
Attachment #172346 - Flags: approval-aviary1.0.1?
Comment on attachment 172346 [details] [diff] [review]
updated patch with reviewer's objections addressed

too late for hitchhikers on the security release. a-
Attachment #172346 - Flags: approval1.7.6?
Attachment #172346 - Flags: approval1.7.6-
Attachment #172346 - Flags: approval-aviary1.0.1?
Attachment #172346 - Flags: approval-aviary1.0.1-
...but please don't forget to land on the trunk.
Comment on attachment 172346 [details] [diff] [review]
updated patch with reviewer's objections addressed

This is an important fix for embedders which use window groups (Epiphany) (see
comment 11 and the gnome bug referenced therein), and doesn't change anything
if they don't; so it's low risk and should go on branches.
Attachment #172346 - Flags: approval1.7.7?
Attachment #172346 - Flags: approval-aviary1.0.3?
*** Bug 288617 has been marked as a duplicate of this bug. ***
Is this code used at all in the Firefox and Mozilla Suite apps?
(In reply to comment #19)
> Is this code used at all in the Firefox and Mozilla Suite apps?

No. It's only used in embedding apps that use GtkMozEmbed (Epiphany).
Comment on attachment 172346 [details] [diff] [review]
updated patch with reviewer's objections addressed

not making 177/103, but based on chpe's comments moving the branch nomination
along.
Attachment #172346 - Flags: approval1.7.8?
Attachment #172346 - Flags: approval1.7.7?
Attachment #172346 - Flags: approval1.7.7-
Attachment #172346 - Flags: approval-aviary1.0.4?
Attachment #172346 - Flags: approval-aviary1.0.3?
Attachment #172346 - Flags: approval-aviary1.0.3-
has this landed on the trunk? Shouldn't it land there first (if it hasn't). If
it has, can we resolve it Fixed so that it's obvious it landed on the trunk?
(In reply to comment #22)
> has this landed on the trunk? Shouldn't it land there first (if it hasn't). If
> it has, can we resolve it Fixed so that it's obvious it landed on the trunk?

attachment.cgi 172346 is checked in on trunk. Should we mark this one FIXED and
file a follow-up bug for the blocking issue?
would be nice to have a similar (?) patch for Firefox/Thunderbird. It would
resolve bug 132710.
Comment on attachment 172346 [details] [diff] [review]
updated patch with reviewer's objections addressed

1.0.5 and 1.7.8 have shipped, removing nominations.
Attachment #172346 - Flags: approval1.7.8?
Attachment #172346 - Flags: approval-aviary1.0.5?
Comment on attachment 172346 [details] [diff] [review]
updated patch with reviewer's objections addressed

Sigh, nominating for next round of branch releases...
Attachment #172346 - Flags: approval1.7.11?
Attachment #172346 - Flags: approval-aviary1.0.7?
Attachment #172346 - Flags: approval1.7.12?
Attachment #172346 - Flags: approval1.7.11?
Attachment #172346 - Flags: approval1.7.11-
Comment on attachment 172346 [details] [diff] [review]
updated patch with reviewer's objections addressed

a=dveditz for drivers. Please add fixed-aviary1.0.8 and fixed1.7.13 keywords when checked into the branches.
Attachment #172346 - Flags: approval1.7.13?
Attachment #172346 - Flags: approval1.7.13+
Attachment #172346 - Flags: approval-aviary1.0.8?
Attachment #172346 - Flags: approval-aviary1.0.8+
Apparently fixed on trunk (comment 23)
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
-> chpe per comment 26. If this is going to make this release it needs to go in ASAP.
Assignee: blizzard → chpe
Status: REOPENED → NEW
Status: NEW → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Comment on attachment 172346 [details] [diff] [review]
updated patch with reviewer's objections addressed

mozilla/embedding/browser/gtk/src/EmbedPrompter.cpp 	1.12.2.2 	MOZILLA_1_7_BRANCH
mozilla/embedding/browser/gtk/src/EmbedPrompter.cpp 	1.12.24.2 	AVIARY_1_0_1_20050124_BRANCH
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.