Last Comment Bug 775336 - untargeted link clicks in the social sidebar sould open in a new tab
: untargeted link clicks in the social sidebar sould open in a new tab
Status: RESOLVED FIXED
[Fx16]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Shane Caraveo (:mixedpuppy)
:
Mentors:
Depends on: 755136
Blocks: 766616
  Show dependency treegraph
 
Reported: 2012-07-18 15:27 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2012-07-26 09:19 PDT (History)
2 users (show)
gavin.sharp: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
same-origin webProgressListener (2.02 KB, patch)
2012-07-18 15:39 PDT, Shane Caraveo (:mixedpuppy)
gavin.sharp: review-
Details | Diff | Splinter Review
updated patch (943 bytes, patch)
2012-07-23 18:43 PDT, Shane Caraveo (:mixedpuppy)
gavin.sharp: feedback+
Details | Diff | Splinter Review
patch with updated comment (963 bytes, patch)
2012-07-25 16:28 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-07-18 15:27:06 PDT
with the removal of the webprogresslistener and setting isAppTab, sidebars can be relocated to non-provider or non-same-origin content.  Need to put those back.
Comment 1 Shane Caraveo (:mixedpuppy) 2012-07-18 15:39:25 PDT
Created attachment 643640 [details] [diff] [review]
same-origin webProgressListener

We need to add this back, it was removed from the patch for bug 755136
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-18 15:40:45 PDT
Setting isAppTab shouldn't have any effect, since no one actually checks that value (apart from some sessionstore code).
Comment 3 Shane Caraveo (:mixedpuppy) 2012-07-18 16:02:15 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #2)
> Setting isAppTab shouldn't have any effect, since no one actually checks
> that value (apart from some sessionstore code).

isAppTab is used in onBeforeLinkTraversal, in browser.js, to modify the target to _blank when the link is not same-origin.  Used in the sidebar, it prevents the sidebar from browsing to a clicked link.
Comment 4 Mark Hammond [:markh] 2012-07-18 17:00:18 PDT
(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> isAppTab is used in onBeforeLinkTraversal, in browser.js, to modify the
> target to _blank when the link is not same-origin.  Used in the sidebar, it
> prevents the sidebar from browsing to a clicked link.

But it doesn't prevent redirection or explicitly setting window.location to some other URL, right?  If we need to handle that case too, I'd guess isAppTab may not be needed as the more general solution will probably also capture the case isAppTab is handling.
Comment 5 Shane Caraveo (:mixedpuppy) 2012-07-18 17:18:54 PDT
(In reply to Mark Hammond (:markh) from comment #4)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> > isAppTab is used in onBeforeLinkTraversal, in browser.js, to modify the
> > target to _blank when the link is not same-origin.  Used in the sidebar, it
> > prevents the sidebar from browsing to a clicked link.
> 
> But it doesn't prevent redirection or explicitly setting window.location to
> some other URL, right?  If we need to handle that case too, I'd guess
> isAppTab may not be needed as the more general solution will probably also
> capture the case isAppTab is handling.

docshell implements good logic for handling link traversal and is presumably well tested for app tabs, something that we never got completely right using webprogresslistener.  

the patch handles window.location or other redirection using webprogresslistener.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-20 13:46:35 PDT
Comment on attachment 643640 [details] [diff] [review]
same-origin webProgressListener

This doesn't seem to work for me. I also tried a variant that used onStateChange instead, and it also failed.

I think using a content policy might be a more robust approach. See also https://github.com/Mossop/WebAppTabs/blob/master/docs/loading.md, which raises a couple of other issues: onBeforeLinkTraversal doesn't redirect targeted links (e.g. target="self" or target="_blank"), and we probably also have to worry about window.open at some point.
Comment 7 Shane Caraveo (:mixedpuppy) 2012-07-23 18:43:18 PDT
Created attachment 645164 [details] [diff] [review]
updated patch

this patches only for isAppTab, the other part of location change is moved to bug 776766.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-25 16:19:43 PDT
Comment on attachment 645164 [details] [diff] [review]
updated patch

In the comment, say "causes clicks on untargeted links to open new tabs". And let's file a followup on fixing this for targeted links too.
Comment 9 Shane Caraveo (:mixedpuppy) 2012-07-25 16:28:06 PDT
Created attachment 645930 [details] [diff] [review]
patch with updated comment
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-26 09:18:53 PDT
https://hg.mozilla.org/mozilla-central/rev/98c2a42a3aef

Note You need to log in before you can comment on or make changes to this bug.