Problem/Motivation

In the 2012 Google Usability tests we saw one poor user loose his content three times by clicking on links in the user interface. Twice in preview (click links to watch videos), once by clicking on the "Read more" link, and once by clicking on the "Create new comment" link. The third painful loss of content was caused by
#87994: Quit clobbering people's work when they click the filter tips link

Additionally, another participant was able to loose her content by clicking on a link she added into her own content from the preview.

Proposed resolution

All links that will cause loss of content on the node preview need are preempted by a core Dialog API for JavaScript, allowing the user to choose before leaving the page. For users who know what they are doing, modal confirmations can be bypassed when left-clicking or pressing any of the following keys: ALT, CTRL, META (Command key on the Macintosh keyboard), SHIFT. Comment #74 suggests,

...This is the "optimal solution", because it doesn't bother users who know what they are doing, while it keeps the others from losing their edit, and at the same time the target behaviour of the link doesn't have to be changed.

Update

#40 was committed, then the class name changed for some reason, breaking the JS. It was fixed in #52 and after some improvements a patch was committed to Drupal 8 #58.

#74 Re-introduced the solution of using a JS confirmation modal.

#77 Introduced a new patch that disabled links within preview, using confirm dialogs. This patch uses core's Dialog API for JavaScript, by adding a HTML5 modal dialog element. As seen in the overlay module, modal confirmations can be bypassed (for users who know what they are doing), when left-clicking and pressing any of the following keys: ALT, CTRL, META (Command key on the Macintosh keyboard), SHIFT.

#79 and #83 Approved the concept use of an existing Dialog API and suggested someone from the JavaScript team to review.

#84 and #88 Provided JavaScript feedback.

#93 Incorporates feedback in a current patch needing review.

Files: 
CommentFileSizeAuthor
#104 core-js-preview-links-1440662-104.patch3.06 KBrak2008
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,738 pass(es).
[ View ]
#99 core-js-preview-links-1440662-99.patch3 KBbdone
PASSED: [[SimpleTest]]: [MySQL] 59,397 pass(es).
[ View ]
#99 interdiff-99-96.txt1017 bytesbdone
#99 1440662-99-ctrl-click.png19.22 KBbdone
#96 core-js-preview-links-1440662-96.patch2.8 KBbdone
PASSED: [[SimpleTest]]: [MySQL] 58,837 pass(es).
[ View ]
#96 interdiff-96-93.txt711 bytesbdone
#93 core-js-preview-links-1440662-93.patch2.8 KBbdone
PASSED: [[SimpleTest]]: [MySQL] 59,019 pass(es).
[ View ]
#93 interdiff-93-87.txt2.68 KBbdone
#87 core-js-preview-links-1440662-85.patch2.86 KBbdone
PASSED: [[SimpleTest]]: [MySQL] 58,181 pass(es).
[ View ]
#87 interdiff-85-76.txt3.63 KBbdone
#85 core-js-preview-links-1440662-85.patch2.86 KBbdone
PASSED: [[SimpleTest]]: [MySQL] 57,936 pass(es).
[ View ]
#85 interdiff-85-76.patch3.63 KBbdone
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-85-76.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#85 Screen Shot 2013-08-19 at 11.07.20 PM.png221.18 KBbdone
#77 core-js-preview-links-1440662-76.patch3.04 KBbdone
PASSED: [[SimpleTest]]: [MySQL] 57,421 pass(es).
[ View ]
#77 confirm-exit-preview.png146.13 KBbdone
#66 core-js-preview-links-1440662-66.patch1.46 KBbdone
PASSED: [[SimpleTest]]: [MySQL] 40,189 pass(es).
[ View ]
#58 core-js-preview-links-1440662-58.patch1.18 KBbdone
PASSED: [[SimpleTest]]: [MySQL] 48,749 pass(es).
[ View ]
#56 core-js-preview-links-1440662-56.patch1.18 KBbdone
PASSED: [[SimpleTest]]: [MySQL] 48,140 pass(es).
[ View ]
#52 core-js-preview-links-1440662-52.patch937 bytesnod_
PASSED: [[SimpleTest]]: [MySQL] 48,006 pass(es).
[ View ]
#51 core-backportd7-js-preview-1440662-51.patch1.42 KBmarthinal
PASSED: [[SimpleTest]]: [MySQL] 39,510 pass(es).
[ View ]
#45 core-backportd7-js-preview-1440662-45.patch1.73 KBmarthinal
PASSED: [[SimpleTest]]: [MySQL] 39,685 pass(es).
[ View ]
#40 core-js-preview-links-1440662-40.patch2.1 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-preview-links-1440662-40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#37 d8node-preview-links-1440662-37.patch829 bytesmarthinal
PASSED: [[SimpleTest]]: [MySQL] 46,240 pass(es).
[ View ]
#34 d8node-preview-links-1440662-34.patch699 bytesmarthinal
PASSED: [[SimpleTest]]: [MySQL] 46,211 pass(es).
[ View ]
#14 drupal8.node-preview-links.13.patch617 bytessun
PASSED: [[SimpleTest]]: [MySQL] 35,007 pass(es).
[ View ]
#12 drupal8.node-preview-links.12.patch1.95 KBsun
PASSED: [[SimpleTest]]: [MySQL] 35,013 pass(es).
[ View ]

Comments

Issue tags:+DrupalWTF

Important tagging

Issue summary:View changes

finish my thought.

Issue summary:View changes

added video to p4 loosing content

Issue summary:View changes

added painful video #1

Issue summary:View changes

added another painful video

Muting is a better option than what we have today, but doing it with no explanation, may lead people to think something is not working. Maybe a target _blank or a pop up with a message like «This is just a preview, so links aren't working until you save this content» would work better.

Gmail, Google Docs et al use Javascript Alert ' Do you want to navigate away from this page ' - yes it's js only solution but target="_blank" is invalid markup.

Example code (yes needs to be Drupalified)

window.onbeforeunload = function(e){
  e = e || window.event;
  if (Drupal.settings.clobberWarning === true) {
     // For IE and Firefox
     if (e) {
       e.returnValue = Drupal.t('You have unsaved changes, if you navigate away without saving your changes will be lost');
     }
     // For Safari
     return Drupal.t('You have unsaved changes, if you navigate away without saving your changes will be lost');
  }
  return;
};

We'd set Drupal.settings.clobberWarning = true during form build.
Then we just need to set Drupal.settings.clobberWarning = false on click actions that don't lose changes - which would be via targeting a class.
Rough as guts I know.

About target blank. It was suggested to solve this issue #87994: Quit clobbering people's work when they click the filter tips link back in 2009 (comment #15) but it was set aside because it's not XHTML compliant. The issue went on for years only to be solved with target blank anyway (actually today). Seems target="_blank" is compliant with HTML5 (see #297 and #298 of #87994: Quit clobbering people's work when they click the filter tips link).

My point is, if there is a better approach that's good, but I think none of us would like to see this issue dragged till D9 or D10...

Note also: Content Lock project has a 'warning you have unsaved changes' implementation - perhaps we could borrow from it?

Title:Quit clobbering people's work when they click the term links, or the "Read more" link in node previewsAdd target_blank attribute to links in node preview
Priority:Normal» Major

Its now a core method to use target_blank for this, can we please add this to all the links in node preview? A quick patch could solve this issue. Upgrading this to major, because this caused several people in usability testing to lose their data - which is a serious issue.

Actually, I think that's the wrong fix in this case. It should just link to '#', so you don't leave the current page.

Yeah, I think webchick is likely correct here; the preview shouldn't actually do anything. :)

Seconding @webchick. Also:

It's not that simple in this case, since arbitrary modules can add arbitrary links to the node preview. We likely need to conditionally apply a #pre_render to the render array for the links, which replaces all hrefs it finds.

Not exactly a novice operation... ;) (but no, this isn't tagged as novice)

Title:Add target_blank attribute to links in node previewAdd target="_blank" attribute to links in node preview
Component:node system» node.module

Status:Active» Needs review
StatusFileSize
new1.95 KB
PASSED: [[SimpleTest]]: [MySQL] 35,013 pass(es).
[ View ]

That's it.

However, this only fixes the links attached to the bottom.

There's still the link to the user account of the author, as well as the node title/heading itself, which links to its own view.

Won't any link on the page break this? Why not disable all possible links with js?

StatusFileSize
new617 bytes
PASSED: [[SimpleTest]]: [MySQL] 35,007 pass(es).
[ View ]

Although I really love that a conditional customization as in #12 is actually viable and possible through render arrays, the remaining links off the page cannot be fixed in the same way (as they live in templates and other theme functions).

Therefore, I fear that a JS-driven solution might be the only fix that isn't limited in 1,000 ways.

i.e., like this.

#14 actually seems fairly reasonable to me.

Title:Add target="_blank" attribute to links in node previewPrevent links in node preview from being clicked

Issue tags:+Needs manual testing

Hah, stupid me. What needs to be manually tested?

Well, I'd say:

  1. Apply the patch and clear the site cache.
  2. Create a node, probably with some links in the node text.
  3. Click preview
  4. Go nuts and observe (presumably) that the links in the preview content don't do anything, but menus and such still work.

We'd probably want to check it in and outside of overlay. And, since this is tagged for backport, we should test in IE6-9, FF, Chrome, and Safari.

Edit: I should note I'm waiting for more review on sun's idea before unleashing office hours on this. :)

