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)
Firefox Graveyard
SocialAPI
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 17
People
(Reporter: mixedpuppy, Assigned: mixedpuppy)
References
Details
(Whiteboard: [Fx16])
Attachments
(1 file, 2 obsolete files)
963 bytes,
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
We need to add this back, it was removed from the patch for bug 755136
Assignee: nobody → mixedpuppy
Attachment #643640 -
Flags: review?(gavin.sharp)
Comment 2•12 years ago
|
||
Setting isAppTab shouldn't have any effect, since no one actually checks that value (apart from some sessionstore code).
Assignee | ||
Comment 3•12 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.
Comment 4•12 years ago
|
||
(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•12 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 6•12 years ago
|
||
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•12 years ago
|
||
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 8•12 years ago
|
||
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+
Updated•12 years ago
|
Summary: clicks in sidebar can change content → untargeted link clicks in the social sidebar sould open in a new tab
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #645164 -
Attachment is obsolete: true
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98c2a42a3aef
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Firefox 17
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•