clicking a link in social content panels should close the panel

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ fixed, firefox18+ fixed)

Details

(Whiteboard: [fx17][qa-])

Attachments

(1 attachment, 8 obsolete attachments)

This doesn't currently happen for the jewel panels. It might also be a problem for the flyout?
tracking-firefox17: --- → +
Whiteboard: [fx17]
(Assignee)

Comment 1

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

Comment 3

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(Assignee)

Comment 4

5 years ago
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?
Yeah, I guess an API makes sense. We might be able to detect clicks on links that target other windows using onBeforeLinkTraversal.
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.
Assignee: nobody → gavin.sharp
(Assignee)

Updated

5 years ago
Assignee: gavin.sharp → mixedpuppy
(Assignee)

Comment 7

5 years ago
I've split out the panel close api to bug 795518 and assigned to mark.  I'll get the click handler working.
(Assignee)

Comment 8

5 years ago
Created attachment 667129 [details] [diff] [review]
clickclose.patch
Attachment #667129 - Flags: review?(gavin.sharp)
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?
Attachment #667129 - Flags: review?(gavin.sharp)

Comment 10

5 years ago
Created attachment 667620 [details] [diff] [review]
onBeforeLinkTraversal.patch

This approach modifies onBeforeLinkTraversal to close the social panels.
Attachment #667620 - Flags: feedback?(gavin.sharp)
(Assignee)

Updated

5 years ago
Attachment #667620 - Flags: feedback?(jaws)
Attachment #667620 - Flags: feedback?(jaws)
Driveby:

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

Should have an explicit "return null;"?
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).
Attachment #667620 - Flags: feedback?(jaws) → feedback+
(Assignee)

Comment 13

5 years ago
Created attachment 668848 [details] [diff] [review]
onBeforeLinkTraversal.patch

fix use of target=_self
Attachment #667620 - Attachment is obsolete: true
Attachment #667620 - Flags: feedback?(gavin.sharp)
Attachment #668848 - Flags: review?(jaws)
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.
(Assignee)

Comment 15

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(Assignee)

Comment 16

5 years ago
Created attachment 668943 [details] [diff] [review]
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.
Attachment #668848 - Attachment is obsolete: true
Attachment #668848 - Flags: review?(jaws)
Attachment #668943 - Flags: review?(jaws)
Attachment #668943 - Flags: review?(gavin.sharp)
(Assignee)

Comment 17

5 years ago
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.
Attachment #668943 - Flags: review?(jaws) → review?(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?
Attachment #667129 - Flags: feedback+
Comment on attachment 667129 [details] [diff] [review]
clickclose.patch

oops - this patch is now obsolete, right?
Attachment #667129 - Flags: feedback+
(Assignee)

Comment 20

5 years ago
(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 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?
Attachment #668943 - Flags: review?(mhammond) → review+
Created attachment 669034 [details] [diff] [review]
rebased

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

try run at https://tbpl.mozilla.org/?tree=Try&rev=903ea027072c
Attachment #667129 - Attachment is obsolete: true
Attachment #668943 - Attachment is obsolete: true
Attachment #668943 - Flags: review?(gavin.sharp)
Attachment #669034 - Flags: review?(gavin.sharp)
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.
Attachment #669034 - Flags: review?(gavin.sharp)
Created attachment 669167 [details] [diff] [review]
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.
Attachment #669167 - Flags: feedback?(mixedpuppy)
(Assignee)

Comment 25

5 years ago
Created attachment 671613 [details] [diff] [review]
onBeforeLinkTraversal.patch
Attachment #669034 - Attachment is obsolete: true
Attachment #669167 - Attachment is obsolete: true
Attachment #669167 - Flags: feedback?(mixedpuppy)
Attachment #671613 - Flags: review?(gavin.sharp)
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.
Attachment #671613 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 27

5 years ago
Created attachment 671681 [details] [diff] [review]
onBeforeLinkTraversal.patch

test added
Attachment #671613 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=3caba9db2ff0
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.
(Assignee)

Comment 30

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(Assignee)

Comment 31

5 years ago
Created attachment 671739 [details] [diff] [review]
onBeforeLinkTraversal.patch

fix domain in link to use the test server
Attachment #671681 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
tracking-firefox18: --- → ?
Keywords: checkin-needed
tracking-firefox18: ? → +
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1d11cdda0a
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fx17] → [fx17][qa-]
https://hg.mozilla.org/mozilla-central/rev/6c1d11cdda0a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
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.
Attachment #671739 - Flags: approval-mozilla-beta+
Attachment #671739 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-beta/rev/3a696702b29b
status-firefox17: --- → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/d723943a6a2d
status-firefox18: --- → fixed
You need to log in before you can comment on or make changes to this bug.