The patch above does some strange things inside the overlay. It seems to change some of the styling on the node/add form. And when I click author in preview it goes to node/add while removing all the data.

Outside the overlay - it all works! :D

Title:Prevent links in node preview from being clickedQuit clobbering people's work when they click on links in node previews

What about people who don't have JavaScript? is 2% of web traffic enough that we need to care about it? Probably not, just sayin'

@xjm: for #4, the current patch here disables /all/ links on node preview including menus - the idea is that you just don't let people leave that screen unless it's back to the node edit form or submitting it.

Title:Quit clobbering people's work when they click on links in node previewsPrevent links in node preview from being clicked
Status:Needs review» Needs work

Does not work in overlay.

the idea is that you just don't let people leave that screen unless it's back to the node edit form or submitting it.

Mmmm, that seems like worse UX to me than currently. Disabling links within the preview text is one thing. Disabling every link on the page? They click the preview button and suddenly can't click on any link on their site with no visual cues? -1.

The patch in #14 only disables links within the node preview, not outside.

With this method (unlinking links in preview) we lose the possibility to check manually added links if they go to the intended pages.

At least for me it's the only reason to use the preview function; solely checking the design is most of the time not helpful (because of different stylesheets and layouts etc.).

Maybe the JS should instead set a different window as the target then, like we did for the filter tips?

Here's an idea:

We can use JS similar to that suggested in #4 or #14 and for people that don't have JS enabled, we can add an ID of something like 'preview-full' to the 'Preview full version' h3 tag and have the node title links under the trimmed preview and the full post preview be an anchor to preview-full.

Does that make sense to anyone?

I like #27.

I'm not sure we need a no-js option for this, my gut feeling is that if you know enough to disable js, you know enough to open links in new tabs (and that you'll have a degraded browsing experience on lots of sites).

I like #27 too!

Issue tags:+tcdrupal2012

Manual test results in Chrome:
Patch worked as expected with overlays turned off. I then turned overlays on, created new content, and clicked preview.

Under preview trimmed version:
node title: ?render=overlay&render=overlay#/node/ -- successfully broken
username: ?render=overlay&render=overlay#/user/1 -- opens a blank add node overlay; clicking the back arrow twice returned me to the node preview with the content, which I successfully saved
read more: ?render=overlay&render=overlay#/node/ -- successfully broken
add new comment: ?render=overlay&render=overlay#/node/#comment-form -- successfully broken

Under preview full version:
node title: ?render=overlay&render=overlay#/node/ -- successfully broken
username: ?render=overlay&render=overlay#/user/1 -- opens a blank add node overlay; clicking the back arrow twice returned me to the node preview with the content, which I successfully saved

Needs more work to break the username link.

#27 == #3.

You don't set "_blank" target in editors but in previews it opened links in new tab, doesn't make sense?

#4 is do more.
"Prevent lose the content, this link will open in a new windows"
OK / Cancel

StatusFileSize
new699 bytes
PASSED: [[SimpleTest]]: [MySQL] 46,211 pass(es).
[ View ]

I think a good option is to disable the links and in the user link case we could open a new tab.

The patch uses alert but it's only to explain the idea. Maybe we could use the same popu up as when a field needs to be filled.

Status:Needs work» Needs review

