224454 - Prompts should not be application modal but just window modal

Status: RESOLVED FIXED

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

Description

[Marco Pesenti Gritti]

That should be impossible to implement using window groups, exactly like
mozilla-the-browser.

Comment 1

[Marco Pesenti Gritti]

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.

Comment 2

[Marco Pesenti Gritti]

Attachment: Incomplete implementation

Comment 3

[Marco Pesenti Gritti]

Attachment: Incomplete patch (removed some not relevant changes in my tree)

Comment 4

[Christopher Blizzard (:blizzard)]

If it's blocking page loading that probably means that the event queue is being
pushed.

Comment 5

[Marco Pesenti Gritti]

The only case where it blocks loading is actually with the dns not found dialog.
The others works fine, odd.

Comment 6

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: another patch

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.

Comment 7

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: this is the right patch

Comment 8

[Christopher Blizzard (:blizzard)]

Are you looking for review for that?

Also, is the group automatically destroyed when the window is destroyed?

Comment 9

[Marco Pesenti Gritti]

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.

Comment 10

[Christian Persch (GNOME) (away; not receiving bug mail)]

(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.

Comment 11

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch

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 :/

Comment 12

[Marco Pesenti Gritti]

Comment on attachment 172294 [details] [diff] [review]
updated patch

You probably should use // commments for consistency.

The two GTK_WINDOW(aParentWindow) casts seem unnecessary.

Comment 13

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch with reviewer's objections addressed

Comment 14

[Christian Persch (GNOME) (away; not receiving bug mail)]

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.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 172346 [details] [diff] [review]
updated patch with reviewer's objections addressed

too late for hitchhikers on the security release. a-

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

...but please don't forget to land on the trunk.

Comment 17

[Christian Persch (GNOME) (away; not receiving bug mail)]

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.

Comment 18

[Robin Monks]

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

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this code used at all in the Firefox and Mozilla Suite apps?

Comment 20

[Christian Persch (GNOME) (away; not receiving bug mail)]

(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 21

[Daniel Veditz [:dveditz]]

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.

Comment 22

[Asa Dotzler [:asa]]

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?

Comment 23

[Christian Persch (GNOME) (away; not receiving bug mail)]

(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?

Comment 24

[Dimitrios]

would be nice to have a similar (?) patch for Firefox/Thunderbird. It would
resolve bug 132710.

Comment 25

[Asa Dotzler [:asa]]

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.

Comment 26

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 172346 [details] [diff] [review]
updated patch with reviewer's objections addressed

Sigh, nominating for next round of branch releases...

Comment 27

[Daniel Veditz [:dveditz]]

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.

Comment 28

[Daniel Veditz [:dveditz]]

Apparently fixed on trunk (comment 23)

Comment 29

[Daniel Veditz [:dveditz]]

-> chpe per comment 26. If this is going to make this release it needs to go in ASAP.

Comment 30

[timeless]

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