Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Prevent the surprising link removal in preview. #638

Closed
wants to merge 1 commit into from

5 participants

@mateusz
Owner

Users can deal with links pointing to external sites - my clients were
surprised the external links don't work, and were asking us to fix that,
just because they have assumed preview will behave like a "real site".

@chillu was there any reason why this code was added in a first place?

If we decide to retain it, http://open.silverstripe.org/ticket/7652 needs to be fixed.

@mateusz mateusz Prevent the surprising link removal in preview.
Users can deal with links pointing to external sites - my clients were
surprised the external links don't work, and were asking us to fix that,
just because they have assumed preview will behave like a "real site".
88b79f0
@hafriedlander

It was added as part of the initial "preview updates tree and vice versa" prototype I did way back when we were scoping 3's UI. We can get rid of it as long as:

  • Any unsaved changes trigger an "are you sure you want to leave this page, you have unsaved changes" method when clicking on an external link, which works (so clicking "no" stops you from following the link)

  • Our history support is rock solid

Basically, in most designs an external link is not that different to an internal link. Content editors shouldn't ever lose work they've done because they accidentally click on an external link.

@hafriedlander

Oh, and we'll probably need to break the iframe too - clicking an external link should probably cause the CMS UI to go away.

@mateusz
Owner

Ok, makes sense. I will be looking into that, PM allowing.

@chillu
Owner

Ditto. Our history isn't rock-solid in a way that navigating back would get you exactly the same state, since we don't record the fact that you opened the history panel in the UI (or any external page you navigated to in there).
So breaking out of the iframe for external links is a necessary evil to avoid inconsistent CMS behaviour.
I would suggest to do this in a new window, maybe with a note in the bottom preview bar about it to avoid confusion?

@sminnee sminnee closed this
@sminnee sminnee reopened this
@travisbot

This pull request passes (merged 88b79f0 into 498a3fd).

@sminnee
Owner

What about opening all external links in a new window?

@chillu
Owner

Came up on a client project as well, fixed by implementing Sam's suggestion: New window, plain and simple :) 1d2288b

@chillu chillu closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 13, 2012
  1. @mateusz

    Prevent the surprising link removal in preview.

    mateusz authored
    Users can deal with links pointing to external sites - my clients were
    surprised the external links don't work, and were asking us to fix that,
    just because they have assumed preview will behave like a "real site".
This page is out of date. Refresh to see the latest.
Showing with 0 additions and 19 deletions.
  1. +0 −19 admin/javascript/LeftAndMain.Preview.js
View
19 admin/javascript/LeftAndMain.Preview.js
@@ -36,8 +36,6 @@
// Create layout and controls
this.find('iframe').addClass('center');
this.find('iframe').bind('load', function() {
- self._fixIframeLinks();
-
// Load edit view for new page, but only if the preview is activated at the moment.
// This avoids e.g. force-redirections of the edit view on RedirectorPage instances.
if(!self.is('.is-collapsed')) self.loadCurrentPage();
@@ -52,8 +50,6 @@
this.find('.cms-preview-overlay-light').hide();
$('.cms-preview-toggle-link')[this.canPreview() ? 'show' : 'hide']();
- self._fixIframeLinks();
-
this._super();
},
loadUrl: function(url) {
@@ -128,21 +124,6 @@
return !(contentEl.is('.' + blockedClasses.join(',.')));
},
- _fixIframeLinks: function() {
- var doc = this.find('iframe')[0].contentDocument;
- if(!doc) return;
-
- // Block outside links from going anywhere
- var links = doc.getElementsByTagName('A');
- for (var i = 0; i < links.length; i++) {
- var href = links[i].getAttribute('href');
- if(!href) continue;
-
- // Disable external links
- if (href.match(/^http:\/\//)) links[i].setAttribute('href', 'javascript:false');
- }
- },
-
expand: function(inclMenu) {
var self = this, containerEl = this.getLayoutContainer(), contentEl = containerEl.find('.cms-content');
this.show();
Something went wrong with that request. Please try again.