794640 - clicking a link in social content panels should close the panel

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

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

This doesn't currently happen for the jewel panels. It might also be a problem for the flyout?

Comment 1

[Shane Caraveo (:mixedpuppy)]

I've been thinking about this and looking into what we would need to do, it does not seem straightforward at all.

There are two ways I am aware of that a new tab would be opened via content:

- window.open, handled in nsGlobalWindow::OpenInternal
- click on href link, handled in docShell

For the first case, I suppose we can ask providers to call window.close after, but we have to support that call for closing the panel.  The second is a bit tougher to get right unless we handle it in docShell.

Comment 2

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

Do we really need to support extended navigation in the panels? It would be really simple to close the panel any time you click in it...

Comment 3

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> Do we really need to support extended navigation in the panels? It would be
> really simple to close the panel any time you click in it...

There are features that would require clicks in the panels in which we would not want to close the panel.

Comment 4

[Shane Caraveo (:mixedpuppy)]

After some discussion here is my thoughts on how this can work.

if the click event.originalTarget is an A tag with a target, we close the panel after the click.  otherwise, content needs to close the panel if necessary (in the case that the panel uses window.open), so we need an api that allows the content to close the panel.  Mark added an api for the flyout, we can make that same api work with any content panel the provider has.

thoughts?

Comment 5

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

Yeah, I guess an API makes sense. We might be able to detect clicks on links that target other windows using onBeforeLinkTraversal.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Time to get people assigned to tracked bugs when this is about to be on Beta in a week, assigning to Gavin to get the ball rolling on this one.

Comment 7

[Shane Caraveo (:mixedpuppy)]

I've split out the panel close api to bug 795518 and assigned to mark.  I'll get the click handler working.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: clickclose.patch

Comment 9

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

Comment on attachment 667129 [details] [diff] [review]
clickclose.patch

Rather than adding a click handler here, couldn't we just add code to the existing onBeforeLinkTraversal implementation (which should already get called for all the link activations we care about) that detects whether the link in question is in a social panel (by checking e.g. target.ownerDocument == socialPanel.iframe.contentDocument, or whatever), and closing the panel if it ends up using a non-""/"_self" target?

Comment 10

[scaraveo]

Attachment: onBeforeLinkTraversal.patch

This approach modifies onBeforeLinkTraversal to close the social panels.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Driveby:

+    if (!container.getAttribute("origin"))
+      return;

Should have an explicit "return null;"?

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 667620 [details] [diff] [review]
onBeforeLinkTraversal.patch

Discussed on IRC that this looks like it doesn't handle "_self" links correctly (I guess we should leave the panel open in that case).

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: onBeforeLinkTraversal.patch

fix use of target=_self

Comment 14

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

