ESC should close the share panel

RESOLVED FIXED

Status

Cloud Services
Share: Firefox Client
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: philikon, Assigned: mixedpuppy)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
Requesting UX feedback here, please advise whether doorhanger should behave like all others (clicking on content area dismisses it) or not.
Blocks: 642684
Whiteboard: [ux-wanted]
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).
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.
Depends on: 646675
Whiteboard: [ux-wanted]
Ok, I can take this one.
Assignee: nobody → philipp
(Reporter)

Updated

7 years ago
Blocks: 650047
(Assignee)

Updated

7 years ago
Assignee: philipp → mixedpuppy
(Reporter)

Updated

7 years ago
Blocks: 651668
(Assignee)

Comment 5

7 years ago
(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.
Created attachment 527420 [details] [diff] [review]
WIP v1
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.
(Assignee)

Comment 8

7 years ago
Created attachment 527708 [details] [diff] [review]
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?
Attachment #527420 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #527708 - Flags: feedback?(philipp)
(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.
(Reporter)

Updated

7 years ago
Blocks: 651666
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.
(Assignee)

Comment 11

7 years ago
(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
(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.
(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.
(Assignee)

Comment 14

7 years ago
Created attachment 528196 [details] [diff] [review]
panel consistency with other fx panels

patch including test and TabClose fix
Attachment #527708 - Attachment is obsolete: true
Attachment #527708 - Flags: feedback?(philipp)
Attachment #528196 - Flags: feedback?(philipp)
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. :)
Attachment #528196 - Flags: feedback?(philipp) → feedback-
(Assignee)

Comment 16

7 years ago
(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.
(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!
(Assignee)

Comment 18

7 years ago
Created attachment 529848 [details] [diff] [review]
new work in progress patch

also now includes fix for bug 643991
Attachment #529848 - Flags: feedback?(philipp)
(Reporter)

Updated

7 years ago
Attachment #528196 - Attachment is obsolete: true
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 ;)
Attachment #529848 - Flags: feedback?(philipp) → feedback-
(Assignee)

Comment 20

7 years ago
Created attachment 530135 [details] [diff] [review]
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)
Attachment #529848 - Attachment is obsolete: true
Attachment #530135 - Flags: review?(philipp)
(Assignee)

Updated

7 years ago
Blocks: 654870
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?
Attachment #530135 - Flags: review?(philipp) → review+
(Assignee)

Comment 22

7 years ago
Created attachment 530210 [details] [diff] [review]
patch with additions from previous comments
(Assignee)

Comment 23

7 years ago
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.
Attachment #530210 - Flags: review?(philipp)
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 :)
Attachment #530210 - Flags: review?(philipp) → review+
(Assignee)

Comment 25

7 years ago
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.
(Assignee)

Comment 26

7 years ago
Created attachment 530213 [details] [diff] [review]
v3 cleaned up
(Reporter)

Updated

7 years ago
Attachment #530135 - Attachment is obsolete: true
(Reporter)

Updated

7 years ago
Attachment #530210 - Attachment is obsolete: true
https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/8a55957fb65e
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
Blocks: 643991
Tests committed in https://hg.mozilla.org/users/pweitershausen_mozilla.com/fx-share/rev/e7ead4b2cb0b
You need to log in before you can comment on or make changes to this bug.