Closed Bug 621506 Opened 11 years ago Closed 3 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox8 affected, firefox9 affected, firefox10 affected, fennec-)

RESOLVED WONTFIX
Firefox 7
Tracking Status
firefox8 --- affected
firefox9 --- affected
firefox10 --- affected
fennec - ---

People

(Reporter: naginenis, Unassigned)

References

Details

(Whiteboard: [fennec 6.0b1])

Attachments

(1 file, 2 obsolete files)

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
tracking-fennec: --- → ?
Let's fix the general case
Assignee: nobody → mbrubeck
tracking-fennec: ? → 2.0+
Duplicate of this bug: 622100
Better uses cases will be needed to block on this. It is a bug, but not worth blocking.
tracking-fennec: 2.0+ → 2.0-
Whiteboard: [fennec-4.1?]
Duplicate of this bug: 647094
I think we need to fix this for 4.1
Attached patch WIP (obsolete) — Splinter Review
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.
Assignee: mbrubeck → wjohnston
tracking-fennec: - → 6+
Whiteboard: [fennec-4.1?]
Attached patch WIP v2 (obsolete) — Splinter Review
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.
Attachment #530383 - Attachment is obsolete: true
Attachment #536477 - Flags: feedback?(mark.finkle)
Whoops. Still has my firstrun testpage-override in there too. Feel free to ignore that.
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?
Attachment #536477 - Flags: feedback?(mark.finkle) → feedback+
Attached patch Patch v1Splinter Review
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.
Attachment #536477 - Attachment is obsolete: true
Attachment #537272 - Flags: review?(mark.finkle)
Attachment #537272 - Flags: feedback?(dolske)
Duplicate of this bug: 641876
Whiteboard: [fennec 6.0b1]
tracking-fennec: 6+ → 8+
Target Milestone: --- → Firefox 7
tracking-fennec: 8+ → +
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.
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'.
(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
OS: Linux → Android
Hardware: x86 → ARM
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?
Attachment #537272 - Flags: review?(mark.finkle)
Attachment #537272 - Flags: feedback?(dolske)
tracking-fennec: + → -
Assignee: wjohnston → nobody
Closing all opened bug in a graveyard component
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.