Comment on attachment 668848 [details] [diff] [review]
onBeforeLinkTraversal.patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  _getSocialPanel: function(linkNode) {

>+    // the browser is either child or child of child of a panel
>+    if (container.parentNode.nodeName == "panel")
>+      return container.parentNode;
>+    else if (container.parentNode.parentNode.nodeName == "panel")
>+      return container.parentNode.parentNode;

This is pretty gross - it would be much nicer to just set an attribute/class on all of these browsers/iframes and just use that, as I mentioned on IRC. getSocialPanel is probably better called getSocialPanelForNode, and should probably also live in browser-social on SocialUI or something.

>   onBeforeLinkTraversal: function(originalTarget, linkURI, linkNode, isAppTab) {

>+    if (!target || target != "_self") {
>+      // find the containing panel and close it

Add some more context and explain what we're doing here, it's not obvious at all that this is related to social (aside from the getSocialPanel call).

It would be nice to have more explicit modularity here, so that the social code and generic link handling code aren't tied together so closely - have a generic way for third-party code to register arbitrary onbeforelinktraversal listeners, or some such. But that's probably overkill for now.

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #14)
> Comment on attachment 668848 [details] [diff] [review]
> onBeforeLinkTraversal.patch
> 
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >+  _getSocialPanel: function(linkNode) {
> 
> >+    // the browser is either child or child of child of a panel
> >+    if (container.parentNode.nodeName == "panel")
> >+      return container.parentNode;
> >+    else if (container.parentNode.parentNode.nodeName == "panel")
> >+      return container.parentNode.parentNode;
> 
> This is pretty gross - it would be much nicer to just set an attribute/class
> on all of these browsers/iframes and just use that, as I mentioned on IRC.

Your suggestion was to use querySelector, but you cant query a parentNode, which is what we need.  Querying for the browser itself is a bit redundant since we can easily get that as I do a couple lines earlier.  I do however, have another idea I'll look into.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: onBeforeLinkTraversal.patch

I think this is a better approach, though I'm not certain about the attribute name "closeonclick", perhaps it should be "closeontraversal".

The hbox in the social panel is unecessary, probably cruft from when we were looking at using a deck there, it's removal simplifies getting at the containing panel.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 668943 [details] [diff] [review]
onBeforeLinkTraversal.patch

Ask Mark to review, but still require gavin to review since he's had opinions here.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 667129 [details] [diff] [review]
clickclose.patch

Review of attachment 667129 [details] [diff] [review]:
-----------------------------------------------------------------

I think we need a new patch with the handlers removed (or at least not re-added)

::: browser/base/content/browser-social.js
@@ +233,5 @@
>    }, false);
>    sizeSocialPanelToContent(iframe);
>  }
>  
> +function socialPanelClickHandler(panel) {

nit: I'd prefer to see this called setupSocialPanelClickHandler or similar

@@ +237,5 @@
> +function socialPanelClickHandler(panel) {
> +  // catch clicks on flyout panels and close the panel if
> +  // click is on an A tag and the target would otherwise
> +  // cause the link to open in a new tab or window.
> +  panel.addEventListener("mouseup", function flyoutClickHandler(e) {

If social is enabled, disabled then enabled, won't this end up with multiple click handlers?

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 667129 [details] [diff] [review]
clickclose.patch

oops - this patch is now obsolete, right?

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #19)
> Comment on attachment 667129 [details] [diff] [review]
> clickclose.patch
> 
> oops - this patch is now obsolete, right?

yeah, it was two different approaches I was getting feedback on, went more in the direction of the modification on onBeforeLinkTraversal, making the clickclose.patch obsolete.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 668943 [details] [diff] [review]
onBeforeLinkTraversal.patch

Review of attachment 668943 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me, but it should have tests as they should be fairly simple to write.

::: browser/base/content/browser.js
@@ +3896,5 @@
>        this.statusText = text;
>      }
>    },
>  
> +  _closeContentPopup: function(target, linkNode) {

_maybeCloseContentPopup?

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Attachment: rebased

Rebased version - the item in my r+ hasn't been addressed.

try run at https://tbpl.mozilla.org/?tree=Try&rev=903ea027072c

Comment 23

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

Comment on attachment 669034 [details] [diff] [review]
rebased

hbox removal makes sense.

I don't really like that _closeContentPopup assumes container.parentNode is a panel, and trying to make this a generic solution seems unnecessary. I think we should use a social-specific solution (e.g. onBeforeLinkTraversal calls SocialUI.closeSocialPanel(target, linkNode) rather than _closeContentPopup()), which calls hidePopup() on container.parentNode if container.hasClass("social-container") && container.parentNode instanceof XULPopupElement, or somesuch.

Comment 24

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

Attachment: alternate proposal

Something like this. Haven't tested this (need an automated test actually), and I also haven't made sure the relevant social iframe/browsers all have the social-panel-frame class set.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: onBeforeLinkTraversal.patch

Comment 26

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

Comment on attachment 671613 [details] [diff] [review]
onBeforeLinkTraversal.patch

We need tests for this.

I'm also a bit worried about the potential performance impact of doing the closeSocialPanelForLinkTraversal "check to see whether this link traversal was in a social panel" dance for every re-targeted link traversal, but I guess we don't have any nice alternatives at the moment. Can you file a bug on adding an API for per-docshell onBeforeLinkTraversal-like notifications? Or ping me to file it if you don't want to.

Comment 27

[Shane Caraveo (:mixedpuppy)]

Attachment: onBeforeLinkTraversal.patch

test added

Comment 28

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=3caba9db2ff0

Comment 29

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

Comment on attachment 671681 [details] [diff] [review]
onBeforeLinkTraversal.patch

>diff --git a/browser/base/content/test/social_flyout.html b/browser/base/content/test/social_flyout.html

>+    <a id="traversal" href="http://example1.com">test link</a>

Probably best to make this example.com, which points to the test server, so that you don't hit the network in this test.

Comment 30

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #29)
> Comment on attachment 671681 [details] [diff] [review]
> onBeforeLinkTraversal.patch
> 
> >diff --git a/browser/base/content/test/social_flyout.html b/browser/base/content/test/social_flyout.html
> 
> >+    <a id="traversal" href="http://example1.com">test link</a>
> 
> Probably best to make this example.com, which points to the test server, so
> that you don't hit the network in this test.

It can't be example.com since that is same-origin to the test provider, traversal would not catch it.  I mistakenly however thought the server supported example1.com (there is some use in mxr) but it is not shown in:

https://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt

I'll change it to mochi.test, that should be sufficient for the test.

Comment 31

[Shane Caraveo (:mixedpuppy)]

Attachment: onBeforeLinkTraversal.patch

fix domain in link to use the test server

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1d11cdda0a

Comment 33

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/6c1d11cdda0a

Comment 34

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

Comment on attachment 671739 [details] [diff] [review]
onBeforeLinkTraversal.patch

[Triage Comment]
This should have very low risk to non-social - this only affect link clicks that are retargeted to a new tab/window. The win to social is significant, so this seems worth taking for beta 3.

Comment 35

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

https://hg.mozilla.org/releases/mozilla-beta/rev/3a696702b29b

Comment 36

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/d723943a6a2d