644182 - ESC should close the share panel

Status: RESOLVED FIXED

(Cloud Services :: Share: Firefox Client, defect)

Description

[Philipp von Weitershausen [:philikon]]

I see there's some code in panel.js to do that, but I bet the event is sent to (or caught at) focused chrome window. Would be nice if we could get it to work.

Comment 1

[Philipp von Weitershausen [:philikon]]

Requesting UX feedback here, please advise whether doorhanger should behave like all others (clicking on content area dismisses it) or not.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Bryan's concern with click outside to close was arround initial percieved dataloss.  This is a very real and legitimate concern, but really everything about the desigb of these panels has sided on first run confusion in return for n runs of efficiency (since n is a much larger number).

His second concern was users who want to copy and paste text from the site.  here click outside to close helps in that it doesn't block access to the part of content below thr panel.  But, it also makes the interaction slightly slower in that you have to click to bring it back (instead of it just being there).

one sollution is to introduce a control on some of these panels that allows thr user to quickly dock them over in a right sidebar.  icon would probaaly look like >[]

that way we could keep the initial panel behavior internally consistent across all arroepanels in firefox.  we're considering the panel to sidebar dock for bookmarks as well as arrow panels in the addons bar (containg the contents of traditional toolbars).

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

Ok, lets go ahead and move to the normal panel usage where clicking outside of the panel will cause it to close.  That change will allow ESC to automatically close this panel.

I've assigned on bug as blocking, bug 646675, because we have a data loss issue with the form data in the panel.  These bugs can be fixed in parallel, it doesn't have to be a serial blocking as the data loss is only on the first time and magically comes back.

Comment 4

[Philipp von Weitershausen [:philikon]]

Ok, I can take this one.

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to comment #3)
> I've assigned on bug as blocking, bug 646675, because we have a data loss issue
> with the form data in the panel.  These bugs can be fixed in parallel, it
> doesn't have to be a serial blocking as the data loss is only on the first time
> and magically comes back.

Bryan, I'm going to question a bug on the web content side blocking a bug on the chrome side.  This bug has been marked as a blocker for landing into firefox, whereas the data loss issue in the content side could be fixed after landing.

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: WIP v1

Comment 7

[Mike Connor [:mconnor]]

It could in theory get fixed post-landing, but we're not going to land on mozilla-central if we have a dataloss bug in primary UI.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: panel consistency with other fx panels

largely taken from WIP v1, this patch fixes bugs 644182, 650047 and 651666

One question, does it matter that the dtd change is not preprocessed?

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to comment #8)
> One question, does it matter that the dtd change is not preprocessed?

We should ifdef it if we can. No idea whether DTDs are preprocessed. I wouldn't worry about it too much if they're not.

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 527708 [details] [diff] [review]
panel consistency with other fx panels

Can we also try to make a browser-chrome test case that synthesizes an ESC event after opening the panel, to verify it's closed?

I'll look at the rest of the patch more closely later.

Comment 11

[Shane Caraveo (:mixedpuppy)]

(In reply to comment #10)
> Comment on attachment 527708 [details] [diff] [review]
> panel consistency with other fx panels
> 
> Can we also try to make a browser-chrome test case that synthesizes an ESC
> event after opening the panel, to verify it's closed?

It looks like you already wrote one: browser_togglePanel_key.js

Comment 12

[Philipp von Weitershausen [:philikon]]

(In reply to comment #11)
> > Can we also try to make a browser-chrome test case that synthesizes an ESC
> > event after opening the panel, to verify it's closed?
> 
> It looks like you already wrote one: browser_togglePanel_key.js

True, although that presses the F1 key again to close it, not the ESC key.

Comment 13

[Mike Connor [:mconnor]]

(In reply to comment #9)
> (In reply to comment #8)
> > One question, does it matter that the dtd change is not preprocessed?
> 
> We should ifdef it if we can. No idea whether DTDs are preprocessed. I wouldn't
> worry about it too much if they're not.

We should not ifdef DTDs.  If we need to have different values per-platform, have the preprocessing in the markup and different entities per platform.

Comment 14

[Shane Caraveo (:mixedpuppy)]

Attachment: panel consistency with other fx panels

patch including test and TabClose fix

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 528196 [details] [diff] [review]
panel consistency with other fx panels

Review of attachment 528196 [details] [diff] [review]:

::: services/share/modules/main.js
@@ +77,4 @@
     if (popup.state == 'open') {
       this.sharePanel.close();
     } else {
+      this.sharePanel.show();

This is not going to be enough. On Windows, if I click the F1 key while the panel is open, the panel briefly closes and then reopens. The expected behaviour is that it stays closed. OSX behaves differently here apparently.

@@ -105,5 @@
-      }, 0);
-    } else {
-      this.window.setTimeout(function () {
-        self.sharePanel.updateStatus();
-      }, 0);

As much as I love seeing code go away, I'm curious why we can get rid of this whole method. :)

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to comment #15)
> Comment on attachment 528196 [details] [diff] [review]
> panel consistency with other fx panels
> 
> Review of attachment 528196 [details] [diff] [review]:
> 
> ::: services/share/modules/main.js
> @@ +77,4 @@
>      if (popup.state == 'open') {
>        this.sharePanel.close();
>      } else {
> +      this.sharePanel.show();
> 
> This is not going to be enough. On Windows, if I click the F1 key while the
> panel is open, the panel briefly closes and then reopens. The expected
> behaviour is that it stays closed. OSX behaves differently here apparently.

I'll look further at what other panels are doing, but there shouldn't be different behavior for an onclick handler.

> @@ -105,5 @@
> -      }, 0);
> -    } else {
> -      this.window.setTimeout(function () {
> -        self.sharePanel.updateStatus();
> -      }, 0);
> 
> As much as I love seeing code go away, I'm curious why we can get rid of this
> whole method. :)

For each tab we kept the open/closed state of the panel, and when switching between tabs we would return to that state.  This method (switchTab) handled the core of that logic.  Since it is no longer possible to keep a panel open when switching tabs, it's not necessary.

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to comment #16)
> I'll look further at what other panels are doing, but there shouldn't be
> different behavior for an onclick handler.

There *should* not, indeed. Unfortunately, this is XUL...

> For each tab we kept the open/closed state of the panel, and when switching
> between tabs we would return to that state.  This method (switchTab) handled
> the core of that logic.  Since it is no longer possible to keep a panel open
> when switching tabs, it's not necessary.

I see. Thanks!

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: new work in progress patch

also now includes fix for bug 643991

Comment 19

[Philipp von Weitershausen [:philikon]]

Comment on attachment 529848 [details] [diff] [review]
new work in progress patch

Just tested on Windows, the event.stopPropagation() call doesn't seem to change the behaviour :(

>diff --git a/services/share/modules/progress.js b/services/share/modules/progress.js
>--- a/services/share/modules/progress.js
>+++ b/services/share/modules/progress.js

Note that I folded this file into main.js and panel.js, respectively, in bug 646658. One of us will have to rebase ;)

Comment 20

[Shane Caraveo (:mixedpuppy)]

Attachment: new wip patch covering several bugs

patch covers bugs 644182 (this bug), 650047 (command click only for f1 button), 651666 (tooltip for share button), 643991 (fix error notification if user has navigated away from shared document)

Comment 21

[Philipp von Weitershausen [:philikon]]

Comment on attachment 530135 [details] [diff] [review]
new wip patch covering several bugs

