The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

unspecified
Firefox 17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx16])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 643640 [details] [diff] [review]
same-origin webProgressListener

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

Updated

5 years ago
Blocks: 766616
(Assignee)

Comment 3

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

Comment 5

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

Comment 7

5 years ago
Created attachment 645164 [details] [diff] [review]
updated patch

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
(Assignee)

Comment 9

5 years ago
Created attachment 645930 [details] [diff] [review]
patch with updated comment
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.