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.

CommentFileSizeAuthor
#116 core-js-preview-links-1440662-116.patch2.91 KBrteijeiro
#116 interdiff.txt964 bytesrteijeiro
#109 1440662-109-padding.png156.85 KBbdone
#109 interdiff-1440662-109-106.txt714 bytesbdone
#109 core-js-preview-links-1440662-109.patch2.49 KBbdone
#108 Screen Shot 2014-09-20 at 11.50.20 am.png29.02 KBkattekrab
#106 core-js-preview-links-1440662-106.patch2.5 KBnod_
#104 core-js-preview-links-1440662-104.patch3.06 KBraedkhurayji
#99 core-js-preview-links-1440662-99.patch3 KBbdone
#99 interdiff-99-96.txt1017 bytesbdone
#99 1440662-99-ctrl-click.png19.22 KBbdone
#96 core-js-preview-links-1440662-96.patch2.8 KBbdone
#96 interdiff-96-93.txt711 bytesbdone
#96 1440662-96-ctrl-click.png23.42 KBbdone
#93 core-js-preview-links-1440662-93.patch2.8 KBbdone
#93 interdiff-93-87.txt2.68 KBbdone
#87 core-js-preview-links-1440662-85.patch2.86 KBbdone
#87 interdiff-85-76.txt3.63 KBbdone
#85 core-js-preview-links-1440662-85.patch2.86 KBbdone
#85 interdiff-85-76.patch3.63 KBbdone
#85 Screen Shot 2013-08-19 at 11.07.20 PM.png221.18 KBbdone
#77 core-js-preview-links-1440662-76.patch3.04 KBbdone
#77 confirm-exit-preview.png146.13 KBbdone
#66 core-js-preview-links-1440662-66.patch1.46 KBbdone
#58 core-js-preview-links-1440662-58.patch1.18 KBbdone
#56 core-js-preview-links-1440662-56.patch1.18 KBbdone
#52 core-js-preview-links-1440662-52.patch937 bytesnod_
#51 core-backportd7-js-preview-1440662-51.patch1.42 KBmarthinal
#45 core-backportd7-js-preview-1440662-45.patch1.73 KBmarthinal
#40 core-js-preview-links-1440662-40.patch2.1 KBnod_
#37 d8node-preview-links-1440662-37.patch829 bytesmarthinal
#34 d8node-preview-links-1440662-34.patch699 bytesmarthinal
#14 drupal8.node-preview-links.13.patch617 bytessun
#12 drupal8.node-preview-links.12.patch1.95 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jenlampton’s picture

Issue tags: +DrupalWTF

Important tagging

jenlampton’s picture

Issue summary: View changes

finish my thought.

jenlampton’s picture

Issue summary: View changes

added video to p4 loosing content

jenlampton’s picture

Issue summary: View changes

added painful video #1

jenlampton’s picture

Issue summary: View changes

added another painful video

Encarte’s picture

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.

larowlan’s picture

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.

larowlan’s picture

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.

Encarte’s picture

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...

larowlan’s picture

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

Bojhan’s picture

Title: Quit clobbering people's work when they click the term links, or the "Read more" link in node previews » Add 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.

webchick’s picture

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

xjm’s picture

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

sun’s picture

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)

sun’s picture

Title: Add target_blank attribute to links in node preview » Add target="_blank" attribute to links in node preview
Component: node system » node.module
sun’s picture

Status: Active » Needs review
FileSize
1.95 KB

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.

catch’s picture

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

sun’s picture

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.

xjm’s picture

#14 actually seems fairly reasonable to me.

xjm’s picture

Title: Add target="_blank" attribute to links in node preview » Prevent links in node preview from being clicked
xjm’s picture

Issue tags: +Needs manual testing
Bojhan’s picture

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

xjm’s picture

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. :)

Bojhan’s picture

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

jenlampton’s picture

Title: Prevent links in node preview from being clicked » Quit 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'

catch’s picture

@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.

Bojhan’s picture

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

Does not work in overlay.

xjm’s picture

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.

sun’s picture

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

no2e’s picture

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

xjm’s picture

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

jludwig’s picture

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?

catch’s picture

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

Bojhan’s picture

I like #27 too!

kendramyers’s picture

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.

droplet’s picture

#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

marthinal’s picture

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.

marthinal’s picture

Status: Needs work » Needs review
xjm’s picture

+++ 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.

marthinal’s picture

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

Shyamala’s picture

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.

xjm’s picture

Issue tags: -Needs manual testing

Thanks @marthinal and @Shyamala!

nod_’s picture

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.

nod_’s picture

tagging

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

xjm’s picture

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.

xjm’s picture

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.
webchick’s picture

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.

marthinal’s picture

Backport d7

First attempt :)

marthinal’s picture

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.

marthinal’s picture

Status: Needs work » Needs review
patrickd’s picture

nod_’s picture

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?

marthinal’s picture

Status: Needs work » Needs review
FileSize
1.42 KB

updated and attached

nod_’s picture

Version: 7.x-dev » 8.x-dev
FileSize
937 bytes

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.

xjm’s picture

Issue tags: +Needs manual testing
bdone’s picture

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.

droplet’s picture

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.

bdone’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.18 KB

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.

star-szr’s picture

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.

bdone’s picture

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

bdone’s picture

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
xjm’s picture

Status: Reviewed & tested by the community » Needs review

Thanks @bdone! Excellent testing.

You can't RTBC your own patch, though.

bdone’s picture

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

nod_’s picture

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.

catch’s picture

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.

David_Rothstein’s picture

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.

David_Rothstein’s picture

Issue summary: View changes

quote repair

bdone’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing, -Needs backport to D7
FileSize
1.46 KB

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
Pancho’s picture

Title: Prevent links in node preview from being clicked » UX 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).

nod_’s picture

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.

Pancho’s picture

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.

??

nod_’s picture

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

Pancho’s picture

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.

catch’s picture

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.

catch’s picture

Priority: Major » Normal
Pancho’s picture

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.

Pancho’s picture

Issue summary: View changes

Issue Summary Update

jenjenjen’s picture

Updated the issue summary with the most recent info.

bdone’s picture

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.

bdone’s picture

Status: Active » Needs review
FileSize
146.13 KB
3.04 KB

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.

larowlan’s picture

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

bdone’s picture

Status: Needs work » Needs review
bdone’s picture

Issue tags: +Needs manual testing

adding tag, "Needs manual testing"

tim.plunkett’s picture

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

Should be wrapped in Drupal.t().

larowlan’s picture

Assigned: bdone » nod_

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

markhalliwell’s picture

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.

bdone’s picture

Status: Needs work » Needs review
FileSize
221.18 KB
3.63 KB
2.86 KB

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.

bdone’s picture

Status: Needs work » Needs review
FileSize
3.63 KB
2.86 KB

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

markhalliwell’s picture

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');
    
Bojhan’s picture

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

markhalliwell’s picture

@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^=#])');
Bojhan’s picture

@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

droplet’s picture

Using a modal for this is highly unusable

Why, can you explain it?

bdone’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
2.8 KB

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

bdone’s picture

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

bdone’s picture

Issue summary: View changes

updated with the most recent information

bdone’s picture

Issue summary: View changes

@bdone: issue summary update

markhalliwell’s picture

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."

bdone’s picture

Status: Needs work » Needs review
FileSize
23.42 KB
711 bytes
2.8 KB

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

bdone’s picture

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.
markhalliwell’s picture

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"?

bdone’s picture

Status: Needs work » Needs review
FileSize
19.22 KB
1017 bytes
3 KB

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.

Anonymous’s picture

Issue summary: View changes

@bdone: issue summary present tense wording

xjm’s picture

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

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

bdone’s picture

Status: Needs work » Needs review
droplet’s picture

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

raedkhurayji’s picture

Issue tags: -Needs backport to D7 +needs back port to D7
FileSize
3.06 KB

I re-roll the patches from 99

swentel’s picture

Status: Needs review » Needs work
Issue tags: -

This needs total reroll with the new preview. Note that only the toolbar is now clickable, the rest should be fine.

nod_’s picture

Status: Needs work » Needs review
FileSize
2.5 KB

"Total Reroll" was an awesome movie.

Still a bit rough but applies now. Since the awesome new preview makes the back button works I wonder if we still need this at all?

swentel’s picture

I guess it can't hurt to show this warning since the toolbar is clickable, I'm almost sure people will click on it by accident;
Otoh, the cool thing is that when they press the back button on their browser and then on 'back to editing' all will be fine :)

kattekrab’s picture

I did a manual test with simplytest.me - screen grab attached.

Modal comes up. Just not displaying the padding nicely like in other's screenshots.

bdone’s picture

here's 106 again, replacing the <dialog> wrapper element w/ <div>, which seems consistent with other usages, and gives us back the missing padding.

kattekrab’s picture

Issue tags: -Needs manual testing

That looks MUCH better! :)

Removing "Needs Manual Testing".

kattekrab’s picture

Status: Needs review » Reviewed & tested by the community

I reckon it's good. Had another go with simplytest/me too

kattekrab’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +a11y

Actually - took it back off RTBC - dunno if there are any A11y implications here?

larowlan’s picture

nod_’s picture

If we did our job well, the Drupal modal is accessible. A confirmation would be nice.

star-szr’s picture

Should we remove Drupal.behaviors.nodePreviewDestroyLinks now?

rteijeiro’s picture

FileSize
964 bytes
2.91 KB

I fixed it to be sure that it's not going to break when #2234331: Change the body classes to follow Drupal 8 CSS standards is commited.

Let me know what do you think :)

mgifford’s picture

I tested it with VoiceOver and as a keyboard only user. It seems fine.

larowlan’s picture

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

per #111 and #112

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c2070f1 and pushed to 8.0.x. Thanks!

  • alexpott committed c2070f1 on 8.0.x
    Issue #1440662 by bdone, marthinal, nod_, sun, rteijeiro, rak2008 |...

Status: Fixed » Closed (fixed)

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