Closed Bug 775336 Opened 12 years ago Closed 12 years ago

untargeted link clicks in the social sidebar sould open in a new tab

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 17

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Whiteboard: [Fx16])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch same-origin webProgressListener (obsolete) — Splinter Review
We need to add this back, it was removed from the patch for bug 755136
Assignee: nobody → mixedpuppy
Attachment #643640 - Flags: review?(gavin.sharp)
Setting isAppTab shouldn't have any effect, since no one actually checks that value (apart from some sessionstore code).
Blocks: 766616
(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.
(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.
(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 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.
Attachment #643640 - Flags: review?(gavin.sharp) → review-
Attached patch updated patch (obsolete) — Splinter Review
this patches only for isAppTab, the other part of location change is moved to bug 776766.
Attachment #643640 - Attachment is obsolete: true
Attachment #645164 - Flags: feedback?(gavin.sharp)
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.
Attachment #645164 - Flags: feedback?(gavin.sharp) → feedback+
Summary: clicks in sidebar can change content → untargeted link clicks in the social sidebar sould open in a new tab
Attachment #645164 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/98c2a42a3aef
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 17
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: