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.
Comment | File | Size | Author |
---|---|---|---|
#116 | core-js-preview-links-1440662-116.patch | 2.91 KB | rteijeiro |
#116 | interdiff.txt | 964 bytes | rteijeiro |
#108 | Screen Shot 2014-09-20 at 11.50.20 am.png | 29.02 KB | kattekrab |
#106 | core-js-preview-links-1440662-106.patch | 2.5 KB | nod_ |
#104 | core-js-preview-links-1440662-104.patch | 3.06 KB | raedkhurayji |
Comments
Comment #1
jenlamptonImportant tagging
Comment #1.0
jenlamptonfinish my thought.
Comment #1.1
jenlamptonadded video to p4 loosing content
Comment #1.2
jenlamptonadded painful video #1
Comment #1.3
jenlamptonadded another painful video
Comment #2
Encarte CreditAttribution: Encarte commentedMuting 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.
Comment #3
larowlanGmail, 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.
Comment #4
larowlanExample code (yes needs to be Drupalified)
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.
Comment #5
Encarte CreditAttribution: Encarte commentedAbout 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...
Comment #6
larowlanNote also: Content Lock project has a 'warning you have unsaved changes' implementation - perhaps we could borrow from it?
Comment #7
Bojhan CreditAttribution: Bojhan commentedIts 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.
Comment #8
webchickActually, I think that's the wrong fix in this case. It should just link to '#', so you don't leave the current page.
Comment #9
xjmYeah, I think webchick is likely correct here; the preview shouldn't actually do anything. :)
Comment #10
sunSeconding @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)
Comment #11
sunComment #12
sunThat'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.
Comment #13
catchWon't any link on the page break this? Why not disable all possible links with js?
Comment #14
sunAlthough 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.
Comment #15
xjm#14 actually seems fairly reasonable to me.
Comment #16
xjmComment #17
xjmComment #18
Bojhan CreditAttribution: Bojhan commentedHah, stupid me. What needs to be manually tested?
Comment #19
xjmWell, I'd say:
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. :)
Comment #20
Bojhan CreditAttribution: Bojhan commentedThe 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
Comment #21
jenlamptonWhat about people who don't have JavaScript? is 2% of web traffic enough that we need to care about it? Probably not, just sayin'
Comment #22
catch@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.
Comment #23
Bojhan CreditAttribution: Bojhan commentedDoes not work in overlay.
Comment #24
xjmMmmm, 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.
Comment #25
sunThe patch in #14 only disables links within the node preview, not outside.
Comment #26
no2e CreditAttribution: no2e commentedWith 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.).
Comment #27
xjmMaybe the JS should instead set a different window as the target then, like we did for the filter tips?
Comment #29
jludwig CreditAttribution: jludwig commentedHere'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?
Comment #30
catchI 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).
Comment #31
Bojhan CreditAttribution: Bojhan commentedI like #27 too!
Comment #32
kendramyers CreditAttribution: kendramyers commentedManual 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.
Comment #33
droplet CreditAttribution: droplet commented#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
Comment #34
marthinal CreditAttribution: marthinal commentedI 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.
Comment #35
marthinal CreditAttribution: marthinal commentedComment #36
xjmYech, 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.
Comment #37
marthinal CreditAttribution: marthinal commentedI tested manually within and without overlay. Seems to work correctly. Disabling href and opening user profile link in another tab.
Comment #38
Shyamala CreditAttribution: Shyamala commentedTested #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.
Comment #39
xjmThanks @marthinal and @Shyamala!
Comment #40
nod_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.
Comment #41
nod_tagging
please update http://drupal.org/node/1777342 with a test scenario so we'll have that for regression testing.
Comment #42
xjmSo, two tasks:
On
node/add/page
as user 1Enter 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.Comment #43
xjmI tested #40 as described in #42. With or without overlay:
Comment #44
webchickTHIS 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:
...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:
as well as this:
...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.
Comment #45
marthinal CreditAttribution: marthinal commentedBackport d7
First attempt :)
Comment #46
marthinal CreditAttribution: marthinal commentedComment #48
marthinal CreditAttribution: marthinal commentedComment #49
patrickd CreditAttribution: patrickd commented#45: core-backportd7-js-preview-1440662-45.patch queued for re-testing.
Comment #50
nod_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?
Comment #51
marthinal CreditAttribution: marthinal commentedupdated and attached
Comment #52
nod_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.Comment #53
xjmComment #54
bdone CreditAttribution: bdone commentedTested and confirmed that patch #52 prevents all links within preview from working.
Tested the following steps (before and after applying patch):
Before applying the patch, clicking links worked normally.
After applying the patch, clicking links has no effect.
Comment #55
droplet CreditAttribution: droplet commentedSeems like it blocked all links and without any friendly message to users.
anchor links navigate to content on same page.
Comment #56
bdone CreditAttribution: bdone commentedthis 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.
Comment #57
star-szrThanks @bdone!
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.
Comment #58
bdone CreditAttribution: bdone commentedthis patch wraps comments at 80 characters, per comment #57. i've updated my text editor to show me where to break. thx @cottser.
Comment #59
bdone CreditAttribution: bdone commentedi've re-tested using the following steps:
confirming that #58 is working in d8 with and without overlay for the following:
Comment #60
xjmThanks @bdone! Excellent testing.
You can't RTBC your own patch, though.
Comment #61
bdone CreditAttribution: bdone commentedwhoops, that's good to know! thanks @xjm.
Comment #62
nod_#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.
Comment #63
catchCommitted/pushed to 8.x, thanks!
Comment #65
David_Rothstein CreditAttribution: David_Rothstein commentedThis got closed, but there's a Drupal 7 patch in progress above.
Comment #65.0
David_Rothstein CreditAttribution: David_Rothstein commentedquote repair
Comment #66
bdone CreditAttribution: bdone commentedhere's a backport for d7 ready for review.
re-tested using the following steps:
confirmed working in d7 with and without overlay for the following:
Comment #67
PanchoSorry 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).Comment #68
nod_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.
Comment #69
PanchoWhy wouldn't editors see the JS warning, or more exactly: a JS confirmation? Compare Google code in #3 ff.
??
Comment #70
nod_oh I thought you were talking about condole.log().
Comment #71
PanchoAhh 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.
Comment #72
catchDowngrading 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.
Comment #73
catchComment #74
PanchoCorrect.
Now what's the expected behaviour?
Currently we're having:
And expected might be:
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.
Comment #74.0
PanchoIssue Summary Update
Comment #75
jenjenjen CreditAttribution: jenjenjen commentedUpdated the issue summary with the most recent info.
Comment #76
bdone CreditAttribution: bdone commented@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.
Comment #77
bdone CreditAttribution: bdone commentedhere'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.
as seen in the overlay module, modal confirmations can be bypassed, when left-clicking and pressing any of the following keys:
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...
Comment #79
larowlanI like the concept, good use of an existing API.
Comment #80
bdone CreditAttribution: bdone commented#77: core-js-preview-links-1440662-76.patch queued for re-testing.
Comment #81
bdone CreditAttribution: bdone commentedadding tag, "Needs manual testing"
Comment #82
tim.plunkettShould be wrapped in Drupal.t().
Comment #83
larowlanLooks good to me, but need someone from the JS team to review, left message with nod_ on @irc
Re-assigning
Comment #84
markhalliwellThanks 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.
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
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.
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?
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 inDrupal.announce()
for accessibility so screen readers can announce these strings.Comment #85
bdone CreditAttribution: bdone commentedattaching a re-roll of #76, including the following:
screenshot:
Comment #87
bdone CreditAttribution: bdone commentedre-attaching patch #85, with a re-named interdiff, so tests won't fail.
Comment #88
markhalliwellAs promised (irc), here is a much more detailed review :)
"... before leaving a node 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: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.
Wrap the button text in
Drupal.t()
, like: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):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?Comment #89
Bojhan CreditAttribution: Bojhan commentedWait, what!? Why do we want a modal? Sorry, but just disabling links would have been fine.
Comment #90
markhalliwell@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:
Comment #91
Bojhan CreditAttribution: Bojhan commented@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
Comment #92
droplet CreditAttribution: droplet commentedWhy, can you explain it?
Comment #93
bdone CreditAttribution: bdone commentedhere's a re-roll of #87 addressing each point of @Mark Carver's review.
Comment #94
bdone CreditAttribution: bdone commentedthere hasn't really been an argument here to NOT use a modal. the comment in #74 suggests a JS confirmation modal, provided that...
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
Comment #94.0
bdone CreditAttribution: bdone commentedupdated with the most recent information
Comment #94.1
bdone CreditAttribution: bdone commented@bdone: issue summary update
Comment #95
markhalliwell@bdone, this is awesome! Thanks! Just one very, very minor coding standards issue that popped out to me:
Anonymous functions should have a space between function and ().
@Bojhan, re: #89
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."
Comment #96
bdone CreditAttribution: bdone commentednew patch, adding anonymous function single space between the word "function" and the left parenthesis "(".
Comment #97
bdone CreditAttribution: bdone commentedif we add a note about CTRL+click, should it have more markup to separate it visually? is this too "cluttered"?
should the message also describe the additional options, as they were worded in overlay's code comments?
Comment #98
markhalliwellHmm... maybe:
I'm not too thrilled with that, but maybe it'll make it look more "decent"?
Comment #99
bdone CreditAttribution: bdone commentedthanks @Mark Carver, i think it looks nicer. i changed the wording only slightly.
Comment #100.0
(not verified) CreditAttribution: commented@bdone: issue summary present tense wording
Comment #101
xjm(Merging "node system" and "node.module" components for 8.x; disregard.)
Comment #102
bdone CreditAttribution: bdone commented99: core-js-preview-links-1440662-99.patch queued for re-testing.
Comment #103
droplet CreditAttribution: droplet commented"CTRL + Left" for Windows only. Maybe suggest to "Right click on the link and open in new tab/windows) ?
Comment #104
raedkhurayjiI re-roll the patches from 99
Comment #105
swentel CreditAttribution: swentel commentedThis needs total reroll with the new preview. Note that only the toolbar is now clickable, the rest should be fine.
Comment #106
nod_"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?
Comment #107
swentel CreditAttribution: swentel commentedI 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 :)
Comment #108
kattekrab CreditAttribution: kattekrab commentedI did a manual test with simplytest.me - screen grab attached.
Modal comes up. Just not displaying the padding nicely like in other's screenshots.
Comment #109
bdone CreditAttribution: bdone commentedhere's 106 again, replacing the
<dialog>
wrapper element w/<div>
, which seems consistent with other usages, and gives us back the missing padding.Comment #110
kattekrab CreditAttribution: kattekrab commentedThat looks MUCH better! :)
Removing "Needs Manual Testing".
Comment #111
kattekrab CreditAttribution: kattekrab commentedI reckon it's good. Had another go with simplytest/me too
Comment #112
kattekrab CreditAttribution: kattekrab commentedActually - took it back off RTBC - dunno if there are any A11y implications here?
Comment #113
larowlanComment #114
nod_If we did our job well, the Drupal modal is accessible. A confirmation would be nice.
Comment #115
star-szrShould we remove Drupal.behaviors.nodePreviewDestroyLinks now?
Comment #116
rteijeiro CreditAttribution: rteijeiro commentedI 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 :)
Comment #117
mgiffordI tested it with VoiceOver and as a keyboard only user. It seems fine.
Comment #118
larowlanper #111 and #112
Comment #119
alexpottCommitted c2070f1 and pushed to 8.0.x. Thanks!