494194 - button to close fullscreen screens

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

We need a way to get out of the awesomescreen, and we can generalize this to provide a app-quit button and a way to get out of the bookmark screen once the bookmark button is out of the title bar (see bug 494191).

See attached image.

Comment 1

[Madhava Enros [:madhava]]

Attachment: sketch of solution

didn't take the first time, for some reason

Comment 2

[Madhava Enros [:madhava]]

also - the icon doesn't have to be an arrow

Comment 3

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

Attachment: patch

This patch adds a simple mechanism for dialogs to register themselves with the application. When registered, the application knows it is visible and will close the dialog when the special button is pressed. If no dialog is visible and the special button is pressed, the app will closed.

* ctrl+w is also hooked up to the mechanism
* dialogs must implement a "close" method for the mechanism to work
* current support: awesomebar autocomplete, bookmark list, bookmark editor, bookmark folder picker
* The patch changes the toolbarbutton style to 17mm as this matches the 64px buttons and allows the special close button to position correctly

Comment 4

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

example as app close:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-close-01.png

example as dialog close:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-close-02.png

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 384217 [details] [diff] [review]
patch

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

> var BrowserUI = {

>+  _closeOrQuit: function _closeOrQuit() {
>+    // Close dialogs until we have none left, then close the application
>+    let dialog = this.activeDialog;
>+    if (dialog)
>+      dialog.close();
>+    else
>+      CommandUpdater.doCommand("cmd_quit");

The comment doesn't really match the method or method (only closes one dialog).

>+  get activeDialog() {
>+    // Return the topmost dialog
>+    if (this._dialogs.length)
>+      return this._dialogs[0];
>+    return null;
>+  },

>+  set activeDialog(aDialog) {
>+    // If we have a dialog push it on the stack and set the attr for CSS
>+    if (aDialog) {
>+      this._dialogs.push(aDialog);
>+      document.getElementById("toolbar-main").setAttribute("dialog", "true")
>+      return;
>+    }
>+
>+    // Passing null means we pop the topmost dialog
>+    if (this._dialogs.length)
>+      this._dialogs.pop();
>+
>+    // If no more dialogs are being displayed, remove the attr for CSS
>+    if (!this._dialogs.length)
>+      document.getElementById("toolbar-main").removeAttribute("dialog")
>+  },

This is a really strange API, I wouldn't expect it to be possible for activeDialog to be non-null right after I set it to null... How about a pushDialog()/popDialog() API instead?

>-  close: function() {
>+  close: function BH_close() {

>-    if (this._editor.isEditing)
>-      this._editor.stopEditing();
>+    // Note: the _editor will have already saved the data, if needed, by the time
>+    // this method is called.

What guarantees this?

Comment 8

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

(In reply to comment #7)
> (From update of attachment 384217 [details] [diff] [review])
> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >+  _closeOrQuit: function _closeOrQuit() {
> >+    // Close dialogs until we have none left, then close the application
> >+    let dialog = this.activeDialog;
> >+    if (dialog)
> >+      dialog.close();
> >+    else
> >+      CommandUpdater.doCommand("cmd_quit");
> 
> The comment doesn't really match the method or method (only closes one dialog).

Change comment to:
// Close active dialog, if we have one. If not then close the application.

> This is a really strange API, I wouldn't expect it to be possible for
> activeDialog to be non-null right after I set it to null... How about a
> pushDialog()/popDialog() API instead?

Added pushDialog(aDialog) and popDialog(). I kept the readonly activeDialog property

> >-    if (this._editor.isEditing)
> >-      this._editor.stopEditing();
> >+    // Note: the _editor will have already saved the data, if needed, by the time
> >+    // this method is called.
> 
> What guarantees this?

The "close" method is called from the "onclose" event in the binding _after_ the data has been saved.

Comment 9

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

Attachment: patch 2 - addresses comments

Unbitrotted and addressed comments

Comment 10

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

Attachment: patch 3 - missed bookmarklist

Same as patch 2, but adds the pushDialog/popDialog needed for the BookmarkList

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 387288 [details] [diff] [review]
patch 3 - missed bookmarklist

(In reply to comment #8)
> The "close" method is called from the "onclose" event in the binding _after_
> the data has been saved.

My question was more "why is onclose garanteed to fire after things have been saved", but I guess stopEditing is what fires the "close" event.

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

>+  _closeOrQuit: function _closeOrQuit() {

>+  get activeDialog() {
>+    // Return the topmost dialog
>+    if (this._dialogs.length)
>+      return this._dialogs[0];
>+    return null;

Don't really need to bother with the length check, right? Can return || null if you really care about null vs. undefined (and the strict warning, I guess?).

>+  pushDialog : function pushDialog(aDialog) {

>+    if (aDialog) {

Doesn't make any sense to call this with a null argument, right?

Doesn't FolderPicker need the same cmd_close/pushDialog/popDialog treatment? This seems to mess up the close button icon in the bookmark folder list popup (appears slightly offscreen), at least on Mac. And what about HelperAppDialog? I guess it's not as big/modal so maybe it doesn't make sense using this button?

Comment 12

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

(In reply to comment #11)
> (From update of attachment 387288 [details] [diff] [review])
> (In reply to comment #8)
> > The "close" method is called from the "onclose" event in the binding _after_
> > the data has been saved.
> 
> My question was more "why is onclose garanteed to fire after things have been
> saved", but I guess stopEditing is what fires the "close" event.

right

> >+  get activeDialog() {
> >+    // Return the topmost dialog
> >+    if (this._dialogs.length)
> >+      return this._dialogs[0];
> >+    return null;
> 
> Don't really need to bother with the length check, right? Can return || null if
> you really care about null vs. undefined (and the strict warning, I guess?).

Closer inspection reveals an error here. It should be | return this._dialogs[this._dialogs.length - 1]; |, so I kept the "if"

> >+  pushDialog : function pushDialog(aDialog) {
> 
> >+    if (aDialog) {
> 
> Doesn't make any sense to call this with a null argument, right?

Right, but that doesn't mean it won't happen. I removed the uneeded "return" in there though.

> Doesn't FolderPicker need the same cmd_close/pushDialog/popDialog treatment?
> This seems to mess up the close button icon in the bookmark folder list popup
> (appears slightly offscreen), at least on Mac. And what about HelperAppDialog?
> I guess it's not as big/modal so maybe it doesn't make sense using this button?

I added the code for FolderPicker and fixed the height of the header to match the URLbar.

HelperAppDialog doesn't need this yet, but we could add it later.

Comment 13

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

pushed: http://hg.mozilla.org/mobile-browser/rev/31dc31f6f1d9

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

verfied with beta3