>+  togglePanel: function(event) {
>+    event.stopPropagation();
>+    if (event) {

I think event.stopPropagation() should live inside this if block here, no? In fact I think your earlier WIP had this.

>+    // Tell the popup to consume dismiss clicks, to avoid bug 395314
>+    popup.popupBoxObject
>+        .setConsumeRollupEvent(Ci.nsIPopupBoxObject.ROLLUP_CONSUME);

Nit: align '.' operator.

>diff --git a/services/share/modules/panel.js b/services/share/modules/panel.js
>--- a/services/share/modules/panel.js
>+++ b/services/share/modules/panel.js
>@@ -346,32 +346,43 @@ SharePanel.prototype = {
>   // default sizes for the panel
>   defaultWidth: 400,
>   defaultHeight: 180,
> 
>   // sizes after resizing the panel to fit content
>   lastWidth: 400,
>   lastHeight: 180,
>   
>+  // shareState cache
>+  shareState: {},
>+  

This makes the share state global across all panel instances (= across all windows). I guess it's not a big deal, but it might be confusing to the casual reader. Should at least document that. Also, please document what the cache is used for and what the lifetime of a sharestate item in it is (because it's much shorter than I initially gathered from the code!).

>+    // Always ensure the button is checked if the panel is open
>+    this.panel.addEventListener('popupshown', this.panelShown.bind(this),
>+                                false);
>+    this.panel.addEventListener('popuphidden', this.panelHidden.bind(this),
>+                                false);    
>+
>     function loadListener(event) {
>       self.window.setTimeout(function () {
>         self.sizeToContent();
>       }, 0);
>     };
>     this.browser.addEventListener("load", loadListener, true);
> 
>+    // ensure the panel is closed if the tab is removed
>+    function tabCloseListener(event) {
>+      self.window.setTimeout(function () {
>+        self.close();
>+      }, 0);
>+    }
>+    this.gBrowser.tabContainer.addEventListener("TabClose", tabCloseListener, true);

I would prefer if we followed the same pattern as with 'popupshown' and 'popuphidden' above and make this a method that we bind to 'this'.

>+    if (shareState && shareState.status === 0) {
>+       delete this.shareState[tabUrl];
>     }

nit: only indent 2 spaces, please.

>   /**
>    * Updates the state of the toolbar button during a share activity or
>    * afterward when a share error is received.
>    * 
>    * The status
>    * @param {Integer} an index value that has meaning in the SHARE_STATUS array
>    * @param {Boolean} only passed by the final success call
>    */
>   updateStatus: function (statusData, success) {

Reading this function I couldn't help but think: there must be a simpler way to phrase this code. Maybe doing switch (status)? I don't know, just a general observation. If you can make it clearer, good; if not, don't worry.

>+		browser_errorNotification.js \
>+		browser_errorNotification_newtab.js \
>+		browser_escape_key.js \
>+		browser_locationChange.js \

Those files aren't part of the diff. Forgot to hg add? :)

r=me with the above points. Can you upload a patch with the tests?

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: patch with additions from previous comments

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 530210 [details] [diff] [review]
patch with additions from previous comments

note that I changed the listener function binding for tabclose, load and unload.

Comment 24

[Philipp von Weitershausen [:philikon]]

Comment on attachment 530210 [details] [diff] [review]
patch with additions from previous comments

>     // Set the browser src to load at first idle moment, if the user has
>     // previously configured sharing accounts.
>     idleService.addIdleObserver(this, IDLE_TIMEOUT);
>     // Ensure we don't leak anything if the window is closed before
>     // we had a chance to run the observer.
>-    this.removeIdleObserver = this.removeIdleObserver.bind(this);
>-    this.window.addEventListener("unload", this.removeIdleObserver, false);
>+    this.window.addEventListener("unload", this.removeIdleObserver.bind(this), false);
>   },

Please revert this change. I should've documented this better. It's vital that 'this.removeIdleObserver' actually is the the thing we register as an event handler because we want to unregister it later:

>   removeIdleObserver: function removeIdleObserver() {
>     this.window.removeEventListener("unload", this.removeIdleObserver, false);
>     idleService.removeIdleObserver(this, IDLE_TIMEOUT);
>   },

>new file mode 100644
>--- /dev/null
>+++ b/services/share/tests/browser/browser_shareState.js

This seems to be just a copy of browser_locationChange...

r=me with the change above reverted and the right file for browser_shareState :)

Comment 25

[Shane Caraveo (:mixedpuppy)]

right, I see now, just not used to using bind().  browser_shareState.js was something I started, but the browser_errorNotification* tests actually cover the share state testing, it was just an accidental hg add and wont get commited.

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: v3 cleaned up

Comment 27

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/8a55957fb65e

Comment 28

[Philipp von Weitershausen [:philikon]]

Tests committed in https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/e7ead4b2cb0b