+++ b/core/modules/node/node.jsundefined
@@ -45,4 +45,20 @@ Drupal.behaviors.nodeFieldsetSummaries = {
+          alert('Option not available in preview mode.');

Yech, the last thing we should do is add a JS alert. Can we remove this line and then test the behavior of that patch, both with and without overlay, both links within the preview and elsewhere on the page? See #19.

StatusFileSize
new829 bytes
PASSED: [[SimpleTest]]: [MySQL] 46,240 pass(es).
[ View ]

I tested manually within and without overlay. Seems to work correctly. Disabling href and opening user profile link in another tab.

Tested #36 in FF, Chrome.

Replicated steps in #19 none of the links with in the content was clickable including Read More and Add New Comments. User profile link opened in another tab. Menu items worked perfectly.

Issue tags:-Needs manual testing

Thanks @marthinal and @Shyamala!

StatusFileSize
new2.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-preview-links-1440662-40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

ok no idea why user profile link was special cased here but anyway, updated patch using event delegation (that's what we want to use here). no more special treatment of the user link.

Separated the behavior in it's own file since that doesn't have much to do in the node.js file.

Added a detach function for the heck of it and because all attach should have a matching detach.

tagging

please update http://drupal.org/node/1777342 with a test scenario so we'll have that for regression testing.

Issue tags:+Needs manual testing

So, two tasks:

  1. Manually test @nod_'s revision of the patch.
  2. Update the manual testing scenarios (after the patch is committed presumably?) to add something like:
    On node/add/page as user 1

    Enter some node content, including an <a> tag within the node body, and click Preview. Verify that all links within the preview do nothing when clicked, but all links outside the preview are still functional. Test both with and without overlay.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs manual testing

I tested #40 as described in #42. With or without overlay:

  • Links within the node body do nothing.
  • The "Read more" link in the node teaser preview does nothing.
  • The node title link within the preview does nothing.
  • Links outside the preview work normally.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

THIS IS SO EXCITING!!!!

I played around with it and only found one little bug, that I don't think is even worth bothering about, but I'll log it nonetheless.

If you put this in a node body:

<a href="your mom">your mom</a>

...then this behaviour doesn't work. When you click the "your mom" link, you get taken to a 404 page not found (The requested page "/node/add/your%20mom" could not be found.) and upon clicking back, your content is once again lost.

I thought this was related to spaces in the URL, but this:

<a href="http://www.google.ca/?q=your mom">your mom</a>

as well as this:

<a href="/your mom">your mom</a>

...correctly get clicks disabled during preview.

Since I can't really think of a use case for "string space string" being a valid URL, I don't think it's worth further complicating the implementation (which is such nice clean JS that even *I* can read it), so...

Committed and pushed to 8.x. :D :D

Would *love* to backport this as well, as long as it passes David's sniff test.

StatusFileSize
new1.73 KB
PASSED: [[SimpleTest]]: [MySQL] 39,685 pass(es).
[ View ]

Backport d7

First attempt :)

Status:Patch (to be ported)» Needs review

Status:Needs review» Needs work

The last submitted patch, core-backportd7-js-preview-1440662-45.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

use delegate() and $(context).find() works in D7 too.

Is the #attached in the form really required? I'd assume the one in theme_node_preview works just fine?

Status:Needs work» Needs review
StatusFileSize
new1.42 KB
PASSED: [[SimpleTest]]: [MySQL] 39,510 pass(es).
[ View ]

updated and attached

Version:7.x-dev» 8.x-dev
StatusFileSize
new937 bytes
PASSED: [[SimpleTest]]: [MySQL] 48,006 pass(es).
[ View ]

the CSS class preview was removed from the HTML (haven't tracked down which issue removed it), breaking the JS.

Just selecting the .node class since we're attaching this JS only on preview it won't interfere with other things outside of the preview.

Issue tags:+Needs manual testing

Status:Needs review» Reviewed & tested by the community

Tested and confirmed that patch #52 prevents all links within preview from working.
Tested the following steps (before and after applying patch):

  1. Clear site cache.
  2. Creating a node with test links in the node's body text.
  3. Click preview.
  4. Click test links within the body.

Before applying the patch, clicking links worked normally.
After applying the patch, clicking links has no effect.

Seems like it blocked all links and without any friendly message to users.

<a href="#next">GO TO NEXT POINT</a>

anchor links navigate to content on same page.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.18 KB
PASSED: [[SimpleTest]]: [MySQL] 48,140 pass(es).
[ View ]

this patch adds support to retain local anchor links, or fragment identifiers.

it does not add any kind of message to the user. what's the best implementation for messages?

adding exceptions for anchor links requires more testing, when admin overlay is enabled.

Thanks @bdone!

+++ b/core/modules/node/node.preview.jsundefined
@@ -3,20 +3,20 @@
- * Disabling all links in node previews to prevent users from leaving the page.
+ * Disabling all links (except local fragment identifiers such as href="#frag") in node previews to prevent users from leaving the page.

I can't comment on the fragment identifier change, but this comment should be wrapped at 80 characters as per http://drupal.org/node/1354.

StatusFileSize
new1.18 KB
PASSED: [[SimpleTest]]: [MySQL] 48,749 pass(es).
[ View ]

this patch wraps comments at 80 characters, per comment #57. i've updated my text editor to show me where to break. thx @cottser.

Status:Needs review» Reviewed & tested by the community

i've re-tested using the following steps:

  1. clear site cache.
  2. creating a node with test links in the node's body text.
  3. set text format to "Full HTML".
  4. click preview.
  5. click test links within the body.

confirming that #58 is working in d8 with and without overlay for the following:

  • Google Chrome Version 23.0.1271.64
  • Firefox 16.0.2
  • Safari Version 6.0.2 (8536.26.17)
  • IE 9.0
  • IE 8.0
  • IE 7.0

Status:Reviewed & tested by the community» Needs review

Thanks @bdone! Excellent testing.

You can't RTBC your own patch, though.

whoops, that's good to know! thanks @xjm.

Status:Needs review» Reviewed & tested by the community

#40 was committed, then the class name changed for some reason, breaking the JS. It was fixed in #52 and after some improvments we have #58 that is RTBC.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Version:8.x-dev» 7.x-dev
Status:Closed (fixed)» Needs work

This got closed, but there's a Drupal 7 patch in progress above.

Issue summary:View changes

quote repair

Status:Needs work» Needs review
Issue tags:-Needs manual testing, -needs backport to D7
StatusFileSize
new1.46 KB
PASSED: [[SimpleTest]]: [MySQL] 40,189 pass(es).
[ View ]

here's a backport for d7 ready for review.

re-tested using the following steps:

  1. clear site cache.
  2. creating a node with test links in the node's body text.
  3. set text format to "Full HTML".
  4. click preview.
  5. click test links within the body.

confirmed working in d7 with and without overlay for the following:

  • Google Chrome Version 27.0.1453.110
  • Firefox 16.0.2
  • Safari Version 6.0.5 (8536.30.1)
  • IE 10.0
  • IE 9.0
  • IE 8.0
  • IE 7.0

Title:Prevent links in node preview from being clickedUX regression: Prevent links in node preview from being clicked
Version:7.x-dev» 8.x-dev
Issue tags:+regression, +needs backport to D7

Sorry for interfering, but I firmly believe this is a major usability regression, even a WTF:
I often use the preview to check if a link really works as it should, meaning:
- I didn't bork it for some reason
- I didn't confuse the URL with the wrong one.
Both isn't possibly anymore, leading to users saving the node first, then checking the link, then editing the node again. Or it leads to not checking the link at all. This is clearly not what we want here.
Whoever isn't sure what the text filter does, also might get the feeling the link is being stripped, so encounters a WTF.

I really believe we should instead raise a JS warning (see #3) and/or target="_blank" which seems to be HTML5 compliant (see #5).

you could stay adding a target blank changes the behavior of the link and doesn't provide a real preview either. It's less wrong than disabling it though.

I don't mind adding target blank. A JS warning wouldn't be appropriate. There is zero chance editors will see it and wouldn't help developers much.

Status:Needs review» Active

A JS warning wouldn't be appropriate. There is zero chance editors will see it...

Why wouldn't editors see the JS warning, or more exactly: a JS confirmation? Compare Google code in #3 ff.

... and wouldn't help developers much.

??

oh I thought you were talking about condole.log().

Ahh okay, sorry, I wasn't too clear about that.
I'd tend to using a target blank, though, because it gets less in the way than a JS confirmation modal. Actually following a link from the preview should never cancel the edit, and opening the link in a new tab via context menu works anyway, so this should be consistent.

This is a new usability issue, so it's a regression, and we certainly don't want to keep users from posting working links.
But that being said, overall it is certainly much better than it was before.

Downgrading this to normal, the issue that's introduced is less bad than the one that was fixed, however target blank seems like a good compromise.

Priority:Major» Normal

Assigned:Unassigned» Pancho

Correct.
Now what's the expected behaviour?

Currently we're having:

  1. Node title link and "Read more" link do nothing.
  2. Author link does nothing.
  3. URL links within the node body do nothing.
  4. Mailto links within the node body do nothing.
  5. Links to a local anchor within the node body work normally.
  6. Links outside the preview work normally.

And expected might be:

  1. Node title link and "Read more" link scroll to the full preview.
  2. Author link works only in a new tab.
  3. URL links work only in a new tab.
  4. Mailto links work normally.
  5. Links to a local anchor within the node body work normally.
  6. Links outside the preview work normally.

The first point might be deferred to a followup issue, so let's focus on the next three:
A major argument against a JS confirmation modal was that it unnecessarily gets in the way of users who know what they are doing. However, it might be an interesting alternative, if at the same time CTRL-click does work without confirmation.
If possible, this might even be the optimal solution, because it doesn't bother users who know what they are doing, while it keeps the others from losing their edit, and at the same time the target behaviour of the link doesn't have to be changed.
I'm going to dig into this a bit.

Issue summary:View changes

Issue Summary Update

Updated the issue summary with the most recent info.

Assigned:Pancho» bdone

A major argument against a JS confirmation modal was that it unnecessarily gets in the way of users who know what they are doing. However, it might be an interesting alternative, if at the same time CTRL-click does work without confirmation.

@Pancho i am going to attempt a pass at this. hope it is okay to assign to myself. please let me know if you're still actively working on it though.

Status:Active» Needs review
StatusFileSize
new146.13 KB
new3.04 KB
PASSED: [[SimpleTest]]: [MySQL] 57,421 pass(es).
[ View ]

here's a new patch that replaces the disabling of links within preview, with confirm dialogs. this uses core's Dialog API for JavaScript, by adding a HTML5 modal dialog element.

confirm-exit-preview.png

as seen in the overlay module, modal confirmations can be bypassed, when left-clicking and pressing any of the following keys:

  • ALT
  • CTRL
  • META (Command key on the Macintosh keyboard)
  • SHIFT

here are a few video recordings showing the modals on a few browsers:

sample links in testing: https://gist.github.com/bdone/4108670/raw/5a3233e0108c8857413674a35b0c24...

Status:Needs review» Needs work

The last submitted patch, core-js-preview-links-1440662-76.patch, failed testing.

I like the concept, good use of an existing API.

Status:Needs work» Needs review

Issue tags:+Needs manual testing

adding tag, "Needs manual testing"

+++ b/core/modules/node/node.preview.jsundefined
@@ -1,28 +1,43 @@
+              Cancel: function() {
...
+              'Exit preview': function() {

Should be wrapped in Drupal.t().

Assigned:bdone» nod_

Looks good to me, but need someone from the JS team to review, left message with nod_ on @irc
Re-assigning

Assigned:nod_» Unassigned
Status:Needs review» Needs work
  • @@ -1,28 +1,43 @@
    -
    -"use strict";
    -
    -/**
    - * Disabling all links (except local fragment identifiers such as href="#frag")
    - * in node previews to prevent users from leaving the page.
    - */
    -Drupal.behaviors.nodePreviewDestroyLinks = {
    -  attach: function (context) {
    -    var $preview = $(context).find('.node').once('node-preview');
    -    if ($preview.length) {
    -      $preview.on('click.preview', 'a:not([href^=#])', function (e) {
    -        e.preventDefault();
    +  "use strict";
    +  /**
    +   * Provide a confirmation for all links (except local fragment identifiers
    +   * such as href="#frag") in node previews to warn users before leaving the page.
    +   */
    +  Drupal.behaviors.nodePreviewDestroyLinks = {

    Thanks for paying attention to the indention detail per the JavaScript coding standards! However, we should keep the top level of code non-indented initially. Lines inside of functions and control logic should be indented code.

  • @@ -1,28 +1,43 @@
    +  /**
    +   * Provide a confirmation for all links (except local fragment identifiers
    +   * such as href="#frag") in node previews to warn users before leaving the page.
    +   */

    This should be a single summary line, less than 80 characters in length. If more text is needed, create a new paragraph with a space in between the summary line and the paragraph. @see https://drupal.org/node/1354#drupal

  • @@ -1,28 +1,43 @@
    +        // Only confirm exiting previews when left-clicking and user is not
    ...
    +            '<p>Are you sure you want to exit the preview? If you navigate away ' +
    ...
    +              'Exit preview': function() {

    This one is rather nit-picky (and probably just personal preference), but I think would come across more pleasant in the context of "Leave preview" instead of "Exit preview". It notes a softer tone I think.

  • @@ -1,28 +1,43 @@
    +          var previewElement = $('<div id="' + previewElementId + '">' +

    This is assigning an element to a variable even though it isn't being used. Can we refactor to either use this or get rid of the assignment?

  • @@ -1,28 +1,43 @@
    +            '<p>Are you sure you want to exit the preview? If you navigate away ' +
    +            'without saving, your changes will be lost.</p></div>').appendTo('body');
    ...
    +              Cancel: function() {
    ...
    +              'Exit preview': function() {

    Strings (no HTML) should be wrapped in Drupal.t() so they can be translated per https://drupal.org/node/172169#translation They should also probably be wrapped in Drupal.announce() for accessibility so screen readers can announce these strings.

Status:Needs work» Needs review
StatusFileSize
new221.18 KB
new3.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-85-76.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.86 KB
PASSED: [[SimpleTest]]: [MySQL] 57,936 pass(es).
[ View ]

attaching a re-roll of #76, including the following:

  • added Drupal.t() per @tim.plunkett in #82
  • and updates from @Mark Carver's helpful feedback in #84

screenshot:

Screen Shot 2013-08-19 at 11.07.20 PM.png

Status:Needs review» Needs work

The last submitted patch, interdiff-85-76.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.63 KB
new2.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,181 pass(es).
[ View ]

re-attaching patch #85, with a re-named interdiff, so tests won't fail.

Status:Needs review» Needs work

As promised (irc), here is a much more detailed review :)

  1. +++ b/core/modules/node/node.preview.js
    @@ -1,26 +1,48 @@
    + * Provides a modal confirmation before leaving node previews.

    "... before leaving a node preview."

  2. +++ b/core/modules/node/node.preview.js
    @@ -1,26 +1,48 @@
    +    $('a:not([href^=#])', context).on('click.preview', function (event) {
    ...
    +      $('a:not([href^=#])', context).off('click.preview');

    These should both use the parent as the event handler (ie: context) and pass the selector in the .on method (see jQuery Coding Standards - Event Delegation:

    $(context).on('click.preview', 'a:not([href^=#])', function (event) {..});
    $(context).off('click.preview', 'a:not([href^=#])');
  3. +++ b/core/modules/node/node.preview.js
    @@ -1,26 +1,48 @@
    +        event.stopPropagation();

    Not entirely sure we need this here. Doesn't really hurt, I guess, but it does seem a bit odd since the event binding happens on context anyway.

  4. +++ b/core/modules/node/node.preview.js
    @@ -1,26 +1,48 @@
    +            Cancel: function() {
    ...
    +            'Leave preview': function() {

    Wrap the button text in Drupal.t(), like:

    Drupal.t('Cancel'): function() {
    Drupal.t('Leave preview'): function() {
  5. +++ b/core/modules/node/node.preview.js
    @@ -1,26 +1,48 @@
    +        var previewElementId = 'node-preview-dialog';
    ...
    +        var previewDialog = Drupal.dialog('#' + previewElementId, dialogOptions);
    +        previewDialog.showModal();

    I know that Drupal.dialog(element, options) requires an element as the first param, but all it does is wrap in a jQuery object anyway ($(element)). jQuery will recognize that it's already an instantiated jQuery object and move on. I think this entire code block could be simplified even more (since we're not actually using any of these instantiated vars elsewhere):

    var $previewDialog = $('<div/') // rest of div chain from the next comment.
    Drupal.dialog($previewDialog, {
      // Dialog options here.
    }).showModal();
  6. +++ b/core/modules/node/node.preview.js
    @@ -1,26 +1,48 @@
    +        var previewDialog = Drupal.dialog('#' + previewElementId, dialogOptions);
    +        previewDialog.showModal();

    I'm not sure a <p/> tag is necessary here. Also we should follow the jQuery Coding Standards - Chaining for better readability. I would also maybe restructure the text a bit, stating what action will happen if the user chooses to leave and then asking them to confirm that choice after. Also, FWIW, I'm almost tempted to say we maybe should provide a third option: "Open in new window" or something (ie: user may want to continue to the link, but keep the preview window open also). Thoughts on this?

    var $previewDialog = $('<div/>')
      .text(Drupal.t('Leaving the preview will cause unsaved changes to be lost. Are you sure you want to leave the preview?')
      .appendTo('body');

Wait, what!? Why do we want a modal? Sorry, but just disabling links would have been fine.

@Bojhan, in response to #67 (which I agree with). @bdone, I will amend my comment on #88.2 to limit the links inside just the node:

$(context).find('.node').on('click.preview', 'a:not([href^=#])', function (event) {..});
$(context).find('.node').off('click.preview', 'a:not([href^=#])');

@marc Oke, I do get that - but why don't we just have blank as a target? Using a modal for this is highly unusable

Using a modal for this is highly unusable

Why, can you explain it?

Status:Needs work» Needs review
StatusFileSize
new2.68 KB
new2.8 KB
PASSED: [[SimpleTest]]: [MySQL] 59,019 pass(es).
[ View ]

here's a re-roll of #87 addressing each point of @Mark Carver's review.

there hasn't really been an argument here to NOT use a modal. the comment in #74 suggests a JS confirmation modal, provided that...

it does not get in the way of users who know what they are doing, if CTRL-click bypasses the confirmation.

the patch in #93 addresses that requirement and provides such a workaround. providing a modal:

* uses a core api
* is accessible, since jquery ui is
* has a work around for CTRL-clicking, to bypass confirmations
* would have prevented the poor user in this video from losing his work

Issue summary:View changes

updated with the most recent information

Issue summary:View changes

@bdone: issue summary update

Status:Needs review» Needs work

@bdone, this is awesome! Thanks! Just one very, very minor coding standards issue that popped out to me:

+++ b/core/modules/node/node.preview.js
@@ -8,41 +8,44 @@
+              click: function() {
...
+              click: function() {

Anonymous functions should have a space between function and ().

@Bojhan, re: #89

Wait, what!? Why do we want a modal? Sorry, but just disabling links would have been fine.

No this wouldn't be "fine". Preventing default behavior (for non-technical people) without giving an explanation of why that link no longer "works" is very bad UX. If someone is clicking a link, they obviously expect it to go somewhere. They're not developers, they don't know that there was a discussion about how to handle accidental link clicking when in preview mode. Yes, a modal is necessary. If you don't like the modal, CTRL+Click the link then.

@bdone: In retrospect, perhaps we should also inform people about this CTRL+Click feature too. Add below in the dialog text: "CTRL+Click will prevent this dialog from showing and proceed to the link clicked."

Status:Needs work» Needs review
StatusFileSize
new23.42 KB
new711 bytes
new2.8 KB
PASSED: [[SimpleTest]]: [MySQL] 58,837 pass(es).
[ View ]

new patch, adding anonymous function single space between the word "function" and the left parenthesis "(".

if we add a note about CTRL+click, should it have more markup to separate it visually? is this too "cluttered"?

1440662-96-ctrl-click.png

should the message also describe the additional options, as they were worded in overlay's code comments?

// Only confirm leaving previews when left-clicking and user is not
// pressing the ALT, CTRL, META (Command key on the Macintosh keyboard)
// or SHIFT key.

Status:Needs review» Needs work

Hmm... maybe:

.html('<p>' +
  Drupal.t('Leaving the preview will cause unsaved changes to be lost. Are you sure you want to leave the preview?') +
  '</p><small class="description">' +
  Drupal.t('CTRL+Left Click will prevent this dialog from showing and proceed to the link clicked.') + '</small>')
)

I'm not too thrilled with that, but maybe it'll make it look more "decent"?

Status:Needs work» Needs review
StatusFileSize
new19.22 KB
new1017 bytes
new3 KB
PASSED: [[SimpleTest]]: [MySQL] 59,397 pass(es).
[ View ]

thanks @Mark Carver, i think it looks nicer. i changed the wording only slightly.

1440662-99-ctrl-click.png

The last submitted patch, core-js-preview-links-1440662-99.patch, failed testing.

Issue summary:View changes

@bdone: issue summary present tense wording

Component:node.module» node system
Issue summary:View changes

(Merging "node system" and "node.module" components for 8.x; disregard.)

Status:Needs work» Needs review

"CTRL + Left" for Windows only. Maybe suggest to "Right click on the link and open in new tab/windows) ?

Issue tags:-needs backport to D7+needs back port to D7
StatusFileSize
new3.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,738 pass(es).
[ View ]

I re-roll the patches from 99