621506 - 'Close tabs' needs to be clicked twice to close the app when there is an another dialog opened behind

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Sudarsana Nagineni]

Steps to reproduce:
1. Open fennec
2. Load javascript:alert(12345)
3. Tap on close 'X' button 
4. Wait for the 'Confirm close' dialog to open
5. Tap on 'Close tabs'

Actual result:
'Close tabs' needs to be clicked twice to close the application. I can see that the js alert is getting closed on first click.

Expected Result:
Tap on 'Close tabs' once should close the application

Reproducible:
Always

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's fix the general case

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Better uses cases will be needed to block on this. It is a bug, but not worth blocking.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

I think we need to fix this for 4.1

Comment 6

[Wesley Johnston (:wesj)]

Attachment: WIP

WIP that basically removes all of our document.getElementById calls and replace them with dialog.querySelector stuff. Creates a new binding for these dialogs to make things look a little prettier. Unfortunately, its currently crashing Fennec when I try to test.

Also, since this is a WIP it kills off about:firstrun and turns it into a crazy fun prompt service test page.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: WIP v2

Updated WIP. Works around the crash problem I was seeing, which can apparently occur if you try to close Fennec while you have a dialog open. I'm trying to make sure all prompts are closed at shutdown now. Unfortunately, we don't keep track of prompts opened with BrowserUI.pushDialog/popDialog, so I added some of that in. Can find alternative ways around if there is a reason not to do that.

Killing all id's inside prompts meant I had to also kill the command elements inside them. I added a little PromptManager object to the main window to make it easier to get back to the prompt from the event.target of buttons or keybindings inside the dialog itself.

Asking for feedback while I look at the shutdown crash a bit more, update tests, and test away on this for awhile.

Comment 8

[Wesley Johnston (:wesj)]

Whoops. Still has my firstrun testpage-override in there too. Feel free to ignore that.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 536477 [details] [diff] [review]
WIP v2

>diff --git a/mobile/chrome/content/bindings/dialog.xml b/mobile/chrome/content/bindings/dialog.xml

>+  <binding id="promptDialog" extends="chrome://browser/content/bindings/dialog.xml#dialog">
>+    <implementation>

>+      <property name="buttons">
>+        <getter>return this.querySelector(".prompt-buttons")</getter>
>+      </property>
>+
>+      <property name="list">
>+        <getter>return this.querySelector(".prompt-list")</getter>
>+      </property>
>+

>+    </implementation>
>+  </binding>

I'm not sure how slow querySelector is, but given how often these could be called, caching (memoizing) the element getters might be worthwhile. Since each <dialog> would have it's own binding, we should not get any conflicts if we cache.

Adding getters for the checkbox and editbox would help cache the other text and chexk getters/setters too.

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>   _waitingToClose: false,
>-  closing: function closing() {
>+  closing: function closing(aSkipPrompt) {
>     // If we are already waiting for the close prompt, don't show another
>     if (this._waitingToClose)
>       return false;

_waitingToClose hack could probably be removed, right? Or you could use it instead of aSkipPrompt

>diff --git a/mobile/chrome/content/prompt/confirm.xul b/mobile/chrome/content/prompt/confirm.xul

>-    <key keycode="VK_RETURN" command="cmd_ok"/>
>-    <key keycode="VK_ESCAPE" command="cmd_cancel"/>
>+    <key keycode="VK_RETURN" oncommand="PromptManager.closeDialog(event, true)"/>
>+    <key keycode="VK_ESCAPE" oncommand="PromptManager.closeDialog(event, true)"/>

Shouldn't this pass | false | to match the cmd_cancel? Or is this something to do with that weird ESCAPE behavior we need to mimic?

>diff --git a/mobile/chrome/content/prompt/select.xul b/mobile/chrome/content/prompt/select.xul

>   <commandset>
>-    <command id="cmd_ok" oncommand="document.getElementById('prompt-select-dialog').PromptHelper.closeSelect(true)"/>
>-    <command id="cmd_cancel" oncommand="document.getElementById('prompt-select-dialog').PromptHelper.closeSelect(false)"/>
>+    <command id="cmd_ok" oncommand="PromptManager.closeDialog(event, true)"/>
>+    <command id="cmd_cancel" oncommand="PromptManager.closeDialog(event, false)"/>
>   </commandset>

<command>s still OK here?

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch v1

OK. A real patch for a real review. I wound up doing a bit more here. Wanted to make sure we push and pop these dialogs when they are shown but didn't want to use BrowserUI in the prompt service and dialog.xml binding. Instead I am depending finding the PromptManager object.

I have previously talked to dolske about merging our prompt service with desktop firefox. If I remember correctly, his idea is to have an object on window that, if the prompt service finds it, can be used to show prompts.

I've tried to do something like that here, and am using objects similar objects to what desktop uses in order to hopefully make that transition easier. Adding dolske for feedback to see whether or not I'm crazy.

Comment 12

[Cristian Nicolae (:xti)]

Is this bug related to Bug 594845? I'm asking this because Bug 641876 was marked as a duplicate of this bug, but also it looks similar to Bug 594845.

Comment 13

[Aaron Train [:aaronmt]]

Is this even reproducible anymore?  On the modal dialog in the STR in comment #0, I don't see any 'x' button nor a 'Confirm Close'.

Comment 14

[Cristian Nicolae (:xti)]

(In reply to Aaron Train [:aaronmt] from comment #13)
> Is this even reproducible anymore?  On the modal dialog in the STR in
> comment #0, I don't see any 'x' button nor a 'Confirm Close'.

Bug 641876 was marked as duplicate of this bug and it's still reproducing on the latest Aurora build.

--
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a2)Gecko/20111004
Firefox/9.0a2 Fennec/9.0a2
Device: Samsung Galaxy S
OS: Android 2.2

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 537272 [details] [diff] [review]
Patch v1

Removing these requests for now. This only affects XUL Fennec and is probably WONTFX. Or maybe once we feel good that everyone is on native we do a mass closing of XUL Fennec bugs?

Comment 16

[BMO Automation]

Closing all opened bug in a graveyard component