Problem

Numerous tests from the Minnesota ones, to the Google one just recently discovered that people do not expect the preview to be shown within their administrative interface. In light of #1510532: [META] Implement the new create content page design, we would like to implement an actual preview of content.

Proposed resolution

  • Use the TempStore to store entity objects that are currently being edited.
  • When a user clicks "Preview", the entity is written to the entity system's TempStore, and the user is redirected to a full-site preview that loads that particular entity from the TempStore. (Maybe a special query string on the URL & preview-related additions to the entity API.)
  • Add some sort of toolbar or visual indicator when the user is in the preview mode (at one of the preview URLs).
  • In NodeForm::form, check for a TempStore record for the entity, and if one is found, load that record into the form if the current user/session is the owner, or do something else if the user is not. We're not abstracting this yet to the entity form level, this can be done in follow ups.

API Changes

Probably lots of 'm?

Original report by bojhan:

In light of #1510532: [META] Implement the new create content page design, we would like to implement an actual preview of content. Numerous tests from the Minnesota ones, to the Google one just recently discovered that people do not expect the preview to be shown within their administrative interface.

We can actually make this happen by implementing a "contextual bar" that is shown in place of the shortcut bar that allows you to switch between views and go back to editing. It is similar to the pattern we use for demonstrating block regions, but its now an actual design pattern one can use for previewing - rather than a one-off design idea.

preview.jpg

CommentFileSizeAuthor
#291 interdiff.txt3.45 KBCottser
#291 1510544-291.patch35.48 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,135 pass(es). View
#286 interdiff.txt1.02 KBswentel
#286 1510544-286.patch35.54 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,389 pass(es). View
#280 interdiff.txt719 bytesswentel
#280 1510544-280.patch35.46 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#276 interdiff.txt3.36 KBplopesc
#276 preview_node-1510544-276.patch35.35 KBplopesc
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,798 pass(es). View
#274 interdiff.txt1015 bytesswentel
#274 1510544-274.patch33.91 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,830 pass(es). View
#271 interdiff.txt1.31 KBswentel
#271 1510544-271.patch34.08 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,673 pass(es). View
#266 interdiff.txt690 bytesswentel
#266 1510544-266.patch34.05 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,879 pass(es). View
#264 interdiff.txt2.93 KBswentel
#264 core-preview-1510544-264.patch34.05 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,424 pass(es), 6 fail(s), and 0 exception(s). View
#261 interdiff.txt1.79 KBswentel
#261 core-preview-1510544-261.patch33.24 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,554 pass(es). View
preview.jpg568.53 KBBojhan
#1 preview.psd6.04 MBBojhan
#55 Screen Shot 2013-01-17 at 19.24.26.png69.21 KBswentel
#55 1510544-55.patch8.57 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1510544-55.patch. Unable to apply patch. See the log in the details link for more information. View
#57 1510544-57.patch11.88 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1510544-57.patch. Unable to apply patch. See the log in the details link for more information. View
#57 Screen Shot 2013-01-17 at 22.11.11.png63.63 KBswentel
#60 Screen Shot 2013-01-17 at 23.02.24.png77.89 KBswentel
#60 1510544-60.patch14.04 KBswentel
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1510544-60.patch. Unable to apply patch. See the log in the details link for more information. View
#61 1510544-61.patch13.99 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 50,322 pass(es), 71 fail(s), and 6 exception(s). View
#62 1510544-62.patch13.89 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 50,402 pass(es), 71 fail(s), and 6 exception(s). View
#65 node_preview_65.patch19.95 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_preview_65.patch. Unable to apply patch. See the log in the details link for more information. View
#67 node_preview_67.patch14.21 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 50,372 pass(es), 87 fail(s), and 40 exception(s). View
#70 node_preview_70.patch16.63 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 50,370 pass(es), 71 fail(s), and 6 exception(s). View
#71 node_preview-71.patch22.32 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 50,476 pass(es), 58 fail(s), and 5 exception(s). View
#76 interdiff.txt8.52 KBswentel
#76 node_preview-76.patch23.86 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 50,565 pass(es), 1 fail(s), and 0 exception(s). View
#82 interdiff.txt6.06 KBswentel
#82 node_preview-82.patch23.5 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 50,617 pass(es). View
#86 node_preview-86-interdiff.txt4.75 KBaspilicious
#86 node_preview-86.patch24.7 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 50,878 pass(es). View
#88 node_preview-88.patch27.72 KBsannejn
PASSED: [[SimpleTest]]: [MySQL] 50,709 pass(es). View
#88 interdiff.txt6.28 KBsannejn
#88 Screen Shot 2013-01-20 at 15.34.10.png102.43 KBsannejn
#98 Screen Shot 2013-01-23 at 10.24.02.png34.53 KBswentel
#98 interdiff.txt825 bytesswentel
#98 node_preview-98.patch27.8 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 50,801 pass(es). View
#100 interdiff.txt738 bytesswentel
#100 node_preview-100.patch27.7 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 50,734 pass(es). View
#104 interdiff.txt7.74 KBswentel
#104 node_preview-104.patch26.98 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 50,757 pass(es), 1 fail(s), and 0 exception(s). View
#106 node_preview-106.patch26.36 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 49,263 pass(es), 4 fail(s), and 16 exception(s). View
#108 interdiff.txt656 bytesswentel
#108 node_preview-108.patch26.35 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 49,396 pass(es). View
#114 node-preview-1510544.114.patch26.45 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 49,291 pass(es). View
#114 node-preview-1510544.114.interdiff.txt1.93 KBlarowlan
#122 interdiff.txt2.33 KBswentel
#122 node-preview-1510544.122.patch28.12 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 49,324 pass(es). View
#132 interdiff.txt10.71 KBswentel
#132 node-preview-1510544.132.patch27.26 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 49,303 pass(es), 74 fail(s), and 8 exception(s). View
#153 node-preview-1510544-133.patch26 KBwebchick
FAILED: [[SimpleTest]]: [MySQL] 54,476 pass(es), 5 fail(s), and 47 exception(s). View
#156 Screen Shot 2013-04-17 at 5.35.01 PM.png80.53 KBmgifford
#159 interdiff.txt1.3 KBtim.plunkett
#159 node-1510544-159.patch26.01 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 54,513 pass(es), 1 fail(s), and 0 exception(s). View
#161 node-1510544-161.patch25.64 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 55,302 pass(es), 1 fail(s), and 0 exception(s). View
#164 node-1510544-164.patch28.66 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 55,308 pass(es). View
#164 interdiff.txt3.02 KBGábor Hojtsy
#164 Screenshot_4_26_13_3_11_PM.png55.45 KBGábor Hojtsy
#172 node-1510544-172.patch28.46 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 55,889 pass(es), 10 fail(s), and 2 exception(s). View
#177 node-1510544-177.patch28.42 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 57,237 pass(es), 10 fail(s), and 2 exception(s). View
#177 node-1510544-177.patch0 bytesGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#180 experiment.txt913 bytesGábor Hojtsy
#181 node-1510544-181.patch27.53 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] 58,267 pass(es), 10 fail(s), and 2 exception(s). View
#182 node-1510544-182.patch24.43 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-1510544-182.patch. Unable to apply patch. See the log in the details link for more information. View
#183 experiment2.txt4.93 KBGábor Hojtsy
#184 experiment3.txt12.69 KBGábor Hojtsy
#210 1510544-210.patch19.33 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,399 pass(es), 60 fail(s), and 5 exception(s). View
#210 preview.png108.8 KBswentel
#211 1510544-211.patch21.88 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,022 pass(es), 48 fail(s), and 3 exception(s). View
#213 1510544-213.patch23.84 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,414 pass(es), 29 fail(s), and 3 exception(s). View
#218 1510544-218.patch29.97 KBswentel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,468 pass(es), 2 fail(s), and 2 exception(s). View
#220 interdiff.txt7.34 KBlarowlan
#220 node-preview.220.patch33.09 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,526 pass(es). View
#225 interdiff.txt10.12 KBswentel
#225 1510544-225.patch33.48 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,529 pass(es). View
#228 preview-1510544-228.patch33.44 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 54,646 pass(es), 7,703 fail(s), and 4,687 exception(s). View
#228 interdiff.txt1.78 KBtim.plunkett
#231 preview-1510544-231.patch33.42 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,114 pass(es), 1 fail(s), and 1 exception(s). View
#231 interdiff.txt1.08 KBtim.plunkett
#234 preview-1510544-233.patch33.84 KBtim.plunkett
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,304 pass(es). View
#234 interdiff.txt425 bytestim.plunkett
#235 core-preview-1510544-235.patch34.11 KBnod_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,442 pass(es). View
#235 interdiff.txt1.73 KBnod_
#236 interdiff.txt707 bytesnod_
#236 core-preview-1510544-236.patch34.21 KBnod_
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,320 pass(es), 37 fail(s), and 0 exception(s). View
#249 core-preview-1510544-249.patch34.13 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,571 pass(es). View
#249 interdiff.txt2.26 KBswentel
#251 interdiff.txt8.5 KBWim Leers
#251 core-preview-1510544-251.patch33.25 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,253 pass(es). View
#253 Real Madrid C.F. s1b293b2bc6b65e5.s2.simplytest.0me.png142.66 KBjibran
#253 Real Madrid C.F. s1b293b2bc6b65e5.s2.simplytest.me_.png334.26 KBjibran
#254 core-preview-1510544-254.patch33.25 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,252 pass(es). View
#254 interdiff.txt1.58 KBWim Leers
#255 ie10-after-scroll.png204.72 KBmparker17
#255 ie9-after-scroll.png198.44 KBmparker17
#255 ie11-before-scroll.png122.22 KBmparker17
#255 ie10-before-scroll.png195.01 KBmparker17
#255 ie9-before-scroll.png187.46 KBmparker17
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Bojhan’s picture

FileSize
6.04 MB

Source file, ideally we tweak the proportions a bit in an actual working patch.

Bojhan’s picture

Priority: Normal » Major

Marking this major, because it seemed to initially confuse everyone we tested.

merlinofchaos’s picture

Implementing this properly will require an upgrade to how we store temporary data.

Right now, in Drupal, we store temporary data in a cache by caching the generated form. In order to preview, we have to process that form and reassemble data. This is fine when you click preview, but in a system like this, you're actually visiting a different URL from the form.

Logically, we could put the form build id in the URL and load the form and process it, but there are several weaknesses with that:

1) It is wasteful (processing a form that was not submitted)
2) Form build IDs are sha generated strings. There is no way to reproduce it if you don't have it. This means you can't do content locking.
3) It continues the trend on Drupal of the form owning the data when the form typically should only be a representation of the data.

If you're familiar with Views, you've probably seen that once you start editing a view, the view is 'locked' to prevent cross editing from another user. This is something we absolutely should have with content as well. Right now, the only protection we have is that if someone else actually submits an edit before you, and the timestamp changes, your submission will be rejected. This is, however, *after* you've made all your edits and your edits are rejected, whereas the Views method happens *before* you've made your edits so you do not waste your time.

This is accomplished by using a separate non-volatile cache (and thus it's not truly a cache) for temporary storage which is tied to the session ID. (Note: Tying to the session ID is something I would loosen in core). When editing begins, this record is created; the existence of that record for a given view is considered a lock. A mechanism is provided to break stale locks, and locks > 14 days old are automatically wiped by cron.

The actual implementation of the underlying layer is very simple: http://drupalcode.org/project/ctools.git/blob/refs/heads/7.x-1.x:/includ...

I don't use a simple key-value store (i.e, CMI) for this because that would make it much more difficult to do the locking aspect. I personally think the locking aspect is very important. Joomla! has had content edit locks since the day I first looked at it, back in 2006. They were clumsy at the time, but even then I thought that such things were essential, which is why Views have them today and I have never once regretted that decision.

IMO, to implement the above preview, we should implement something like my object cache system in core and implement a layer directly with entities so that all entities, everywhere, can automatically have content edit locking with no more difficulty to implement than editing the entity.

stevector’s picture

At my core conversation in Denver I suggested that if core accommodates "forward revisions" (saved revisions that are newer than the vid in in the node table) we may no longer need previewing tied directly to the form system. Content authors could just look at the saved node revision.

Agentrickard has a patch that starts to implement forward revisions. #218755-106: Support revisions in different states

That being said, getting CTools-style object/form caching/locking in core would be great too.

quicksketch’s picture

I personally would prefer the CTools approach because it would also solve the "Oops I left the page" problem. I'll never forget the time I was teaching a class with 8 computers plugged into a powerstrip that blew right in the middle of everyone configuring views. Everyone was immensely impressed that Views could pick right back up where they left off after the whole class just had their computers shut down on them. Using the temporary object store approach would also give us the locking and auto-saving possibilities that Views has had for years. Not to mention contrib could use it too.

Forward-saving revisions could also solve the preview problem, though the niceties and reusability of the object store make me lean towards that approach. From a usability perspective, I don't think "Preview" and "Revisions" is something that people will immediately be able to make a connection between. I wouldn't expect a preview to create a revision, since I haven't saved anything yet.

merlinofchaos’s picture

I'll note that Views has a bit of an advantage because it has a LOT of very small forms where content editing will be just one form. That said, auto-save draft with the object cache back-end would be trivial with an AJAX timer hook that fires every few seconds, checks to see if any gadget on the form has changed, and does an auto-update-draft in the background and prints a little update saying the last time the draft was saved.

Wordpress does this and it's by far one of their best content editing features.

jenlampton’s picture

I lile the idea of ctools in core, and I *love* the idea of fixing previews :) +1 for everyone working on this!!!

Dave Reid’s picture

I have a hard time understanding how the CTools cache and lock system actually gives us better previews. When I create a new View with a page display at '/my-new-view' I can't actually go preview how that view would work at mysite.com/my-new-view; I have to Save (essentially "Publish") the View before I can do that. The mockup in the original post shows interactions with menu links and blocks (aka how does this node look when actually on my site?), which I don't understand how that would work with cached form data and how the rest of the Drupal parts would be expected to work with that data when it doesn't "really" exist. I can see how it would work with a 'Save and preview' button saving an unpublished revision and actually letting you go to /node/1 to see how it interacts and fits.

Auto-saving and content locking would be great improvements for content editors to have but seem kinda unrelated to the actual issue of improved previews? I'm also not trying to say that either way is correct. I'm just trying to ensure we're asking a bunch of questions first about how it would fit together.

Dave Reid’s picture

Side note: I would imagine that if improved previews were powered by revisions, that most of the revision aspects would actually be hidden to the end users by default (only if revisions are "actually" enabled for a content type). For example, the 'Save and preview' button would save a draft, unpublished revision. Editing that node would continue editing that draft revision or be silently creating new revisions on each edit. Clicking the 'Save and publish' button would publish the current revision being edited. Using revisions uniformly across multiple entity types requires a *lot* of cleanup to revisions in core (very hard-coded to nodes), but it may be more worth it in the end by improving revisions and better tying it together in the UI.

merlinofchaos’s picture

I have a hard time understanding how the CTools cache and lock system actually gives us better previews. When I create a new View with a page display at '/my-new-view' I can't actually go preview how that view would work at mysite.com/my-new-view;

Revisions don't give you this either.

The mockup in the original post shows interactions with menu links and blocks (aka how does this node look when actually on my site?), which I don't understand how that would work with cached form data and how the rest of the Drupal parts would be expected to work with that data when it doesn't "really" exist.

It's not showing that. It's showing how node/XXX will work with the edited node instead of the original, with a widget that lets you pick the view mode. The blocks that will appear there will appear on the preview. Also, 'revisions that are not the current revision' don't really exist any more than cached form data, so your argument is actually a little off base, in that it's positioning one as wildly different than the other.

Auto-saving and content locking would be great improvements for content editors to have but seem kinda unrelated to the actual issue of improved previews?

And so are forward-revisions. If we're going to talk about strictly required features for the preview presented, then revisions shouldn't even be on the table. Why is my pet feature less valid than the pet feature you are in favor of?

merlinofchaos’s picture

Let me also add:

1) A LOT of fixing of revisions would have to happen in order to have forward revisions working. For example, revisions are very node specific. They would have to be genericized. You can currently only save the current revision -- field API would have to be updated to let you save arbitrary revisions. (Note: Both of these things should happen regardless.)
2) This would leave out any entity that does not support revisions (either because they do not want or do not need them).
3) I doubt the ability to make revisions invisible.
4) Failed editing could lead to a proliferation of revisions -- typically in this kind of workflow (i.e, Wordpress which is where some of the inspiration is from) you'll "change your mind" about revisions and cancel. But in some proposed systems, revisions are never deleted. They'd just hang around. Need to be careful about that.
5) No matter how much you want to make the UX easy, I don't think it would be. Once you start using revisions this way, it becomes intrinsic to their nature when they are administered.

EDIT: My bad, 3 and 5 are basically the same. I've had a little to drink. Sorry. :)

Dave Reid’s picture

I think I failed to express my primary concern: how does previewing work for *new* entities that do not yet have a node ID, have any of its data saved in the database, none of its menu links saved, etc? I totally understand how a CTools form cache system would work with existing entities and it makes sense to me. But when you hit the 'Preview' button on a new node, you can't go to node/1 - it does not exist.

merlinofchaos’s picture

Temporary data doesn't need an entity id. Typically I use a key that means "there is no id assigned to this data yet". In fact, this works better than revisions when there is no entity id; you cannot have a revision without an entity id, since the key is compound. The object cache has no such limitation.

Also, it's not a form cache. It's an object cache. You store a fully formed entity object in the cache. The form is merely what you use to transform it.

yoroy’s picture

Solid stuff gentlemen :) It seems both approaches would bring us (much) more than "just" fixing previews.

From what I gather, the CTools way brings us more directly to real previews whereas improved revisions sounds a bit more forced, not really directly tied to the previews use case but could-be-used to make it work.

Are these options mutually exclusive? Obviously we'd need only one way to fix previews, but that doesn't imply we don't want both better revisions *and* have the "niceties and reusability of the object store", right?

I hope this discussion can focus on what's best for getting good previews in core.

stevector’s picture

These options are definitely not mutually exclusive. From the arguments made above, a CTools-style object cache is the better candidate to improve previews and it's help with auto-saving would be a huge win too.

Improved revision handling should be done too but it does not need to impact core previewing.

So to move forward, where should CTools-cache-in-core start? There's a meta-issue tracking entity changes. #1346204: [meta] Drupal 8 Entity API improvements One of the main changes is converting entities to classed objects. Merlinofchaos, does that make caching any easier/harder/irrelevant? Or should this be done at a level deeper than entities so that the same system could cache entity objects or CMI objects?

merlinofchaos’s picture

I agree with stevector; I think both setups are extremely useful, and at the end of the day I think it'd be awesome to be able to put them together. Fixing revisions so you can use them for more than just history is valuable all on its own, and having temporary data so you can lock and better support multi-step content forms would be a bigger win now.

I don't think converting entities to classes matters too much. As I see it, the steps forward go like this:

  1. Analyze the object cache as it is now and see what improvements it needs for core. Since Core is transitioning to more OO, I was thinking it could be rewritten as an OO layer which would allow us to make use of PSR and streamline the API somewhat. (It could also hamper the API for OO unfriendly people. I want to discuss this one with Larry and a few others first.)
  2. Figure out how to implement that toolbar. There are some questions about it: What is it, really? IS it specific to preview? I suspect this is a secondary toolbar that is part of toolbar itself that allows us to put some resource specific 'things' in the toolbar, which include links and a form.
  3. Get a patch that gets that into core, with tests. The current implementation in CTools has tests. Now, we have the problem that firstphase doesn't actually use the system right away, but because it's generically useful I can only hope that catch thinks this is okay. If now we'll keep rolling into the next phase.
  4. Integrate entities to use the object cache for new and existing entities; update the preview path to something completely different. This is all one big patch probably. We have to figure out the actual path the preview will take. Is it node/XXX with a query string indicating preview mode? IS it a totally different path? And how do we handle a new node? That part will be particularly tough, though I believe we could put a special XXX key that says "not-yet-existing entity" and have entity_load() automatically handle it.

That's what I can think of offhand.

yoroy’s picture

Looks like a plan. Then point 1 is the next step for the code, point 2 is the next step for the design. I propose merlinofchaos & friends do the code discussion and ux team works on a more fleshed out design/prototype :)

Thanks.

Bojhan’s picture

Assigned: Unassigned » catch

@catch Could you give this approach a look?

webchick’s picture

Assigned: catch » Unassigned

I think both setups are extremely useful, and at the end of the day I think it'd be awesome to be able to put them together. Fixing revisions so you can use them for more than just history is valuable all on its own, and having temporary data so you can lock and better support multi-step content forms would be a bigger win now.

Yeah, this captures my feelings pretty well, too. I don't think this is an either/or situation. I think it's an "and" situation. "Future revisions" solves one ginormous class of problems, but in terms of both new entities, as well as the "Oops, my cat just stepped on the power strip" situation, the object cache seems like a good direction for solving the preview stuff.

Can we make sure there is (if there's not already) an issue about allowing forward revisions in core? Because that's also really important. #218755: Support revisions in different states is at 110 comments and has no issue summary so I can't quite tell if that's it or not.

Bojhan’s picture

Assigned: Unassigned » catch
webchick’s picture

Oops. Didn't mean to cross-post.

David_Rothstein’s picture

Is this issue a duplicate of #1167242: Expected a realistic preview of changes.?

A discussion has been going on there for a while, and from a design perspective it seems to have come to a similar conclusion as this one (full page preview with a backlink in the upper left, similar to the block region demo page)...

timmillwood’s picture

I have marked #1167242: Expected a realistic preview of changes. as a duplicate. Although it came first, this thread seems to have more direction.

@Bojhan. I like your mockup in #1, I'm not sure it needs the 'full node' select list, especially with the term 'node' in it. I also think it needs to be clearer that this is a preview. Imagine the use case that someone aims to click save, but clicks preview by mistake. They get this page which looks like their post is saved and looks good. So they close their browser and loose their work. For this use case I also think there should be a save button on the right of the preview bar.

Bojhan’s picture

@timmilwood Thanks for doing that! I totally agree that we should not use the word node. I have thought about adding a save button to the preview screen, but concluded that would change the purpose of this screen - from merely a preview, to something where the action also happens. I am not sure if that is good from a conceptual level (preview, is not just a preview), to interaction level (how do we handle errors upon submit, multiform submits, publishing state). When it comes to losing your work, ideally clicking preview makes an auto-save in cache/forward revision/something else.

webchick’s picture

Note that the following is totally out of scope for the initial patch, but.

We should probably anticipate if not core then at least contrib wanting to stick more options on the preview bar. For example, preview as a given role, preview in a given language, preview as a user from a given ip2geo location, etc. That one drop-down could quickly become 7-8 drop-downs, some checkboxes, a calendar widget, and a partridge in a pear tree. How do you anticipate the design dealing with that?

yoroy’s picture

We don't since we have to protect developers, builders and end users from getting calendars and pear trees in there :)

yched’s picture

At least a "theme" selector would be needed for multi-themed sites

Bojhan’s picture

I imagine that modules that want to add settings like "per role displays" can put this in the blue bar. From a visual standpoint a lot more can be placed in it, if they make sure to apply adequate spacing. Obviously this only scales to a few items, more complex preview methods (like pear trees) could potentially use a modal dialog, where they only expose a button/link in the top bar.

@yched Yes.

jide’s picture

My 2 cents : Have we considered using an iFrame to preview content directly from the node form ?

catch’s picture

With revisions, in the back of my mind I'd been considering the possibility of google docs-style autosave with revisions (so you can get a list of changes and roll back to specific ones either during editing or after), so I'm not sure it's poorly suited to previews necessarily except for the content ID issue (at the very least it'd be necessary to reserve an ID when preview is clicked). However as merlinofchaos says there's a tonne of work necessary to get close to doing that, and no-one's working on it at the moment. #1082292: Clean up/refactor revisions is an issue just for fixing some obviously broken things with revisions without adding any new features, I haven't had time to get to it, but it's a lot more focused than the 'different states' issue.

I haven't reviewed the ctools object cache code (or at least not for a long time), but in principle I'm not opposed to trying to get that into core at all - if we end up improving revisions for entities, there's still potentially lots of other forms that could benefit from it that aren't entity based at all.

Bojhan’s picture

Assigned: catch » Unassigned
Bojhan’s picture

Issue summary: View changes

meh

yoroy’s picture

Thanks catch. As for #1082292: Clean up/refactor revisions, looks like real progess is being made in #218755: Support revisions in different states

I had a first stab at an issue summary, using merlinofchaos's list in #16.

klonos’s picture

Is CTools object cache flashed when all caches are cleared? Would this flush any auto-saved data too if they were based on that?

merlinofchaos’s picture

No, they are not. They flush stale data after a set amount of time but they are non-volatile and are not cleared with normal caches.

It's not truly a cache. When I rewrite it for core inclusion I think I will call it a TempStore instead.

Bojhan’s picture

Status: Active » Needs work

Looks like this needs a starter patch then!

klonos’s picture

Status: Needs work » Active

Thanx for taking the time to explain this Earl. The reason I asked was because I could grasp (roughly) how this would be done with revisions, but not the CTools way.

Anyways, as long as we get this feature in D8, I don't really care how it's done. I guess whichever way is easier/faster for the people that'll code it + maintain it afterwards. I agree with others though that revisions should receive a major overhaul anyways, but that's a matter to be discussed in #218755: Support revisions in different states, #1082292: Clean up/refactor revisions and elsewhere - not here. Same goes for thoughts about implementing auto-saves, and supporting revisions for all entities too. They too deserve their own separate issues.

klonos’s picture

Status: Active » Needs work

...sorry.

Dave Reid’s picture

Status: Needs work » Active

There is no patch here, so hence we set the status to active.

quicksketch’s picture

It's not truly a cache. When I rewrite it for core inclusion I think I will call it a TempStore instead.

Which I believe we should also use instead of the for the extremely abused "cache_form" table, which isn't really a cache either and also shouldn't be flushed with cache clears.

klonos’s picture

Any issues filed so we can keep track of these tasks then?

moshe weitzman’s picture

Crickets ...

Dave Reid’s picture

crick·et
/ˈkrikit/
noun

  1. An insect (family Gryllidae) related to the grasshoppers. The male produces a characteristic rhythmical chirping sound.
  2. An outdoor game played on a large grass field with ball, bats, and two wickets, between teams of eleven players, the object of the game...
  3. A low stool, typically with a rectangular or oval seat and four legs splayed out.
merlinofchaos’s picture

I've gotten a start on the TempStore object. I haven't created an additional issue (perhaps there should be one) and I haven't had a chance to finish it. There's a lot tugging on my time right now, but it's important to me for many reasons.

merlinofchaos’s picture

tim.plunkett’s picture

Category: task » feature

TempStore is in, this should use that!

yoroy’s picture

Hah, cool. I was just about to bump this issue for a status update. Good news!

I guess people might want to know where to find the latest code for this TempStore and any pointers on how to apply it here would be welcome too. People will want to read comment #16 and #30.

tim.plunkett’s picture

Here's the change notice with some examples: http://drupal.org/node/1805940

Bojhan’s picture

Category: feature » task
tim.plunkett’s picture

Can you explain why this is more than a feature request?
Also, the issue summary is fairly out of date.

xjm’s picture

I believe it's categorized a task because it causes pretty drastic usability issues, as seen in the Google UX study earlier this year. However, it's clearly a new feature, so this is sort of a gray area.

Bojhan’s picture

Usability problems can hardly be categorized as feature requests, sadly that has been the trend of the past few weeks. We slowly moved from bug -> task -> and now feature request is the new trend. However as far as I am concerned this is a serious issue that affects most people using this functionality, that we need to resolve before final release.

I can't update the issue summary, it seems like it needs to have a new technical approach.

Bojhan’s picture

Issue summary: View changes

first stab at issue summary

xjm’s picture

I updated the summary a bit to recommend recent core API additions.

xjm’s picture

Some considerations:

  • Hook invocations. Since the entity isn't saved yet, hook_entity_insert() or hook_entity_update() hasn't fired. Which is by design I think, but might affect how actual the actual content preview is.
  • Access. The aforementioned unfired hook invocations might change the entity's access restrictions. So we probably want to restrict access to the preview to the owner, or perhaps administer content for nodes.
goyb’s picture

Would it also be possible to make a preview for different devices i.e mobile, ipad etc?

swentel’s picture

Status: Active » Needs review
FileSize
69.21 KB
8.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1510544-55.patch. Unable to apply patch. See the log in the details link for more information. View

First stab at this, needs a lot of work, but could use a lot of reviews/tips on how to implement this cleanly. Still lots of things to cover like images, tags (and more stuff along the road I guess).

Note that I won't dedicate full time on this, so feel free to hack away at this at all times, assign yourself or ping me on IRC in case so we don't do double work :)

Attached a screenshot of how it currently looks like - the link to go back is at the top left.

swentel’s picture

Note, the uuid is available, will use that as the tempstore_id in the next patch

swentel’s picture

FileSize
11.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1510544-57.patch. Unable to apply patch. See the log in the details link for more information. View
63.63 KB

New patch: uses uuid, you can now switch view mode as well, css needs love :)

moshe weitzman’s picture

I doubt that calling drupal_render() is the way to go here. In general we want to return a render array and let drupal_render_page() do its thing.

Also noticed a variable thats not used: $trimmed = drupal_render($elements);.

Thanks!

Wim Leers’s picture

Great progress! Interdiffs would be useful though :)

swentel’s picture

FileSize
77.89 KB
14.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1510544-60.patch. Unable to apply patch. See the log in the details link for more information. View

Page callback now returns render array, $trimmed variable is gone, css been updated (copied action-button from 7 basically).
Sorry for not posting interdiffs so far, probably after this one as it becomes more mature.
Oh and Wim, I need a way to tell the edit module to not fire on the preview page, see the temporarily extreme hack in the patch right now ;)

That's all for today, more this weekend.

swentel’s picture

FileSize
13.99 KB
FAILED: [[SimpleTest]]: [MySQL] 50,322 pass(es), 71 fail(s), and 6 exception(s). View

Quick re-roll after #1838114: Change node form’s vertical tabs into details elements went in. Now *really* off.

swentel’s picture

FileSize
13.89 KB
FAILED: [[SimpleTest]]: [MySQL] 50,402 pass(es), 71 fail(s), and 6 exception(s). View

Last one - cleaner check for edit module by setting $entity->in_preview to true in preview callback and checking on that in edit_preprocess_field()

Wim Leers’s picture

#60: Something like this should work:

hook_toolbar_alter(&$items) {
  if (isset($items['edit']) {
    unset($items['edit']);
  }
}
aspilicious’s picture

Status: Needs work » Needs review
FileSize
19.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node_preview_65.patch. Unable to apply patch. See the log in the details link for more information. View

This version fixes the newline in the css file and makes tags work again when reloading the content

// @todo can we do this somewhere different ?
$tempstore_id = drupal_container()->get('request')->query->get('tempstore_id');
if (!empty($tempstore_id) && ($data = node_tempstore_load($tempstore_id))) {
$node = $data;

+ // We have to prepare the node to fix the autocomplete tags widget.
+ // @todo now we do a double prepare... Is that allowed? Probably not...
+ node_invoke($node, 'prepare');
+ module_invoke_all('node_prepare', $node);
}

aspilicious’s picture

Patch is crap. Will reroll...

aspilicious’s picture

FileSize
14.21 KB
FAILED: [[SimpleTest]]: [MySQL] 50,372 pass(es), 87 fail(s), and 40 exception(s). View

Second try....

aspilicious’s picture

There is also a problem with query redirects. When we have a redirect in the query we should ignore it when pressing the preview button. We don't ignore it at the moment :).

You can test this by editing a node through contextual links. After doing that you get on the node edit form. But when you press preview you get redirected back to the node page.

PS: great... ignore the dpm for now...
PS2: suddenly tags are broken again :s something weird is going on

aspilicious’s picture

Ok some more digging:

My patches above are incorrect they just seemed to work because I was dealing with existing terms.

Problem:
When defining new terms in an autocomplete, they are not yet saved, they get a tid "autocreate" when going to the preview.
When going back to the node form all the "autocreate" tags can't be loaded so they don't get rendered.

This feels like a general issue with references that don't exist. And probably needs a general solution.
I have no idea how the autocomplete system works but this seems to me the only situation where this could happen.
If this is a central system it would be cool if we could fix it there.

But I fear this needs a unique solution for each entity that is facing this problem.

aspilicious’s picture

FileSize
16.63 KB
FAILED: [[SimpleTest]]: [MySQL] 50,370 pass(es), 71 fail(s), and 6 exception(s). View

Dirty workaround for now. :)

swentel’s picture

FileSize
22.32 KB
FAILED: [[SimpleTest]]: [MySQL] 50,476 pass(es), 58 fail(s), and 5 exception(s). View

This patch should turn back green, removed some todo's as they are actually ok, did some fixes for node preview titles etc. I think after this, it's time for interdiffs. I've asked a colleague frontender to look at the css and the javascript this weekend.

- edit - note that I don't necessarily consider the code for taxonomy dirty, it's just the way that its autcomplete feature works, however, optimization is always possible of course .. :)

Wim Leers’s picture

Nice progress — the code already looks much better!

Bojhan’s picture

Just wondering, where is that font coming from looks like its not Seven/toolbar but Bartik?

aspilicious’s picture

+++ b/core/modules/node/node.pages.incundefined
@@ -121,11 +121,15 @@ function node_add($node_type) {
+  // @todo move access check earlier ?
   if (node_access('create', $node) || node_access('update', $node)) {

We can move the access check to the hook_menu (in stead of returning true). The doc has to be changes as we return a render array now. And not pure html.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/field/widget/TaxonomyAutocompleteWidget.phpundefined
@@ -52,7 +52,13 @@ public function formElement(array $items, $delta, array $element, $langcode, arr
+      $tid = $item['tid'];
+      if ($tid == 'autocreate') {
+        $tags[] = $item;
+      }
+      else {
+        $tags[$item['tid']] = isset($item['taxonomy_term']) ? $item['taxonomy_term'] : taxonomy_term_load($item['tid']);

I should have used $tid everywhere here

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -950,8 +950,17 @@ function _taxonomy_get_tid_from_term(Term $term) {
+    if (is_array($tag) && $tag['tid'] == 'autocreate' && $label = $tag['name']) {
+      // Commas and quotes in tag names are special cases, so encode 'em.
+      if (strpos($label, ',') !== FALSE || strpos($label, '"') !== FALSE) {
+        $typed_tags[] = '"' . str_replace('"', '""', $label) . '"';
+      }
+      else {
+        $typed_tags[] = $label;
+      }

I considered this dirty because it is duplicate code. We can refactor this to be a bit cleaner.

swentel’s picture

FileSize
8.52 KB
23.86 KB
FAILED: [[SimpleTest]]: [MySQL] 50,565 pass(es), 1 fail(s), and 0 exception(s). View

Used an access check now, changed doc block and changed the tests because of the way that the testbot runs (in a subdirectory).

I haven't touched the term workaround yet and I think we need to think about one important thing and that's how we're going to deal with the tempstore when anonymous users have access to edit nodes, because then $node->uuid won't be enough.

swentel’s picture

Status: Needs work » Needs review
swentel’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -26,6 +26,16 @@ class NodeFormController extends EntityFormController {
+    // Get the tempstore id.
+    $tempstore_id = drupal_container()->get('request')->query->get('tempstore_id');
+    if ($tempstore_id) {
+      $node->tempstore_id = $tempstore_id;
+    }
+    else {
+      $node->tempstore_id = $node->uuid;

We can probably just remove the tempstore_id property completely and use the uuid. So replace all tempstore_id everywhere with uuid.

+++ b/core/modules/node/node.moduleundefined
@@ -2696,6 +2808,24 @@ function node_node_access($node, $op, $account) {
+  if (node_access('create', $node) || node_access('update', $node)) {
+    return TRUE;
+  }
+  return;

return FALSE instead of 'return;'

- edit - we also need a test for switching the view mode on the preview page and maybe also a 'clickLink' on the button at the top.

Moshe also suggested to use a lock mechanism like views does, so we can finally change the mechanism where you'd potentially get a 'This content has been changed by another user' when trying to save a node. Basically create the tempstore object immediately after creating on node/add or node/x/edit and then checking the expire time we can add a message like 'This node is being edited etc'. This would solve the problem even for anonymous users.

- edit 2 - after actually checking the user.tempstore class the user with aspilicious this should actually be *really* easy and probably working fine for everyone :)

However, I'd leave that for a follow up, although I don't think it would be *that* much code.

swentel’s picture

Status: Needs work » Needs review
FileSize
6.06 KB
23.5 KB
PASSED: [[SimpleTest]]: [MySQL] 50,617 pass(es). View

Some cleanups, added tests for view mode switching and simplified some tests by using clickLink() method.
I was wrong re: removing the tempstore_id property, we actually do need it.
Not sure also why Drupal\system\Tests\Form\RebuildTest failed, works perfect on my local machine.

swentel’s picture

@Bojhan - The font comes from Bartik because we are in that theme.
Also, do you have an idea how we would handle locking nodes ? The idea is that when a user tries to edit a node which is edited by someone else would get a warning that this one is locked and can potentially (with permission) can break that lock, so just like views. We can do that in a follow up if/when (crossing fingers) this goes in.

Bojhan’s picture

@swentel Could you explain to me how the locking nodes occurs here? How is this different from what we do now?

tim.plunkett’s picture

The locking of nodes isn't actually even in the OP, though it could be built on this.

This is just about a true preview, not just "your words maybe run through a text format".

aspilicious’s picture

FileSize
4.75 KB
24.7 KB
PASSED: [[SimpleTest]]: [MySQL] 50,878 pass(es). View

Node destination redirects are working again. Not sure if this is the best we can do.
Cleaned my taxonomy code a bit.

swentel’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -86,6 +86,14 @@ public function form(array $form, array &$form_state, EntityInterface $node) {
+      // Restore the destination if we previewed the node
+      if (isset($node->destination)) {
+        $form['node_destination'] = array(
+          '#type' => 'hidden',
+          '#value' => $node->destination,
+        );

Missing dot in the comment. Let's use #type 'value' instead of hidden.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -391,6 +399,17 @@ public function submit(array $form, array &$form_state) {
+    // In case the destination parameter is set

Missing dot

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -950,7 +950,18 @@ function _taxonomy_get_tid_from_term(Term $term) {
+      // Make sure we have a completed loaded taxonomy term.

complete ?

- note - we'll upload a new version with css and js changes as well

sannejn’s picture

FileSize
27.72 KB
PASSED: [[SimpleTest]]: [MySQL] 50,709 pass(es). View
6.28 KB
102.43 KB

Updated some css to make the preview look like the provided design. Also updated the node.preview.js to disable all links except for the 'back to content editing'. Swentel addressed soms issues mentioned in #87.

actual-preview.png

meba’s picture

I would be concerned that actually disabling links will be a confusion for users. Links that are disabled need to be visually identified, can the whole page except preview and the button be greyed out for example? Or JS warning attached to clicking somewhere else?

Wim Leers’s picture

You can add an invisible overlay. For inspiration on how that could work, see core/modules/edit/js/views/overlay-view.js (if memory serves me well), along with the corresponding CSS. Instead of making the CSS white out everything, just make it transparent instead and ensure the z-index is lower than that of both the toolbar and your "back to content editing and view mode" bar. Now you can very simply intercept every click event; but at the cost of no longer having :hover events on the previewed content.
I'm not sure if that's the best approach, but I think it's a step in the right direction, at least for now.

I'd vote against a greying out or anything else, because that would prevent the preview from being a proper preview.

aspilicious’s picture

I'm confused why add a new layer if the patch already have a way to prevent clicks to work.

meba’s picture

My comment wasn't specific to the disabling clicks. My concern is that disabling clicks will be a UX disaster unless we give users some visual indication that they can only click the Back button (either before they click or after they click)

Bojhan’s picture

@meba I dont know, this could be something we split of from this issue as a followup. As its ideally something we explore a few options on, for several reasons;

  1. Some users might want to check links to see if they work
  2. Not being able to click links is a work around for not having auto-save ability, which with temp store and other tricks we can now actually do
  3. Preview is about being an actual preview, not a different one - if we make it visually different by disabling links it will create a different experience (possibly requiring explanation) - most likely even harder on custom themes - requiring "disabled" looks for all different kind of links.

However these are all somewhat assumptions, because in our current preview screen - we hardly see users click the links. Its just not a common use case, however this is a completely new model - so it might become a more prominent use case. With all this in mind, I would suggest a good follow up issue.

aspilicious’s picture

2) If you click away from the screen you can't go back to edit the node. Unless you know the UUID...

It's a bigger WTF to edit a complete node, go to the preview and loose everything because you clicked a link.

Wim Leers’s picture

#91: Apologies — I didn't know that was already implemented. I was just providing a suggestions as per #90.

#93.1: we could make it so that only links inside the previewed node are clickable. Or potentially add a Google Docs-like tooltip when clicking the link.
#93.2: It doesn't make any sense to click links other than those inside the previewed node's contents when previewing a node. If you disagree, please explain why. So: I disagree it's a work-around for not having an autosaving capability, IMVHO it's common sense.
#93.3: I wasn't arguing for disabling links in the sense that they would no longer be <a> tags, or would get some class attribute. Nothing should look differently. But a user should not be able to start navigating elsewhere, and if he should, then he should explicitly confirm that first.
#93.conclusion: I agree, this the perfect example of a follow-up-able issue.

#94: It's not very clear to me what you mean, but I presume it's a reply to #93.2? Because of the temp store, they wouldn't lose everything, but you'd lose your context, so in that sense I agree :)

Bojhan’s picture

@aspilicious Totally, I think you misunderstood me. I totally think we should do our current implementation of not allowing to open links in preview. I just don't think its a good idea to visually make them different, until we have covered the considerations I noted above.

soulfroys’s picture

Just open a parenthesis on the feature raised by @merlinofchaos about content locking in #3. I use Drupal 6 on an intranet with 1800 users based on Organic Groups and it would be impossible to implement it without content locking.

You probably already know, but worth mentioning, the Content Locking module do it, and do it very well. It is absolutely perfect for our needs and it has the entire workflow cited by @merlinofchaos. It's a great reference.

Autosave is also very welcome and necessary, but it is essential to lock content in a collaborative environment.

(Sorry for my english)

swentel’s picture

FileSize
34.53 KB
825 bytes
27.8 KB
PASSED: [[SimpleTest]]: [MySQL] 50,801 pass(es). View

Small update where the font is used from the toolbar, as requested by Bojhan.

I'd say, RTBC on 99, commit on 100 ? :) (and of course follow ups after that)

Screen Shot 2013-01-23 at 10.24.02.png

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, I tried an earlier patch. There is probably some follow up work, e.g. making the select list nicer, disabled links. But from my point of view this is a really good initial go, that seems to achieve all that we set out to do.

swentel’s picture

FileSize
738 bytes
27.7 KB
PASSED: [[SimpleTest]]: [MySQL] 50,734 pass(es). View

Remove some redundant code, thanks aspilicious.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

Lots of feedback. Boatloads of nitpicks, but also a few big questions, amongst one which is a very fundamental concern — also unanswered by the rest of this issue AFAICT.

+++ b/core/modules/edit/edit.moduleundefined
@@ -164,7 +164,9 @@ function edit_library_info() {
+  if (empty($entity->in_preview)) {

Good catch :)

+++ b/core/modules/node/images/btn-arrow-preview.pngundefined
@@ -0,0 +1,5 @@
+<rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"> <rdf:Description rdf:about="" xmlns:xmpRights="http://ns.adobe.com/xap/1.0/rights/" xmlns:xmpMM="http://ns.adobe.com/xap/1.0/mm/" xmlns:stRef="http://ns.adobe.com/xap/1.0/sType/ResourceRef#" xmlns:xmp="http://ns.adobe.com/xap/1.0/" xmpRights:Marked="False" xmpMM:OriginalDocumentID="uuid:2C69DF84A6C0DF1182FCEF8E87685653" xmpMM:DocumentID="xmp.did:9834D3895B1F11E29836AA070337CF68" xmpMM:InstanceID="xmp.iid:9834D3885B1F11E29836AA070337CF68" xmp:CreatorTool="Adobe Photoshop CS5.1 Windows"> <xmpMM:DerivedFrom stRef:instanceID="xmp.iid:5698AC1B3A7BE11187FEA4A0A56EE5FD" stRef:documentID="uuid:2C69DF84A6C0DF1182FCEF8E87685653"/> </rdf:Description> </rdf:RDF> </x:xmpmeta> <?xpacket end="r"?>

All this metadata should be stripped from the PNG file, but this can be a follow-up commit. UPDATE: considering the loads of other feedback, you may want to fix this now. Optimize it using http://imageoptim.com/ (i.e. which uses PNGOUT, AdvPNG, Pngcrush, extended OptiPNG).

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -26,6 +26,16 @@ class NodeFormController extends EntityFormController {

@@ -26,6 +26,16 @@ class NodeFormController extends EntityFormController {
    * Overrides Drupal\Core\Entity\EntityFormController::prepareEntity().
    */
   protected function prepareEntity(EntityInterface $node) {
+
+    // Check if we can retrieve a node from the tempstore.
+    $tempstore_id = drupal_container()->get('request')->query->get('tempstore_id');
+    if ($tempstore_id) {
+      $node->tempstore_id = $tempstore_id;
+    }
+    else {
+      $node->tempstore_id = $node->uuid;

Does this mean we only support node entities? Is there a reason we can't do this for all entities generically?

Or is that a follow-up issue?

I saw some discussion about this in comments #9 and #11, but it's not explained in the issue summary. So can we please clarify this?

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.phpundefined
@@ -381,9 +397,22 @@ public function submit(array $form, array &$form_state) {
+    if (isset($_GET['destination'])) {
+      $entity->destination = $_GET['destination'];
+      unset($_GET['destination']);

Is it still allowed to manipulate $_GET directly? Shouldn't we be using drupal_container()->get('request') to modify this?

+++ b/core/modules/node/node.moduleundefined
@@ -943,6 +939,23 @@ function node_load($nid = NULL, $reset = FALSE) {
+ * Load the node from the tempstore.

the -> a ?

+++ b/core/modules/node/node.moduleundefined
@@ -943,6 +939,23 @@ function node_load($nid = NULL, $reset = FALSE) {
+ *   The id of a tempstore object.

s/id/ID/, no?

+++ b/core/modules/node/node.moduleundefined
@@ -943,6 +939,23 @@ function node_load($nid = NULL, $reset = FALSE) {
+ * @return Drupal\node\Node|false

Leading slash missing.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+

Extraneous newline.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+  $node = menu_get_object('node_tempstore', 2);

Should we still use menu_get_object()?

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+

Another extraneous newline.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+ * @param Drupal\node\Node $node

Missing leading backslash.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+ * @return $form

array $form

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+function node_preview_form_select($form, $form_state, Node $node) {

array $form, array $form_state

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+  // Always add default.

"add default"? Sounds strange.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+  $link = 'node/add/' . $node->bundle();

I understand why you call this variable "link", but wouldn't "path" be more in line with the rest of Drupal?

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+    $view_mode = 'full';

Maybe $current_view_mode instead? That makes the default value setting for the select less weird, twenty lines further.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+    //'#prefix' => '<div class="node-preview-backlink-container">',
+    //'#suffix' => '</div>'

Should be removed.

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+ * Submit handler for the node preview select form.

s/node preview select form/node preview view mode select(ion) form/

+++ b/core/modules/node/node.moduleundefined
@@ -2256,6 +2276,98 @@ function node_page_view(Node $node) {
+function node_preview_form_select_submit($form, &$form_state) {

array $form, array &$form_state

+++ b/core/modules/node/node.moduleundefined
@@ -2696,6 +2808,24 @@ function node_node_access($node, $op, $account) {
+ * @return

"@return bool" ?

+++ b/core/modules/node/node.pages.incundefined
@@ -119,79 +119,50 @@ function node_add($node_type) {
+  // Set status to true so we don't get the 'unpublished' css.

s/css/CSS/

+++ b/core/modules/node/node.pages.incundefined
@@ -119,79 +119,50 @@ function node_add($node_type) {
+  // Load the user's name when needed.

s/user/author/ ?

+++ b/core/modules/node/node.pages.incundefined
@@ -119,79 +119,50 @@ function node_add($node_type) {
+      $node->uid = 0; // anonymous user

This comment is unnecessary, especially because it's already explained in a comment a few lines higher.

+++ b/core/modules/node/node.pages.incundefined
@@ -119,79 +119,50 @@ function node_add($node_type) {
+  // Property so we can manipulate $page in template_preprocess_node.

Bad sentence, and it doesn't seem to match what the code is doing?

+++ b/core/modules/node/node.preview.cssundefined
@@ -0,0 +1,70 @@
+  position: fixed;
...
+

Extraneous newline.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -950,17 +950,24 @@ function _taxonomy_get_tid_from_term(Term $term) {
 function taxonomy_implode_tags($tags, $vid = NULL) {

Why are changes to this function necessary?

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -950,17 +950,24 @@ function _taxonomy_get_tid_from_term(Term $term) {
+      // Make sure we have a complete loaded taxonomy term.

s/complete/completely/

aspilicious’s picture

Is it still allowed to manipulate $_GET directly? Shouldn't we be using drupal_container()->get('request') to modify this?

You're right we should manipulate the object.

webchick’s picture

Additionally, we need to check the accessibility (and usability, for that matter) of just having a dropdown in the middle of a blue bar that says "Full". Methinks that should have a label of some kind. The feature in general needs to be tested as well to ensure it passes the accessibility gate before it can be committed. Hopefully since this is just largely re-using stuff already in Views there won't be a problem here.

This doesn't need to be a commit blocker, but one thing we should think about is making this preview bar something other modules can easily hook into, so say for example the http://drupal.org/project/ad_geoip module could preview how the node will look if the user comes from Spain vs. Canada, and http://drupal.org/project/browscap could let you preview it on such and such browser, and so on. Right now, implementation-wise, this is highly tied to nodes, and node is an optional module. It would be cool if node was simply implementing hook_preview_bar_alter() (or whatever) instead.

swentel’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
26.98 KB
FAILED: [[SimpleTest]]: [MySQL] 50,757 pass(es), 1 fail(s), and 0 exception(s). View

Thanks for the review! :)

  • Image has been optimized
  • I'm not sure if there's an equivalent for menu_get_object() yet - there are other places in node.module still using it, so leaving as is for now.
  • This patch currently only supports nodes indeed. I've been thinking about generalizing this, but let's start here first. I'm willing to investigate this in a follow up issue.
  • The code in taxonomy is needed because the format can be different, depending on whether the tag already exists or not. In case it doesn't exist yet, the format on the $entity is not good to be added in the form field.

I've tried removing the 'destination' get parameter with $request->query->remove('destination'); but that didn't seem to stop Drupal from redirecting, so still using unset($_GET['destination']) as drupal_goto() looks directly for $_GET['destination'].

All other things should be addressed and are in the interdiff. Please recheck those extraneous newlines, because it's not always clear for me to find out, but I *think* it's ok.

Re: webchicks remarks: yeah, the hook_page_build() doesn't 'feel' right, it would be more awesome to have more flexibility there and also the ability for other modules to alter the node before preview as well.

swentel’s picture

Status: Needs work » Needs review
FileSize
26.36 KB
FAILED: [[SimpleTest]]: [MySQL] 49,263 pass(es), 4 fail(s), and 16 exception(s). View

Reroll after poll is gone and some entity info related changes.

swentel’s picture

Status: Needs work » Needs review
FileSize
656 bytes
26.35 KB
PASSED: [[SimpleTest]]: [MySQL] 49,396 pass(es). View

And now fixing good this time, API changes after #1822458: Move dynamic parts (view modes, bundles) out of entity_info()

xjm’s picture

Issue tags: +Novice

Novice task for this issue: test the patch against the core accessibility gate. Four parts of the accessibility gate are straightforward to test:

  1. HTML structure meets WCAG 2.0
  2. Text color has sufficient contrast
  3. Form fields are labeled
  4. Javascript is keyboard-usable

Choose one (or more) of these points, apply the patch and clear your caches, and follow the instructions provided for each in the accessibility gate information. Post a comment describing your testing and the results here on the issue. Thanks in advance!

xjm’s picture

Issue tags: +Needs manual testing
mgifford’s picture

@xjm - There's no label on the view_mode form to switch between full & teaser modes.

Took a brief look at the color contrast. With white text & a gradient between #4F9FEA & #4481DC for the background, unfortunately it fails for all but large text (and this isn't large text) - http://webaim.org/resources/contrastchecker/

Other than that it looks fine. Keyboard navigation was fine & http://wave.webaim.org/toolbar didn't complain about anything else.

Will be a good addition when we get those two things cleaned up.

swentel’s picture

@mgifford

- is adding a 'label', with #label_display = 'hidden' enough, or does it have to be visual ?
- I'm absolutely not sure how to handle that color/contrast thing, never done that honestly, so if you could tell me what todo, that would be fine :)

Also, feel free to reroll yourself of course :)

Bojhan’s picture

@mgifford Is that white on blue related to this patch, seems like an overall issue with that button then.

larowlan’s picture

FileSize
26.45 KB
PASSED: [[SimpleTest]]: [MySQL] 49,291 pass(es). View
1.93 KB

So this uses #title_display => 'invisible' to add the label for screen readers.
Changed the css to use the same gradient as the action buttons in seven.
The top of this gradient fails the contrast, but the bottom passes.
At least this way if we decide that we need to modify these, we need to modify all of them - and we can search for the rgb.
Also fixed the fall-back background for the preview bar for browsers with no gradient support (was the same color as the button) and removed the underline from the :hover pseudo-class which made the button look like a link (it doesn't do this in seven).

The last submitted patch, node-preview-1510544.114.patch, failed testing.

swentel’s picture

#114: node-preview-1510544.114.patch queued for re-testing.

meba’s picture

Status: Needs review » Needs work

If I enable comments on the node and go to preview, I see no comment form. If I then go back to editing the comments are back disabled.
Also menu options (and it seems other options).

I am also not sure why I see strange flickering once I click on Preview - the site first opens the page in Overlay and then switches off the overlay.

larowlan’s picture

The overlay flickr is because the preview page callback is not an admin path, so it does not use the overlay. Users without permission to use the admin theme won't see that flickr.
The preview does not belong in the overlay in my opinion, the intent is to use the actual theme, not the admin theme.

swentel’s picture

The preview does not belong in the overlay in my opinion, the intent is to use the actual theme, not the admin theme.

True, I guess this is the same behavior of the block demonstration page.

I don't think we'll able to get the menu's working easily (although theoretically possible I guess), comments will be a bit easier, however, dataloss when going back is something we'll have to look at.

larowlan’s picture

Comments are slated to become an ordinary field in #731724: Convert comment settings into a field to make them work with CMI and non-node entities so that problem should just go away.
Ideally once the entity reference field lands #1801304: Add Entity reference field and menu links become content entities #916388: Convert menu links into entities theoretically (and ideally) node menu links would become and entity reference field too.

yoroy’s picture

Swentel showed me this today. It already works so naturally, it's basically not exciting anymore! Which is a good thing, because that means it does what is expected.

I think the view mode label should be visible.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
28.12 KB
PASSED: [[SimpleTest]]: [MySQL] 49,324 pass(es). View

This fixes most of the data loss, except for the menu handling.
Haven't been able to get the comment form rendered though.

I'm pretty much offline until monday, so someone else can take over to bring this home.

jibran’s picture

Awesome work.
I just have one suggestion. Can we have save button next to 'Back to content editing' link? So that if I have done editing I can save content immediately. Maybe we can do it in followup if it is out of scope.

moshe weitzman’s picture

Good idea. It would be a Save dropbutton as per #1751606: Move published status checkbox next to "Save". Could be a follow-up.

Bojhan’s picture

Lets do that as a followup, its a little bit of a can of worms - because you might want to include revision data as part of this - what is "saving"? etc.

webchick’s picture

Spent some time with this patch this afternoon. It is COMPLETELY AMAZING! I literally laughed out loud when I went back to Drupal 7 and previewed my content. The difference is stunningly remarkable. Screw web services and configuration management, this is the one feature everyone's going to want to upgrade to Drupal 8 for. ;)

The one dark mark on its eye is that #1848210: [Tests] Submitting a form in Overlay shows content + dsm() for a split second before redirecting to front-end theme gives it a really jarring/ugly experience in the Overlay. However, that's a pre-existing bug that pre-dates this patch, and without the Overlay it seems to work great.

I think the last commit-blocker from my side is I feel like we need a label on the view mode selector. I'm not sure what to call it that's not overly technical, but "Full" floating out in space doesn't really make any sense. The existing label "Select a view mode" might be too technical though.

Agreed with jibran that I found my natural inclination while testing this was to want to be able to save right when I'm looking at the page. However, also agreed with Bojhan that it's likely better as a follow-up in case we run into issues. I'd also love a follow-up to discuss how other modules can add their preview doo-dads to this bar. We're working on a mobile preview doo-dad at #1880606: Introduce a configuration UI for theme-based breakpoints and it'd be great to have somewhere to put it.

geerlingguy’s picture

For the label, how about just "Mode:" or "View mode:" — doesn't seem too technical for a content editor, who should at least understand there are different ways a bit of content will be displayed.

Bojhan’s picture

@geerlingguy Then it should be Display mode :)

jibran’s picture

swentel’s picture

Assigned: Unassigned » sun
+++ b/core/modules/node/node.pages.inc
--- /dev/null
+++ b/core/modules/node/node.preview.css

We need some eyeballs on node.preview.css in this patch from a frontend/markup perspective.

Made the exercise on where CSS should be and I'm guessing this the wrong approach. So what probably should happen is that:

  • there is is some minimal styling in node.preview.css
  • the actual colors etc (maybe more ?) go into bartik somewhere ?

Assigning to sun for review - look I'm learning ;)

Also, in the proposed resolution, there's a mention about moving some things to EntityFormController::prepareEntity(), which would probably make sense - and would make it easier for a generic entity approach.

sun’s picture

Title: Actual preview of content » Allow to preview content in an actual live environment
Component: base system » entity system
Assigned: sun » Unassigned
Category: task » feature
Status: Needs review » Needs work
Issue tags: +Needs themer review

Hah. Thanks for looping me in. ;)

Interesting issue. To my knowledge, one of the top-notch Drupal agencies scoped a feature like this to require a mere ~$500,000 budget for being developed. Of course, that included to take care of the utter pitfalls and consequences a functionality like this would entail.

The changes to Menu module in this patch pretty much demonstrate the problem space: As long as you assume that you're on your own and on your own isle, you can do whatever you want. But as soon as you take multidimensional modularity into account, any kind of simple idea blows up completely and leaves the overall system in a state we definitely do not want to be in. In turn, that gets you pretty quickly to the aforementioned budget.

This patch does not address that. It ignores the problem space of entities getting enhanced by modules, which, each, have their own storage, and which need to participate in the rendering process, but whereas the rendering process relies on the previously mentioned storage. Worse, the submitted, entity-specific form values can have a significant effect on other parts of the page/site, outside of the rendered entity.

Due to that, it is only a showcase of a hypothetical feature. The feature cannot be implemented in the proposed way, as it would have massive implications on contributed modules down the line.

However, I do think that it would be very beneficial and worthwhile to actually leverage the most recent improvements to the entity system, in order to make a relatively big leap in the right direction for Drupal 8. Details are following below.

So, alas, most of this is not the front-end related review that has been asked for, but instead, a much more significant, architectural review:

+++ b/core/modules/menu/menu.module
@@ -566,6 +566,13 @@ function menu_form_node_form_alter(&$form, $form_state) {
+  // In case we're in preview and there is no mlid yet but a title
+  // has been supplied, trick the menu form to store it's information.
+  if (!empty($node->in_preview) && empty($link['mlid']) && !empty($link['link_title'])) {
+    $link['mlid'] = TRUE;
+    // @todo move all other properties to attributes as well ?
+    $link['options']['attributes']['title'] = $link['description'];
+  }

1) I do not understand what this "trick" is doing, and, which consequences of this manipulation have been considered (and thus are expected) and which have not. Right now, I assume that plenty of consequences were not considered, since this entire feature request is a million-dollar effort in the current architecture, and when accounting for all consequences.

2) I don't understand why the conditional code futzes with the link's attributes.

3) Why is the preview shown in a form in the first place...?

+++ b/core/modules/menu/menu.module
--- /dev/null
+++ b/core/modules/node/images/btn-arrow-preview.png

Arrow buttons are a thing of the pre-HTML5 era.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -26,6 +26,16 @@ class NodeFormController extends EntityFormController {
+    // Check if we can retrieve a node from the tempstore.
+    $tempstore_id = drupal_container()->get('request')->query->get('tempstore_id');

A preview like this, on a separate page, should not call into the entity form controller in the first place. That's view vs. edit.

+++ b/core/modules/node/node.module
@@ -964,6 +960,23 @@ function node_load($nid = NULL, $reset = FALSE) {
+function node_tempstore_load($tempstore_id) {
+  $node = drupal_container()->get('user.tempstore')->get('node')->get($tempstore_id);
+  if ($node) {
+    return $node;
+  }
+  return FALSE;
+}

OK, so here's the utterly funny part about this:

Ever since, I'd argued and preached that we simply need to save content entities, if we want to preview them. Or in more common speak: Autosave.

You're essentially doing exactly that here — merely with the exceptionally complex workaround of going through a custom tempstore to achieve a preview.

The tempstore approach prevents each and everything else from working like it is supposed to work — which essentially circles back into the Menu module change: Any module that integrates with the content entity suddenly has to take extra care for the possibility of a fake-stored entity in a tempstore, and adjust ALL of its code to work with that edge-case.

So here's the essential question:

Why do we even try to store something in a fake storage and invent tons of custom plumbing for dealing with that, instead of actually storing it?

Make "live" previews limited to entities that can be stored and have a publishing status. — No one will complain.

We've significantly improved entity revisions for D8. — The moment you click Preview, you surely have reached a state of art that is worth to backup in a new revision.

If necessary, we can look into "tagging" auto-created revisions later on, so as to prune and clean them up upon final save.

However, the big, important, and essential difference is this:

1) We're a Content Management System. We care for your content, even if you merely hit "Preview".

2) We do not force 13,000+ contributed modules to deal with a (strange) concept of unsaved-but-then-again-saved-but-then-again-not-for-realz entities that may or may not be rendered.

3) Instead, we pave the way to actual, site-wide previews, which actually include entire site sections, content, menu link tree structures, permissions, and whatever else is needed. This is the multi-million-dollar part that will take until D10 to be figured out in total. But at the very least, all modules are able to rely on clearly defined concepts (revisions) in working their way towards the New World Order™.

And of course, everything else related to this, is mostly baked-in already: Permissions to view a (unpublished?) node in preview? A router path to view a node revision? Differences to your last preview? etc.pp.

+++ b/core/modules/node/node.module
@@ -1761,6 +1774,13 @@ function node_menu() {
+  $items['node/preview/%node_tempstore'] = array(

Can we reverse this into /node/%node_tempstore/preview ?

+++ b/core/modules/node/node.module
@@ -2277,6 +2297,103 @@ function node_page_view(Node $node) {
+function node_preview_form_select(array $form, array $form_state, Node $node) {

It looks like this form should be in node.pages.inc.

Or even better, part of NodeRenderController. That is, because this form is attached to a rendered node entity.

That said, the view mode to render the entity in should be part of the router path; i.e., /node/%node_tempstore/preview/%[view_mode]

Or even more appropriate, leave it out of the path, but turn it into a ?view_mode=xyz query string parameter instead.

In turn, this form can be changed to use 'method' => GET, which in turn eliminates the submit handler, and instead, moves the view mode processing into the actual code path that should care for it.

+++ b/core/modules/node/node.preview.css
@@ -0,0 +1,70 @@
+.node-preview-container {
+  background: #d1e8f5;
+  background-image: -webkit-linear-gradient(top, #d1e8f5, #d3e8f4);

Minor, in terms of the overall patch: ;)

As you already figured, any kind of gradient or special background, positioning, and any other fancy visual styling doesn't belong into module CSS. :)

swentel’s picture

Status: Needs work » Needs review
FileSize
10.71 KB
27.26 KB
FAILED: [[SimpleTest]]: [MySQL] 49,303 pass(es), 74 fail(s), and 8 exception(s). View

3) Why is the preview shown in a form in the first place...?

It's not, it's basically just like a node/nid page but with the node loaded from tempstore. The only form on that node preview page is the view mode select form to switch view mode, that's it :) The code in NodeFormController() is to figure out if we have something for this node/add node/x/edit page or not.

Or even more appropriate, leave it out of the path, but turn it into a ?view_mode=xyz query string parameter instead.

That's already the case.

Addressed following things

  • change path to node/%tempstore/preview
  • moved node_preview_form_select to node.pages.inc
  • removed the 'dirty' tricky in menu and path- and others. It's not needed because of the new code that disables form_state cache and sets it to rebuild if wanted. This isn't commit/production ready code as it's not perfect, but this should allow us not to make hacks, and contrib modules can sleep nicely as submitted their data is preserved when coming back to the edit screen. Need to digg into form.inc - or if someone with Form API voodoo power can always help along to figure out where we can easily plug this in.

The interdiff is against #114

larowlan’s picture

sun’s picture

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -67,6 +67,25 @@ protected function prepareEntity(EntityInterface $node) {
+    // Don't use form cache.
+    $form_state['no_cache'] = TRUE;

I don't think that's possible, since #ajax and a couple of other things rely on the form cache.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -67,6 +67,25 @@ protected function prepareEntity(EntityInterface $node) {
+    // Check if we can retrieve from the tempstore.
+    $tempstore_id = drupal_container()->get('request')->query->get('tempstore_id');
+    if (!empty($tempstore_id) && ($data = node_tempstore_load($tempstore_id, FALSE))) {
+      $form = $data['form'];
+      $form_state = $data['form_state'];
+      $node = $form_state['entity'];
+      $form_state['rebuild'] = TRUE;

That said, this code pretty much resembles what Form API's form cache is doing already... unless I'm terribly mistaken, the entity is cached in there already.

It looks like you should be able to simply leverage the Form API cache by doing this:

node/%nid/preview/%form_build_id

And with that:

$form_state['input']['form_id'] = 'node_form';
$form_state['input']['form_build_id'] = $form_build_id;
$form = drupal_build_form($form_id, $form_state);

(give or take some details)

That said, I still don't understand why this code gets executed in the code path of the entity form.

meba’s picture

I think sun raised a good point - wouldn't it be better to simply save the node unpublished? 90 % of Drupal users have some kind of Workflow module installed anyway so would expect this behaviour.

Bojhan’s picture

@meba Where do you get that number from - with our long tail I imagine its far lower? I am skeptical about auto-save, not because it wouldn't be good - but because all these "modules" that will then integrate, to me sounds like a dream. I have been waiting for years to get any type of preview on blocks/menu's etc, and no one is interested. Unless anyone offers to help out on getting those parts integrated with this, I dont see much point to swentel turning it all around - also lets keep in mind what Edit module does, that uses tempstore almost entirely?

aspilicious’s picture

When you PUBLISHED a node and you edit it and you want to PREVIEW your changes the node can not be autosaved because that would destroy the entire point of the preview.

sun’s picture

@aspilicious: There have been significant improvements to node/entity revisions in D8 (#218755: Support revisions in different states) and we continue to work very hard on that (#1643354: [Policy, No patch] Overhaul the entity revision terminology in UI text), so what you're saying was the behavior of D7, but with all the entity revision improvements, we should be in a state that allows for that. Unless I'm mistaken, that's what seems to be called "forward-revision" (#1776796: Provide a better UX for creating, editing & managing draft revisions.).

moshe weitzman’s picture

@Sun - I think saving temporary work revisions also has problems. Namely, hooks like to fire during Save operations and all those hooks would need to decide if they want to act on temporary saves. So we have traded off complexity by avoiding tempstore. I don't know which is the worse evil (tempstore or meaningless revisions).

As for autosave, I see that as a different thing, and it is confusing to mention it here. I use the word autosave to refer to client side caching of forms. This form storage is robust enough to survive when you navigate off a form, close a tab, etc. Core patch in progress at #153313: Form data is lost when using the browser's back button

sun’s picture

@moshe:

2) There's admittedly some confusion around the term "autosave", potentially caused by the contributed Autosave module. However, all client-side editor library plugins called "autosave" that I know of are actually issuing a HTTP request to the server, in order to simply save the content being edited by the user on the server-side, because the client-side is not a reliable storage and not what a user expects from a system that claims to manage content (e.g., switching between clients, browsers, power outages for more than 6 hours, whatever). Which in turn brings me to:

1) I think I've sufficiently clarified that unpublished draft revisions will not magically work. In fact, I'm relatively sure that entity-integrating modules are full of bugs right now (e.g., Menu module creating a publicly visible menu link for a unpublished node/entity). However, what I'm saying is this: We need to cater for that anyway, because draft revisions are absolutely going to happen. So why do we introduce yet another possible entity state + storage, next to draft revisions, which adds a considerable amount of complexity across the entire system?

In the end, the experience that users expect from a system that claims to manage content is exactly this:

Just save what can be saved, and manage the content for me. Don't make me think of "save." And, screw you if you ever fail to save my previous work.

That's the today's experience of Google Docs/Drive, and alas, also WordPress (apparently, since years already, since they're using the TinyMCE Autosave plugin, which essentially triggers a form submit every 30 seconds or so). What both have in common is that they truly manage the content for you and make you forget about "saving".

Additionally, both are automatically aggregating, pruning, and cleaning up needless intermediate automatically saved revisions in between (hence, "autosave"). AFAIK, WordPress achieves that by "tagging" auto-saved revisions, in order to identify them later (but since it's just WordPress, they're [ab]using the revision log message field to contain an auto-generated log message, and simply querying on that unique string identifier in the revision clean-up operation).

In the end, I'd like us to move on (and actually forward) on these pathways:

1) Become an actual Content Management System. It's surprising that we're even listed as one currently. Drupal is a pure Input/Output layer, there's factually no user content that is actually "managed". Entity revisions are the key facility towards that.

2) Leverage the architectural functionality that we already have, instead of inventing a new, custom "temporary storage" for user content.

3) Push the entity revision efforts forward.

4) Pave the way to CRAP (Create, Read, Archive, Prune) instead of CRUD (..., Update, Delete).

5) Pave the way to previews of complete site sections, instead of just a single entity.

In essence, I'm not saying that the tempstore approach cannot work out. But what I'm saying is that it's step in a very wrong direction. Technically, architecturally, and also from a product perspective. The web and end-user-facing technologies and applications are on their way to completely abandon and leaving behind the stone-age approach of a form that needs to be submitted or saved. Instead, users expect to interact with something, and as soon as they do, the app takes care of making sure that the user's interactions are persistently recorded somewhere. That is what enables "Undo" and "Redo". That's what enables you to forget about "Save". That is what gets you draft revisions that you can compare and revert, *before* making your post final.

In turn, a "Preview" button that saves into a tempstore, which will inherently forget about my entire precious work in X amount of minutes is the exact opposite of that. All we really gain from that is a fake-preview of content, which is inherently limited to exactly the entity itself, by design. It won't be able to support anything beyond that in the future. And it's totally not what end-users expect from a CMS in 2013.

swentel’s picture

It looks like you should be able to simply leverage the Form API cache by doing this:

Looked some more into this today and this is hard. If rebuild is not set, then the form is not cached, so no entity, so I can't leverage the form api cache at all and if set to true - even only in the preview method - the form won't redirect, unless I force it. Oh, the fun ..
Even #512026: Move $form_state storage from cache to new state/key-value system won't help me any further on that.

Bojhan’s picture

@sun Could you perhaps shoot a helping hand on this one, it looks like we are fully willing to do it - but it requires some hardcore knowledge of Form API. I agree that in the end we want entity revisions and with that preview, on everything - so this is the road to go, but I think swentel is struggling to make this happen given the timelines.

swentel’s picture

#1913742: Allow custom form state to be passed to entity_get_form() got in, so I'll give it one last try to see if I can some up with something so we don't loose data - it won't be perfect, but at least something ..

Bojhan’s picture

Category: feature » bug

I am actually going to move this to bug report, I have seen users get confused about this on many occasions throughout usability tests. This is not a "nice-to-have", its an essential part of a CMS that Drupal does not support. Being able to preview content before, one presses save is an expectation many users have and the fact that we offer it with a incredibly confusing experience should warrant it as a bug (it's not really a new feature, as much as fixing a incredibly broken one).

webchick’s picture

I concur with #145. We terribly broke preview in D7, and it needs to be fixed.

Damien Tournoud’s picture

Version: 8.x-dev » 9.x-dev
Category: bug » feature

Let's not fool ourselves here. We are *not* going to tackle preview in a live environment in Drupal 8. I'm moving this as a feature request to Drupal 9.

The main problem of this kind of feature, other then the reservations that @sun raised, is that a piece of content never, ever, works in isolation. Pieces of content are displayed in Views, put in Menus, pulled by Panels, displayed in Blocks, sliced in meta-tags, indexed in Solr, etc.

This patch hardcodes the preview as the full page view, this is not going to be useful to anything except trivial sites.

The problem space described here is the one of the "Future preview", which is one very complex beast to get right. We have built full-site previews before, but that was by content staging / synchronization between Drupal instances. Building a real preview mode inside a single instance of Drupal requires all the components to be aware of the preview (including Solr indexes, database denormalization etc.), so it needs to be deeply rooted at the core of the system.

Adding some half-backed preview right at the last minute will not do Drupal 8 any good.

webchick’s picture

Version: 9.x-dev » 8.x-dev
Category: feature » bug

Sorry, but no. Even if this only worked for full node view (which it doesn't... see screenshot in #88), it's still a huge improvement on the current preview functionality, which doesn't actually let you preview anything. It shows you your node content, twice, stacked on top of each other, in the admin theme + overlay, with a yellow background. Nothing about that is even remotely what your content will actually look like when posted to your actual site.

An imperfect preview is what we already have in core today. This issue's just about fixing it so it actually works, within its current limited scope. I agree that a full-fledged time-based preview ala http://drupal.org/project/sps is definitely a D9 thing, but that's not what this issue is about (or at least, was about for the first 130 comments).

Damien Tournoud’s picture

Given that we have the option to edit in place now, which is the best preview you can build in a non-hardcoded way, why not simply removing the preview option from the node module?

webchick’s picture

Because the full node form still exists, and you need a way to preview changes from there as well. Also, you don't "add in place," only "edit in place." Adding is still done through the primary node form. And finally, "edit in place" only works for visual properties, but there may be other things you need to preview (e.g. switching the display: hidden taxonomy term causes a theme change to kick in).

effulgentsia’s picture

I'm joining this issue late, but I quickly skimmed through the recent comments. I think I agree with sun that ultimately, we'll want preview functionality to be based on unpublished forward revisions, but until the work in #1776796: Provide a better UX for creating, editing & managing draft revisions. is settled, I don't see a problem in proceeding with the tempstore approach. Tempstore is now as much a part of core architecture as $form_state is, and better suited to preview functionality, because $form_state is limited to when you stay in form context. In fact, maybe when we want to refactor previews to pull from an unpublished revision, we'll actually want to make that swappable/configurable, so that whether it's tempstore or a revision can vary.

I also agree that this issue is a bug fix more so than a new feature. If that means there's more interest in going straight to a revision-based approach (either after #1776796: Provide a better UX for creating, editing & managing draft revisions. or in parallel with it), rather than tempstore first, I think that would be ok too.

aspilicious’s picture

I strongly agree this is a bug. There is a reason why we disable the preview button by default on every site we make (even if our clients are asking for it)

webchick’s picture

FileSize
26 KB
FAILED: [[SimpleTest]]: [MySQL] 54,476 pass(es), 5 fail(s), and 47 exception(s). View

This remains one of my top 2 favourite Drupal 8 issues.

Here's an attempted re-roll from #122, back when this was passing testbot, and before things went completely off the rails.

webchick’s picture

Status: Needs work » Needs review

See what testbot has to say about that.

mgifford’s picture

The title doesn't seem to be coming through.
Notice the Error for a title

The title was definitely not Error. That would be messed.

tim.plunkett’s picture

Check watchdog, when the site title is Error there is something useful in there.

mgifford’s picture

This doesn't look like a useful message to me:

Recoverable fatal error: Argument 1 passed to node_access_preview() must be an instance of Node, instance of Drupal\Core\Entity\EntityBCDecorator given in node_access_preview() (line 2719 of /DRUPAL8/core/modules/node/node.module).

There were 6 similar php errors.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
26.01 KB
FAILED: [[SimpleTest]]: [MySQL] 54,513 pass(es), 1 fail(s), and 0 exception(s). View

That is the most useful error it could have been. It explains exactly what is wrong, on which line, and roughly how to fix it.

EDIT: This sort of EntityInterface nonsense will go away once #1391694: Use type-hinting for entity-parameters is in.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +sprint, +Spark
FileSize
25.64 KB
FAILED: [[SimpleTest]]: [MySQL] 55,302 pass(es), 1 fail(s), and 0 exception(s). View

Patch only applied with offsets. Here is a directly applying reroll. Also the fail seemed to be unrelated, due to the test system, so intrigued if this would still fail.

Status: Needs review » Needs work

The last submitted patch, node-1510544-161.patch, failed testing.

webchick’s picture

Note at some point there was lovely blue styling on that back link that seems to have gotten lost. Probably my fault.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
28.66 KB
PASSED: [[SimpleTest]]: [MySQL] 55,308 pass(es). View
3.02 KB
55.45 KB

Restoring the blue styling. @webchick did not include the node.preview.css file. Did not fix the fail yet around the log going missing when going into preview. Current state:

Screenshot_4_26_13_3_11_PM.png

moshe weitzman’s picture

IMO, the dropdown with the view modes looks terrible. I would prefer a list of buttons or links, there will rarely be more than a handful of view modes. If we have to do a dropdown, lets use one of our dropbutton thingies.

Bojhan’s picture

@moshe I am sure we can make the select list look better (e.g. see the design in the issue summary). I am not sure if we want to abuse the dropbutton for this, its quite nice to have two distinct form elements a button to go back, and a select list to switch view modes. I don't care as much about the implementation particulars, but having two different visual elements will look and perform better.

Gábor Hojtsy’s picture

@Bojhan/@moshe: not sure of best next step there, the design in the issue summary was pre-seven-style-guide and it does not seem to look anywhere close to how Seven is evolving with http://groups.drupal.org/node/283223

Bojhan’s picture

@Gabor I am sure we can make it match, I'd rather make sure we push on the functionality than its styling. We still have very little of the style guide in, anyway.

Gábor Hojtsy’s picture

Right, so the point of reviewers above was that the functionality is already greatly improved vs. the current "preview", which is a joke :) We have a passing test that works. What else do we need then? :)

moshe weitzman’s picture

Status: Needs review » Needs work

OK, we can defer style issues.

#159 suggests that we can cleanup once a dependant issue lands. That issue has landed so Needs Work? That's my reading of the situation. If I'm wrong, I see no other blockers.

swentel’s picture

+++ b/core/modules/menu/menu.moduleundefined
@@ -544,6 +544,13 @@ function menu_form_node_form_alter(&$form, $form_state) {
+  // In case we're in preview and there is no mlid yet but a title
+  // has been supplied, trick the menu form to store its information.
+  if (!empty($node->in_preview) && empty($link['mlid']) && !empty($link['link_title'])) {
+    $link['mlid'] = TRUE;
+    // @todo move all other properties to attributes as well ?

This is a hack I added at some point, however, this won't fly at all as we'd have to 'fix' this for every form that alters the node form, e.g. the alias, comment settings ... Going back to the node edit form will loose data.

I've tried getting around that in #132, but got completely stuck on Form API/tempstore.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
28.46 KB
FAILED: [[SimpleTest]]: [MySQL] 55,889 pass(es), 10 fail(s), and 2 exception(s). View

Patch did not apply anymore. First a straight reroll.

Gábor Hojtsy’s picture

@Moshe: re #170, to get rid of EntityInterface as a generic and use NodeInterface instead, the patch does at two new functions where EntityInterface is used, however there is already 50+ uses of EntityInterface in node.module (where these 2 functions are added), and absolutely no use whatsoever of NodeInterface. So I think for consistency, it makes most sense to use EntityInterface in this patch and do a generic EntityInterface => NodeInterface conversion at a different issue.

@swentel: re #171, what do you think should be the way forward here? Rip that code out and accept the form data loss? Sounds bad. :/ What did you get tripped up with above? It is not clear from the comments, so I could use a short summary. @sun's idea in #135 to use the form cache is also an interesting one, might actually be the easiest approach?

The last submitted patch, node-1510544-172.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

#172: node-1510544-172.patch queued for re-testing.

The last submitted patch, node-1510544-172.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
28.42 KB
FAILED: [[SimpleTest]]: [MySQL] 57,237 pass(es), 10 fail(s), and 2 exception(s). View
0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Straight reroll since one of the node.module hunks did not apply anymore.

@swentel, feedback is still welcome!

swentel’s picture

+++ b/core/modules/path/path.moduleundefined
@@ -99,7 +99,10 @@ function path_menu() {
+  if (!empty($node->in_preview) && !empty($node->path['alias'])) {
+    $path = $node->path;
+  }
+  elseif (!empty($node->nid)) {
     $conditions = array('source' => 'node/' . $node->nid);

This is problematic, already mentioned by sun, see his review in #131. It means that we should do this in every module that form alters on the node form, but the DX is just, well, crap :)

My follow up patch in #132 (see interdiff) uses a different trick where it manipulates the form state in the form method itself. However, ajax was broken with that. Trying to leverage the form cache alone is not easy either, because on a submit, that cache is cleared, so we loose it.

#1913742: Allow custom form state to be passed to entity_get_form() is now in, so maybe we can leverage that. We'll need to store the form state ourselves and add it again in the node add/edit page callback, but I haven't tested that.

Gábor Hojtsy’s picture

FileSize
913 bytes

I've been thinking about and been experimenting with alternate solutions. One idea I had is to keep the preview on the same path and just do alternate renderings of the form. When in preview, render the frontend theme and only affordances to go back to the form and when in editing, keep the admin look if configured. Now overlay breaks this whole thing, because it cannot redirect a form POST operation, it will do a GET request on the same path when we switch from the same path being admin/frontend, so all your ongoing changes are lost. Damn overlay! It looked too simple otherwise (even if a bit hackish). Attached the start of the experiment which in itself proved a fail :/ (it works nicely if you turn off the overlay altogether).

So looks like our best bet here is to capture the form cache and reinstate it when coming back to the form. Meh.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
27.53 KB
FAILED: [[SimpleTest]]: [MySQL] 58,267 pass(es), 10 fail(s), and 2 exception(s). View

All right, once again a straight reroll because this did not apply anymore. Status merely for the testbot.

Gábor Hojtsy’s picture

FileSize
24.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-1510544-182.patch. Unable to apply patch. See the log in the details link for more information. View

Dropped all path.module, taxonomy.module and menu.module special casing code that should not be present as discussed above.

Gábor Hojtsy’s picture

FileSize
4.93 KB

All right based on that not working really for fields, here is another concept idea based on @swentel's suggestion above.

1. In the node form preview(), we store the current form_state in tempstore and just modify it a tiny bit to flag it as already coming from tempstore.
2. We redirect to the preview page with the tempstore ID passed on
3. The preview page takes the tempstore ID, loads form state from the tempstore and reignites the form.
4. Based on where we froze the form state, it will end up in preview() again but now we have our own flag on telling this is now an actual preview of the node, so it will do the render instead of the storage.
5. Most other things for rendering the actual preview page can stay, I just did not include them here to make the concept easier to understand...

The only little problem is I have been banging my head into the wall on some parts of this for a while, eg. how to get a node entity form for a yet non-existent but half-submitted node... Help on making this concept work would be welcome :)

Theoretically this uses the same tricks and rendering from the form state as before, it would just do it in the frontend theme, so the preview would appear in the actual design that it will end up in. Second experiment attached.

Gábor Hojtsy’s picture

FileSize
12.69 KB

Bringing the experiment forward. This will render the node on a different path based on the frozen form state. It does not work with exposing the different view modes though (and for some reason it may not really preview the full view mode even though that is what we request). The preview is displayed as part of a re-rendering of the form. We should not show the actual form :) Then reinjecting the form state back into the form when going back is not working yet either. I looked but not sure where could that happen in the routing/controller setup that we have.

webchick’s picture

I didn't think it was possible, but we somehow made the preview in D8 even worse than in D7, because it now displays the node four times (two copies each of trimmed/full) instead of two, which was already freaking ridiculous. :) I still feel extremely strongly that we need to do this.

I can't quite parse out how to help with #183 and #184 though.

tim.plunkett’s picture

How can we help here? Not sure if I should reroll #182 or #184

tim.plunkett’s picture

Ugh.

Gábor Hojtsy’s picture

@tim: essentially what we found with intense discussions with @swentel is that the current API only allows the temporary node to be built from a form state. You cannot just save the temporary node in tempstore because existing code only reacts to save and has special case code for preview. So unless we want to rewrite all the special case code for preview (and change the APIs for that), we need to pass around the form state and rebuild the node preview from that. Ideally we could keep posting the form from page to page, but overlay makes it totally impossible, since if you are editing the node in an overlay, the only way to break out of the overlay is with a GET request for the parent page. So we need a temporary form state storage on the side. I built a custom form state store in tempstore in the experiments above, but we may be able to retrieve the form state from the form cache if we pass around the form build ids. Then we can display the form again on a later page with the prior form state and/or use the form state to initiate a preview. The experiments only went so far to prove that storing the form state elsewhere can let us initiative a preview on a different path for the same form. It is far from complete. It would need to complete the rest of the pieces:

1. See if the form state can be directly retrieved from the form cache instead of custom tempstore.
2. Make it possible to restore the form based on the prior state on a later page.
3. Ensure this works with overlay :)

Or scrap this whole exploration and look into all the special casing code for previews and rewrite them for a different workflow of rendering. :)

larowlan’s picture

Note that EntityViewController might help here, if we can set the entity from form state to the $defaults['_entity'] and also set $defaults['view_mode'] we can get the output from EntityViewController. Sounds like a job for a RouteEnhancer?

jibran’s picture

The last submitted patch, node-1510544-182.patch, failed testing.

Bojhan’s picture

Since this isn't really worked on actively, and it doesn't seem part of Spark or any other initiative. Should we just postpone this to D9?

i always see people showing it in D8 demo's but its hasn't been close to RTBC since we started it.

PieterDC’s picture

#2123085: Add preview options may be related to this issue, but I'm not sure (yet).

webchick’s picture

No, I don't think we can postpone this to D9, at least without fixing the critical regression we currently have where previewing your content shows it *four* times instead of only 2 in D7. Since this patch *also* happens to fix the critical problem we introduced in D7 (but didn't discover until after we shipped) of showing content intended for the front-end of your site in your *admin theme* (wtf?!?) I'd prefer to do this instead.

#2123085: Add preview options seems very cool, but ideally would hook into the preview bar added here.

Bojhan’s picture

So I think given that its wise to make this critical. I dont think we can call ourselves a CMS, without a proper way to preview content.

webchick’s picture

Priority: Major » Critical
Issue tags: -sprint

It's tricky, because the preview bar "feature" certainly isn't critical by any definition of critical. Fixing the "4x" regression in the preview feature is, and wouldn't necessarily need this approach to solve it. But. It's also known that in UX testing Drupal vs. WordPress, "my front-end content shows up in my admin theme" consistently comes up as a critical UX issue for Drupal to solve, and this patch solves that very well. Additionally, fixing preview this way in core would open the doors to the reality of what's needed to preview in 2014+ (#2123085: Add preview options touches on some of that), that could then be extended by contrib, once the basis was in core.

So given all of that, I think I agree that this belongs as a critical issue. See #145 for the reasoning. Just because we shipped D7 with this in a completely broken state wrt this feature doesn't mean that the status quo shouldn't block release of D8.

sun’s picture

The currently existing content previews are historically based on the idea that user input != output due to input filters. This kind of "text content preview" was the one and only purpose of our current previews.

Fast-forward to 2014, end-users reasonably assume that a text content preview can be easily delivered via Ajax, perhaps even in a "live" (real-time) updating fashion.

Our "Preview" buttons attempted to advance on that base concept over time, in order to preview additional data to the user. We added a "preview" of how e.g. the entered author name of your comment will be interpreted, as a [form] #item.

And over time, we further advanced on that concept to actually take your user input, process it into a "fake" entity (that doesn't exist) with plenty of fake property values, in order to be able to actually render your user input into a "real" entity in your preview. (A contrib developer's nightmare.)

Heck, for nodes, we're even generating two previews instead of one for you: A summary preview + A full preview.

But in a modular system (like Drupal) and a system in which final appearance vastly depends on context (like Drupal), these kind of previews are 99% inaccurate at max.

By turning the preview from an in-page preview to a full-page preview, you're improving the accuracy from 1% to 2% (at max). You will still not preview the associated menu link, the URL alias, nor anything else. Instead, you're showing me (1) an arbitrary page, (2) on which my entered content happens to be output, but (3) that is not an improvement over status quo.

I'd agree with the proposed solution, if it would not be a PITA for the application's architecture, storage, and DX at glance.


Given the architecture of D8, the only solution for this issue is #131 and #141. Revision that shit. Store forward-revisions. Only read the latest and published revisions/versions in production. Other applications did succeed using that approach already. There's no point in re-inventing the wheel.

Anything else will leave us in cluster of WTFs, both from a developer perspective and user perspective.

A concrete plan exists. You may dislike it, due to the massive changes being required. But that is not a reason to favor a "stop-gap" approach that is going to add a good amount of complexity to a system that is way too complex already, and which is not going to improve the situation in substantial ways.


FWIW, I disagree with bumping this to critical. Drupal's "previews" suck, I couldn't agree more. +1 on that.

However, this is a feature that requires serious architectural rethinking in order to work out. You had ~4 years (1,350+ days) to attack this problem space and, if necessary (certainly necessary), fully revamp how content is handled in Drupal. That did not happen.

By bumping this to a critical, release blocking "bug", the notion of "API/code freeze" has lost each and every meaning.

Regardless of how lackluster current previews are, this is a (major) feature request.

webchick’s picture

Hm. I don't recall saying that the current patch was necessarily a viable way to fix this issue, only that the issue itself is critical to resolve in D8, at least from a UX POV.

David_Rothstein’s picture

I don't think D7 shipped with this more broken than previous versions - just with the problem more visible (especially in user testing) since the admin theme was part of the standard install profile. Anyone who used an admin theme before that hit the problem before that.

@sun's ideas look pretty interesting to me. #140 raises some possible complications:

@Sun - I think saving temporary work revisions also has problems. Namely, hooks like to fire during Save operations and all those hooks would need to decide if they want to act on temporary saves. So we have traded off complexity by avoiding tempstore. I don't know which is the worse evil (tempstore or meaningless revisions).

But I would say that's arguably a good thing because we already have a need for this distinction even independent of previews. For example, if your module wants to fire off a notification e-mail every time someone saves a node, you already have the issue where if nodes are saved programmatically (like in an update function which runs in order to fix something about the node) you can wind up with a lot of e-mails you didn't want. Being able to distinguish "housekeeping" saves from "real" saves is important generally, and temporary revisions might just be a particular instance of a "housekeeping" save.

Gábor Hojtsy’s picture

We've explored saving temporary revisions many times before. The main problem is only files have a garbage cleanup operation, where the files that were uploaded for a preview which was then not saved are later removed. Other items, like menu items or taxonomy terms, which would be created with this preview would need a garbage collection feature as well and be able to save as temporary as a consequence.

AFAIS keeping the in-memory entities that are being displayed is not *adding* complexity to the system, because that is already what we do. The only thing we don't do is to preview it in a frontend theme. That becomes much easier to do if overlay is not being used (eg. removed from core), because then we can post around data between different URLs easily, and some of them could display the in-memory entity with a frontend theme, while others could display the node form. I don't think that is any more of a hack than what we do now, we also do in-memory entity creation based on post data and then rendering that. We'd just post to a different URL and let the user come back to the editing URL.

My 2 cents.

Gábor Hojtsy’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Issue summary: View changes
Issue tags: -Novice

Now that Overlay is gone, this should theoretically be easier. I'm going to take a look at this.
However, the last patch was in June, and it doesn't even come close to applying. I'll use it for inspiration, but I'll likely have to start from scratch :(

PieterDC’s picture

Go for it, Tim!

tim.plunkett’s picture

That becomes much easier to do if overlay is not being used (eg. removed from core), because then we can post around data between different URLs easily, and some of them could display the in-memory entity with a frontend theme, while others could display the node form

I've tried several different approaches for this, but none seemed to work. chx didn't think that FAPI actually supported POSTing from one form to another.

Gabor, any insight?

Otherwise, I think we're stuck with:

So unless we want to rewrite all the special case code for preview (and change the APIs for that)

...

yched’s picture

Gábor Hojtsy’s picture

@tim.plunkett: the idea was not that one form would post to another but the same form would appear on multiple URLs. One URL (node edit) could post to another (node preview). The another can be on the frontend (with frontend theme). Since it would be the same form on two different URLs, it could direct its behaviour between the two URLs. I did not look how the form is identified in FAPI but assumed the path would not be significant so long as the form ID is the same (eg. a form submitting form one URL to another would work if both URLs have the same form attached).

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
John_B’s picture

I have read & thought about this in the hope that a view from an 'outsider' to core will bring more clarity than confusion (which is a risk). The aim is to make the job look much more manageable, and less complex.

1. A lot of the thread was made more complex because of the overlay problem, which is gone.

2. Although I will suggest that displaying form state is not the way to go, passing form state back to the form should not be an issue. It is perfectly acceptable to open a Preview in a new tab / window, as WP does.

3. To be controversial, the Teaser preview issue is a hang-up of the Drupal mindset. To the content creator, a teaser or other view mode is no less an arbitrary re-working of a node than the output as rearranged by a Views row, etc. Besides nobody is likely to view a Teaser alone, and think 'hey, I am previewing a live environment' because one rarely makes a page with a single teaser. Once one steps outside a Drupal mindset, it is safe to drop view modes from the preview button and preview only the default view mode of a node or other entity. Teaser previews can be optionally added later.

4. There must be a single non-serialised copy of the node using ordinary 'save', not special case preview code mentioned as a problem in #189.

5. I am not calling the temporary copy a forward revision, it could have its own store outside revision table (which is my understanding of the aim of patch at #184). However using revision tables makes sense.

6. If that means ripping out a lot of special case Preview code, my feeling as someone not familiar with that code that this just has to be faced.

7. There must be a user lock as used by Views, or Wordpress (which has an alert 'locked to user [name]: do you want to break that lock?'). Granular access control on that 'break lock' option can be added optionally later, or in contrib, once the feature is working. The 'break lock' button deletes any forward revision or other temporary copy so a new user-specific one can be created.

8. There should be a popup alert 'do you really want to leave this page' (another WP idea) when leaving a page with unsaved data. Clicking 'Yes' triggers the cleanup. However, a full files cleanup is not absolutely essential for a first iteration, there could be a message 'please delete your uploaded files first.'

9. A copy of form data should be saved to revision tables (or special store) with an autosave-like post request every two minutes. Expired forms, and lost work, is a bigger criticism of Drupal 7 in my experience, and an even more basic problem, than lack of live preview.

webchick’s picture

As further anecdotal evidence of how much this actively bites actual users of Drupal, I gave a Spark session at DrupalCon Austin and spent the last 30 minutes in a big messy group prioritization exercise, where everyone in attendance (maybe 80-100 people) could shout out pain points for content authors (among other questions) and then everyone got two "votes" (hand raises) as we went through the list, which I then tried to eyeball and stick in some kind of order. Here's the full results for those who are curious: http://www.slideshare.net/webchickenator/spark-authoring-experience-in-d... (slides 32+)

When we got to "Preview," almost every single hand shot up in the room. While I can't say for sure who was there, and what their specific backgrounds were, it's safe to assume it was a good mix of both big site and small site builders, some content authors, etc. I feel really strongly that we have to make some amount of headway on improving Previews in Drupal 8 as a release-blocking effort. If that means breaking APIs, so be it. But ideally, we'd take something like Gábor's approach, and possibly some of John B's suggestions and do this as an incremental improvement over the current (absolutely horrible) preview experience (it's seriously not really possibly to make it much worse) rather than a "boil the ocean" effort that attempts to cover every possible edge case like SPS tries to do.

If the preview simply happened in the front-end theme and there was a way to select among different build modes (which is exactly what swentel's patch did over a year ago) that would be sufficient to close this critical, IMO.

swentel’s picture

FileSize
19.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,399 pass(es), 60 fail(s), and 5 exception(s). View
108.8 KB

So, last try :)

There's now a preview controller and a param converter for {node_preview}. It looks a bit more modern already. Work in progress though.

Two things don't work yet:

  • Switching a view mode - and the form is still old school FAPI call.
  • 'back to content editing' only works for a /new/ node (but is not hard to make it work though), so if you want test already, use an existing node.

Theoretically we could probably make this available for all entity forms/types, but I didn't want to go that far for now - I did a quick test by just adding a retrieved $form_state in the form builder getForm() method and that seemed to do the job just fine, but for now, the manipulation just happens in NodeForm::form().

I'll be back this evening on IRC - feel free to post updated patches any time.

swentel’s picture

Status: Needs work » Needs review
FileSize
21.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,022 pass(es), 48 fail(s), and 3 exception(s). View

More updates - node/add and view mode switching (with javascript reload) now works too.
Still todo (at least technically): access check (it's now using 'administer content types' permission for testing).
Setting to needs review for testbot.

swentel’s picture

Status: Needs work » Needs review
FileSize
23.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,414 pass(es), 29 fail(s), and 3 exception(s). View

Added access controller (which returns allow for now) - another testbot check

Bojhan’s picture

Wowa, great that you found time to work on this! It seems to work also switching display modes (Teaser/default).

joelpittet’s picture

FileSize
188.6 KB

@swentel++

larowlan’s picture

  1. +++ b/core/modules/node/node.module
    @@ -1128,6 +1123,76 @@ function node_view_multiple($nodes, $view_mode = 'teaser', $langcode = NULL) {
    +  $node = \Drupal::request()->attributes->get('node_preview');
    

    We should be checking the route name here instead of an attribute

  2. +++ b/core/modules/node/src/Controller/NodePreviewController.php
    @@ -0,0 +1,71 @@
    + * Contains \Drupal\node\Controller\NodeViewController.
    

    NodePreview

  3. +++ b/core/modules/node/src/NodeForm.php
    @@ -355,6 +393,10 @@ public function submit(array $form, array &$form_state) {
    +    // Remove
    

    comment needs work

  4. +++ b/core/modules/node/src/NodeForm.php
    @@ -367,11 +409,10 @@ public function submit(array $form, array &$form_state) {
    +    $form_state['redirect'] = 'node/preview/' . $this->entity->uuid() . '/default';
    

    Should use redirect route

  5. +++ b/core/modules/node/src/ParamConverter/NodepreviewConverter.php
    @@ -0,0 +1,58 @@
    +class NodepreviewConverter implements ParamConverterInterface {
    

    I think should be NodePreviewController

As discussed on irc, I'm happy to help move the form stuff to OO.

swentel’s picture

Status: Needs work » Needs review
FileSize
29.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,468 pass(es), 2 fail(s), and 2 exception(s). View

Mosts tests fixed - except for the two extending on kernelTestBase.
Review from #217 still needs to be address as well, but that's for tomorrow - or for anyone who wants to pick this up in meantime.

larowlan’s picture

FileSize
7.34 KB
33.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,526 pass(es). View

Should fix tests, moves form to OO and fixes 217.1 and 217.4

larowlan’s picture

Status: Needs work » Needs review
jibran’s picture

  1. +++ b/core/modules/filter/src/Tests/FilterFormatAccessTest.php
    --- /dev/null
    +++ b/core/modules/node/css/node.preview.css
    

    Have we made changes for rtl CSS.

  2. +++ b/core/modules/node/css/node.preview.css
    @@ -0,0 +1,21 @@
    +  z-index: 499;
    

    I am not a CSS expert but isn't it a lot?

  3. +++ b/core/modules/node/node.routing.yml
    @@ -37,6 +37,19 @@ node.add:
    +    #_entity_access: 'node.view'
    

    Is it needed? Perhaps convert it to node.preview.

  4. +++ b/core/modules/node/src/Access/NodePreviewAccessCheck.php
    @@ -0,0 +1,53 @@
    +    // TODO
    

    ok

  5. +++ b/core/modules/node/src/Form/NodePreviewForm.php
    @@ -0,0 +1,115 @@
    +namespace Drupal\node\Form;
    +use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
    

    space missing.

  6. +++ b/core/modules/node/src/Form/NodePreviewForm.php
    @@ -0,0 +1,115 @@
    +    $query_options = !$node->id() ? array('query' => array('uuid' => $node->uuid())) : array();
    

    Perhaps $node->isNew() can be used here instead of $node->id()

  7. +++ b/core/modules/node/src/NodeForm.php
    @@ -47,6 +78,22 @@ protected function prepareEntity() {
    +    if ($request_uuid = \Drupal::request()->query->get('uuid')) {
    

    Is there $this->request in this class?

  8. +++ b/core/themes/bartik/css/style.css
    @@ -847,6 +847,59 @@ ul.links {
    +    z-index: 499;
    

    same as above.

  9. +++ b/core/themes/bartik/css/style.css
    @@ -847,6 +847,59 @@ ul.links {
    +    border: 1px solid #0048c8;
    ...
    +    padding: 4px 1em 4px 0.6em;
    ...
    +    border: 1px solid #0048c8;
    

    I haven't seen any RTL changes in the patch.

Wim Leers’s picture

Thank you, swentel, for taking this on again! I have to say it works wonderfully well: taxonomy terms that don't exist yet are simply not yet linked during the preview stage (just like in in-place editing), but work correctly otherwise. It works fine both for adding and editing nodes. AFAICT in terms of pure functionality, this works well, or at least well enough.

The only problem I was able to find while doing manual testing, was that when you switch to the teaser view mode, the page title is still being set to the node title. So then you see the node title as both the page title *and* inside the teaser.

I also looked at the code, where it's still easy to find things to nitpick about — but overall, I think this will get us there :) Are there any things you're not happy about, swentel?

  1. +++ b/core/modules/node/node.libraries.yml
    @@ -10,6 +10,9 @@ drupal.node:
    +    theme:
    +      css/node.preview.css: {}
    

    I don't think "theme" is correct here.

  2. +++ b/core/modules/node/node.module
    @@ -643,7 +638,7 @@ function template_preprocess_node(&$variables) {
    -  $variables['page'] = $variables['view_mode'] == 'full' && node_is_page($node);
    +  $variables['page'] = ($variables['view_mode'] == 'full' && (node_is_page($node)) || (isset($node->in_preview) && in_array($node->preview_view_mode, array('full', 'default'))));
    

    This used to be easy to understand, but that's no longer the case. An extra comment here would be helpful.

  3. +++ b/core/modules/node/node.module
    @@ -1128,6 +1123,25 @@ function node_view_multiple($nodes, $view_mode = 'teaser', $langcode = NULL) {
    +    $page['page_top']['node-preview'] = array(
    ...
    +    $page['page_top']['node-preview']['view-mode'] = $form;
    

    Super nitpicky: I don't think we use dashes in render array keys anywhere in core?

  4. +++ b/core/modules/node/node.routing.yml
    @@ -37,6 +37,19 @@ node.add:
    +    #_entity_access: 'node.view'
    

    I think this is a @todo?

  5. +++ b/core/modules/node/src/Access/NodePreviewAccessCheck.php
    @@ -0,0 +1,53 @@
    + * Contains \Drupal\node\Access\NodeAddAccessCheck.
    ...
    +   * Constructs a EntityCreateAccessCheck object.
    

    Class names outdated :)

  6. +++ b/core/modules/node/src/Access/NodePreviewAccessCheck.php
    @@ -0,0 +1,53 @@
    + * Determines access to for node preview pages.
    

    Typo.

  7. +++ b/core/modules/node/src/Access/NodePreviewAccessCheck.php
    @@ -0,0 +1,53 @@
    +    // TODO
    +    return static::ALLOW;
    

    Hrm, what exactly *is* the condition to be allowed to preview nodes? If you can create or edit a node, shouldn't you automatically be allowed to preview those cases also? Perhaps that's what this access check will do?

  8. +++ b/core/modules/node/src/Controller/NodePreviewController.php
    @@ -0,0 +1,71 @@
    +  public function view(EntityInterface $node_preview, $view_mode_id = 'full', $langcode = NULL) {
    

    Perhaps this should also set the necessary HTTP header to always prevent browsers from caching this page?

  9. +++ b/core/modules/node/src/Controller/NodePreviewController.php
    @@ -0,0 +1,71 @@
    +
    

    Extraneous newline.

  10. +++ b/core/modules/node/src/ParamConverter/NodepreviewConverter.php
    @@ -0,0 +1,58 @@
    +class NodepreviewConverter implements ParamConverterInterface {
    

    s/NodepreviewConverter/NodePreviewConverter/

swentel’s picture

FileSize
10.12 KB
33.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,529 pass(es). View

New patch - biggest change is that NodePreviewAccessCheck now does actual checking.

Addressed from #217

  • 1. Done by larowlan
  • 2. done
  • 3. Was not needed anymore
  • 4. Done
  • 5. Done

Addressed from #223

  • 3. not needed
  • 4. is now implemented
  • 5. done
  • 6. good suggestion
  • 7. apparently not, it blew up completely (unless I was doing something stupid

Addressed from #224

  • 2. Yep, makes sense, added comments to clarify the $variables['page'] now.
  • 3. Yep, underscores now
  • 4. 5. 6. & 7. done - either calls node access check or entity access check.
  • 8. Makes sense indeed, added drupal_page_is_cacheable(FALSE).
  • 9. Done - I think, bit hard to spot though
  • 10. done

The only problem I was able to find while doing manual testing, was that when you switch to the teaser view mode, the page title is still being set to the node title. So then you see the node title as both the page title *and* inside the teaser.

Yeah, that one is tricky: so, the node template file does not render the title in case it's on the full page, because then the page title is used. However, on any other preview, the title will be printed because that makes sense for the view mode, e.g. teaser. So maybe we just always set 'Preview @node_title' as the page title ? If so, tests needs to be changed for that as well in a couple of places in PreviewTest.php

For the points I missed from the last two reviews: those were css/theme stuff, which is is not my main area of expertise, so someone else should work on that :)

Overall I'm pretty happy with the state of this patch as it is. Like I already said, it's probably possible to make this available for any entity, but I didn't want to go so far, as that would lead to much more code changes.

One suggestion in terms of UX:

  1. If the 'full' view mode is enabled, redirect to it by default in the preview() submit method as the 'full' view mode is usually the most useful one.
  2. If the 'full' view mode is disabled, rename the 'Default view mode' label in the switch box on the preview page to 'Full' as that makes more sense.

I'm not available until friday now for coding, so someone else can take over from here to push it forward (I'll check possible failures after this post though)

webchick’s picture

Issue tags: +Favourite of webchick

Sorry that I have not yet gotten around to giving this a UX review. It's definitely on my list!

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

No longer applies unfortunately.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
33.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 54,646 pass(es), 7,703 fail(s), and 4,687 exception(s). View
1.78 KB

Rerolled.

jibran’s picture

Issue tags: +JavaScript
  1. +++ b/core/modules/node/css/node.preview.css
    @@ -0,0 +1,21 @@
    +  content: '<';
    

    Why is it not '.'? Why is it '<'?

  2. +++ b/core/modules/node/node.module
    @@ -1045,6 +1043,25 @@ function node_view_multiple($nodes, $view_mode = 'teaser', $langcode = NULL) {
    +  $attributes = \Drupal::request()->attributes;
    +  if ($attributes->get(RouteObjectInterface::ROUTE_NAME) == 'node.preview') {
    

    I think this can be modified using route matcher.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
33.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,114 pass(es), 1 fail(s), and 1 exception(s). View
1.08 KB

Funny, #229.2 was actual the bug preventing all of those tests from passing.

No idea about #229.1.

jibran’s picture

Ok thank you for fixing this. IMO it is ready but I am no usability expert. PFA screenshots.
Teaser Mode
Only local images are allowed.

Defualt/Full Mode
Only local images are allowed.

Here is a little pointer maybe follow up. We should add save and publish dropdown to the preview bar.

Only local images are allowed.

Another suggestion could we move preview bar to the bottom of the page.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
33.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,304 pass(es). View
425 bytes

I manually tested it (just to make sure I wasn't missing anything in the reroll), and it works like a charm. I would not consider what I did enough manual testing.

nod_’s picture

FileSize
34.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,442 pass(es). View
1.73 KB

tweaked some JS stuff.

nod_’s picture

FileSize
707 bytes
34.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,320 pass(es), 37 fail(s), and 0 exception(s). View

Sorry one more, forgot a .once().

Bojhan’s picture

I dont't fully understand swentels last suggestion. "If the 'full' view mode is disabled, rename the 'Default view mode' label in the switch box on the preview page to 'Full' as that makes more sense."

We shouldn't have anything labelled "Default" and instead say what it is about- Full.

yched’s picture

@nod_: just curious
- Why not .once('autosubmit', function() {$(this).on(...) }); ?
- [data-drupal-autosubmit] doesn't look specifically tied to the "preview" feature. If other JS behaviors use it, they will each run the same $(context).find('[data-drupal-autosubmit]').once('autosubmit') in their own JS ? Then the first one that runs wins ?

nod_’s picture

  1. It's because that "feature" is scope creep in the jQuery once plugin (and after chatting a bit with him, Rob agrees). It also adds a needless nested scope.

    I'm planning on removing all their uses through core… at some point #2180921-1: [META] Use data attributes rather than classes wherever possible

  2. That's correct. We could use .once('autosubmit-preview') or something. I'm not too worried though.
swentel’s picture

@Bojhan Well, there's always a view mode called 'default' which you can configure as a fallback. If someone in code for instance calls node_view($node, 'search_result), but the search result view mode is not configured, it will use the configuration of the default view mode.

My suggestion is to rename 'Default' to 'Full' when the 'Full view mode' is not configured, because that's usually the main view that people will see the most of a node. When 'Full' is configured though, we probably need to leave the 'Default' option as well, but we should show the preview in 'Full' though.

jibran’s picture

My suggestion is to rename 'Default' to 'Full' when the 'Full view mode' is not configured, because that's usually the main view that people will see the most of a node. When 'Full' is configured though, we probably need to leave the 'Default' option as well, but we should show the preview in 'Full' though.

Can we please move this discussion to follow up and fix this critical bug?

Bojhan’s picture

This needs a review from the technical side.

mparker17’s picture

Overall, +1000 to this.

I manually tested the patch from #236 on simplytest.me just now (at commit 9b7ead2), using the Standard install profile and all the default settings. I went to node/add/page, entered "Lorem" into the Title field, "Ipsum dolor" into the Body field, and clicked the Preview button. I got:

Fatal error: Call to a member function setAbsolute() on a non-object in /home/sc1cf97e98e3d9ff/www/core/lib/Drupal/Core/Form/FormState.php on line 638

Leaving the "needs manual testing" tag on since it'll need manual testing when it's working again.

webchick’s picture

Status: Needs review » Needs work

That sounds like a needs work.

LewisNyman’s picture

I can't demo this patch on simplytest.me, so I'll just review the CSS for now.

  1. +++ b/core/modules/node/css/node.preview.css
    @@ -0,0 +1,21 @@
    +.node-preview-container .form-type-select {
    +  margin-left: 25%;
    +}
    

    Does this need a media query so this styling doesn't kick in on mobile?

  2. +++ b/core/modules/node/css/node.preview.css
    @@ -0,0 +1,21 @@
    +.node-preview-backlink::before {
    +  content: '<';
    +  display: inline-block;
    +  width: 10px;
    +  height: 10px;
    +  color: #000;
    +}
    

    We have a left arrow in our icon set that looks a lot better than the character '<'.

  3. +++ b/core/themes/bartik/css/style.css
    @@ -847,6 +847,59 @@ ul.links {
    +    background: #d1e8f5;
    +    background-image: -webkit-linear-gradient(top, #d1e8f5, #d3e8f4);
    +    background-image: -moz-linear-gradient(top, #d1e8f5, #d3e8f4);
    +    background-image: -o-linear-gradient(top, #d1e8f5, #d3e8f4);
    +    background-image: linear-gradient(to bottom, #d1e8f5, #d3e8f4);
    

    We don't require any fallbacks for gradients based on caniuse.com and our supported browsers.

  4. +++ b/core/themes/bartik/css/style.css
    @@ -847,6 +847,59 @@ ul.links {
    +    -webkit-box-shadow: 0 1px 3px 1px rgba(0, 0, 0, 0.3333);
    

    We shouldn't be using webkit-box-shadow anymore.

  5. +++ b/core/themes/bartik/css/style.css
    @@ -847,6 +847,59 @@ ul.links {
    +    font-family: "Source Sans Pro", "Lucida Grande", Verdana, sans-serif;
    

    We're specifying a font in this stack that doesn't exist. Why are we changing the font family in the first place?

Does the button still look the same as the screenshot in #216? Is this supposed to mimic the button styling in Seven? I guess it's just out of date? We've run into this problem a few times in Drupal 8 #2195695: Admin UIs on the front-end are difficult to theme

Gábor Hojtsy’s picture

Issue tags: +Drupalaton 2014
Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Reviewing + rerolling to address #243 and #245.

Also retesting the current patch to ensure that the test coverage is actually catching the fatal in #243.

Status: Needs work » Needs review
swentel’s picture

FileSize
34.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,571 pass(es). View
2.26 KB

Was earlier :)
Works again now.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
8.5 KB
33.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,253 pass(es). View

The previous patch failed as expected, which means we have the needed test coverage. Hurray!


#245:

  1. Fixed.
  2. This is an override for .button-action::before's content: '+' declaration. Using an SVG here means we should use an SVG there, so I think this is out of scope. After talking to lewisnyman, turns out that this shouldn't be using .button-action, which means we can solve it after all. I solved it, but even though the CSS works, it's probably not the most elegant way to do it.
  3. Fixed.
  4. Fixed.
  5. Copied font-family: Arial, sans-serif; from Seven. We're changing the font family because otherwise this gets a Serif font, which feels very bizarre for a UI.

In this reroll, you'll also find some nitpick fixes.

Finally, I also have a few CSS-related questions about this patch:

  1. +++ b/core/modules/node/css/node.preview.css
    @@ -0,0 +1,21 @@
    +  z-index: 499;
    

    Why this particular z-index? Ideally, this would get a short explanatory comment. That would help with future maintenance.

  2. +++ b/core/modules/node/node.libraries.yml
    @@ -10,12 +10,16 @@ drupal.node:
    +  css:
    +    theme:
    +      css/node.preview.css: {}
    

    I think the fixed positioning at the top (the .node-preview-container CSS) and back-link can be considered structural and hence belongs in a "base" CSS file? The various padding and margin stuff would belong in a "theme" CSS file.


I can't find any "real" problems with this patch anymore, so I think this is almost RTBC.

jibran’s picture

Thanks for the fixes.
I think some one has an emotional connection with 499. This is the only logical explanation I have. :D

+++ b/core/themes/bartik/css/style.css
@@ -862,14 +858,10 @@ ul.links {
+    background: url(../../..//misc/icons/000000/chevron-left.svg) left no-repeat, linear-gradient(to bottom, #419ff1, #1076d5);

@@ -881,25 +873,22 @@ ul.links {
+    background: url(../../..//misc/icons/000000/chevron-left.svg) left no-repeat, linear-gradient(to bottom, #0e69be, #2a93ef);

double //

jibran’s picture

Wim Leers’s picture

FileSize
33.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,252 pass(es). View
1.58 KB

Fixed #252.

mparker17’s picture

I was able to successfully test this on simplytest.me on 8.0.x HEAD using the Standard install profile with all the default settings for both Articles and Basic pages in the following browsers, using "typical" content (i.e.: the contents of recent announcements and documentation pages on Drupal.org, with and without images and taxonomy terms):

  • Firefox 31.0 on Mac OS/X 10.9.4
  • Chrome 36.0.1985.125 on Mac OS/X 10.9.4
  • Safari 7.0.5 on Mac OS/X 10.9.4
  • Opera 23.0.1522.60 on Mac OS/X 10.9.4
  • IE 9.0.8112.16421 on Windows 7 Enterprise SP1
  • IE 10.0.9200.16521 on Windows 7 Enterprise SP1
  • IE 11.0.9600.17031 on Windows 8.1 Enterprise Evaluation

Everything functioned exactly how I expected it.

The only issue I can see is that, in IE9 and IE10, the node preview toolbar was initially underneath the main toolbar. When I scrolled down, it sat nicely below the main toolbar. When I scrolled back up to the top, it slid under the main toolbar again. Once I could see it, I could switch view modes as-expected. This problem was not present in any other browser, including IE11. I've attached screenshots.

Because this patch provides such a monumental usability improvement over what we have now, and because the described visaul/browser-compatibility issue is easily avoided by scrolling down the page, I would recommend filing the above visual / browser compatibility issue as a follow-up.

Removing "Needs manual testing" tag.

yched’s picture

Curious why the patch currently repeats "@mode view mode" (--> "full view mode", "teaser view mode"...) in the dropdown options, rather than factorizing "view mode" into a form label ("View mode: [dropdown]") like we would do in any other form ? Was that a deliberate UI decision at some point in the discussion ?

swentel’s picture

@yched - not particular reason, the bar at the top was created very fast just to make the POC work at the time. It's probably better yes to change those labels.

Bojhan’s picture

I am fine with changing those labels to View mode: [dropdown]

fago’s picture

+++ b/core/modules/node/src/NodeForm.php
@@ -47,6 +80,21 @@ protected function prepareEntity() {
+    $store = $this->tempStoreFactory->get('node_preview');
+    if ($request_uuid = \Drupal::request()->query->get('uuid')) {

I'm wondering whether it makes sense and is possible to read the edited entity out of cached form state instead of the temp store? We could pass on the form build id to the view page, what should allow it to read it.

fago’s picture

+      $form_state['values'] = $preview['values'];
+      $form_state['input'] = $preview['input'];
+      $form_state['rebuild'] = TRUE;

As discussed, this might loose other form state modules could have put into it, like a multistep module tracking its steps in form state. We need to persist all of form state somehow to make sure the form is properly re-initialized.

swentel’s picture

FileSize
33.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,554 pass(es). View
1.79 KB

Addressed #260 and #258 - could probably use a test to see if a form_state entry is preserved (although I'm almost sure it will)

Re: #259 and form state : I remember myself using form state, but then there was some funky things going on in Form API at that time. Maybe it's possible again, but maybe investigate it in a follow up ?

Wim Leers’s picture

Yes — this issue already has had so many patches (88 files attached!), the TempStore-based approach works fine technically, let's pursue absolute purity in a follow-up if that's considered valuable.

What's left to be done here?

webchick’s picture

I'm happy to do review + rtbc of this patch but I won't be able to get to it until Thursday, most likely.

swentel’s picture

FileSize
34.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,424 pass(es), 6 fail(s), and 0 exception(s). View
2.93 KB

rerolled + added the missing chevron left file + small docs changes in NodeForm::form().

swentel’s picture

Status: Needs work » Needs review
FileSize
34.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,879 pass(es). View
690 bytes

Ugh crap

penyaskito’s picture

  1. +++ b/core/modules/node/src/NodeForm.php
    @@ -80,22 +80,30 @@ protected function prepareEntity() {
    -      foreach (array('redirect', 'errors') as $key) {
    -        if (isset($form_state[$key])) {
    -          unset($form_state[$key]);
    -        }
    

    We don't clear errors anymore. Is this on purpose?

  2. +++ b/core/modules/node/src/NodeForm.php
    @@ -80,22 +80,30 @@ protected function prepareEntity() {
    +      if (isset($form_state['redirect'])) {
    +        unset($form_state['redirect']);
           }
    

    We should use methods if possible.

swentel’s picture

re 1: so the unsetting of keys was a quick experiment after #260 which I discussed with fago at Drupalaton. 'redirect' is a key that needs to be removed for sure. I was also adding 'errors', but after giving it some thought, pushing the preview button should call validate() and in case there are errors you won't get to the preview page in the first place, so I assume (not tested) that this is fine.

re 2: ah yes, that should probably become unsetValue() - not able to reroll for now - we can also always wait till #2310255: [meta] Remove ArrayAccess from FormState :)

webchick’s picture

Took this for a test drive tonight. (Thanks for the re-roll, swentel!)

Everything looks and works exactly like I'd expect, and it's wonderful that we get rid of so much hackish one-off code to support previews in favour of a much better system!

I did find some things as I was going through. Two of them in particular make me hesitate on the RTBC button:

1) No matter how many view modes I check off under admin/structure/types/manage/article/display for Articles, the preview is still only letting me select between Default and Teaser. By design, or is this a bug?

2) The JS to kill links so you can't accidentally destroy your data also kills the toolbar links, but not in an obvious (visual) way. Multiple times while testing, I wasted a minute or more clicking on admin links thinking they would do something when they did not. Drupal appeared to be super jankily broken. :\ Can we please limit it to only killing links in the preview itself? (which is all the previous code did)

Then, some other general comments. Don't think any of these are commit-blockers, but if they're easy that'd be great to get them done.

- Agreed that "Default" rather than "Full" is confusing. Also agreed it could be a follow-up.
- This has nothing to do with this patch, but we should explore how to make the view modes UI more intuitive. The screen at admin/structure/display-modes/view has no explanation or tie in to e.g. admin/structure/types/manage/article/display which also needlessly burned time. Since form/display modes are such a key component, it'd be nice if the admin pages were more user-friendly and less divorced from one another.
- There's an overlap of the "back to editing" link + logo in Stark. Not a huge deal, but if it's easy to fix here that'd be cool.

Things spotted while glancing through the code:

+  // The 'page' variable is set to TRUE in two occasions:
+  //   - The view mode is 'full' and we are on the 'node.view' route.
+  //   - The node is in preview and view mode is either 'full' or 'default'.
+  $variables['page'] = ($variables['view_mode'] == 'full' && (node_is_page($node)) || (isset($node->in_preview) && in_array($node->preview_view_mode, array('full', 'default'))));

Wow, that condition is craaaazy. Wonder if we should encapsulate some or all of that into a helper function.

- *   - node--preview: Whether a node is in preview mode.

Love the re-use of node.twig.html for the preview. :D

swentel’s picture

Issue summary: View changes
swentel’s picture

FileSize
34.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,673 pass(es). View
1.31 KB

1. That is a bug in getViewModeOptions(). Because if you enable them, Field UI does it right. Haven't fully looked at it but it looks like getViewModeOptions() is looking at the wrong 'status', namely that of the view mode config entity, and not the entity display. Filed #2322503: getDisplayModeOptions() returns only full or teaser regardless of the status of the entity display.

2. Not a big fan of enabling them, but I understand the disconnect. And there's still the back button :)

Addressed #267 as wel.

tim.plunkett’s picture

+++ b/core/modules/node/src/NodeForm.php
@@ -93,9 +93,9 @@ public function form(array $form, FormStateInterface $form_state) {
+      if ($form_state->hasValue('redirect')) {
+        $form_state->unsetValue('redirect');

This shouldn't work, hasValue/unsetValue was for $form_state['values'], not arbitrary keys.

swentel’s picture

Ugh right, and we save $form_state before calling setRedirect, so this isn't on it .. like ever .. :)

swentel’s picture

FileSize
33.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,830 pass(es). View
1015 bytes

Addressed #272

swentel’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/src/Form/NodePreviewForm.php
@@ -0,0 +1,119 @@
+    $view_mode_options = $this->entityManager->getViewModeOptions('node');

So plopesc did some research and this function actually behaves as intented. We should mimic something like DisplayOverviewBase::getDisplayStatuses() which is used in Field UI.

Setting to needs work because the issue I filed is not relevant anymore.

plopesc’s picture

Status: Needs work » Needs review
FileSize
35.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,798 pass(es). View
3.36 KB

Hello.

Replacing the call to EntityManagerInterface::getViewModeOptions() by new method NodePreviewForm::getViewModeOptions() that gets available view modes at bundle level, as desired for this feature.

The Full/Default issue is not managed in this patch, because there are cases where Default and Full are not the same. But given that the view modes list is now created in NodePreviewForm, should be easier handle there all the possible exceptions.

Regards.

hueman’s picture

I get:

Uncaught PHP Exception Symfony\\Component\\Routing\\Exception\\RouteNotFoundException: "Route "entity.node.preview" does not exist." at /var/www/drupal/core/lib/Drupal/Core/Routing/RouteProvider.php line 147,

Clean install 276 - patch.

swentel’s picture

Status: Needs work » Needs review
FileSize
35.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
719 bytes

Can't reproduce #279

Rerolled the patch + ignored the RSS view mode. Should be ready now IMO.

swentel queued 280: 1510544-280.patch for re-testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
35.46 KB
yched’s picture

Not sure why we remove 'rss' mode and not, say, 'search_index' ? Neither are inteded to get displayed in the front end ?

Also, minor, but looks like $display->get('mode') could be extracted into a variable in the hunk at the end of the interdiff ?

swentel’s picture

FileSize
35.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,389 pass(es). View
1.02 KB

Good points.

swentel queued 286: 1510544-286.patch for re-testing.

swentel’s picture

Status: Needs work » Needs review

Botfluke

escoles’s picture

Re. #285: FWIW unless you're talking about something else I'm not familiar with, search results (search_index) are definitely a front end display mode. Most people don't theme them, but they are front end.

Cottser’s picture

FileSize
35.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,135 pass(es). View
3.45 KB

Two super duper minor points, updated the patch.

  1. +++ b/core/modules/node/src/Tests/PagePreviewTest.php
    @@ -116,13 +116,31 @@ function testPagePreview() {
    +    // Get the uuid.
    

    I think this should be capitalized - UUID. Although there are some like this already in core.

  2. +++ b/core/themes/bartik/css/style.css
    @@ -847,6 +847,48 @@ ul.links {
    +.node-preview-container {
    +    background: #d1e8f5;
    +    background-image: linear-gradient(to bottom, #d1e8f5, #d3e8f4);
    +    font-family: Arial, sans-serif;
    +    box-shadow: 0 1px 3px 1px rgba(0, 0, 0, 0.3333);
    +    position: fixed;
    +    z-index: 499;
    +    width: 100%;
    +    padding: 10px;
    +}
    +.node-preview-backlink {
    +    background-color: #419ff1;
    +    background: url(../../../misc/icons/000000/chevron-left.svg) left no-repeat, linear-gradient(to bottom, #419ff1, #1076d5);
    +    border: 1px solid #0048c8;
    +    border-radius: .4em;
    +    box-shadow: inset 0 1px 0 rgba(255, 255, 255, .4);
    +    color: #fff;
    +    font-size: 0.9em;
    +    line-height: normal;
    +    margin: 0;
    +    padding: 4px 1em 4px 0.6em;
    +    text-shadow: 1px 1px 0 rgba(0, 0, 0, 0.5);
    +}
    +.node-preview-backlink:focus,
    +.node-preview-backlink:hover {
    +    background-color: #419cf1;
    +    background: url(../../../misc/icons/000000/chevron-left.svg) left no-repeat, linear-gradient(to bottom, #59abf3, #2a90ef);
    +    border: 1px solid #0048c8;
    +    text-decoration: none;
    +    color: #fff;
    +}
    +.node-preview-backlink:active {
    +    background-color: #0e69be;
    +    background: url(../../../misc/icons/000000/chevron-left.svg) left no-repeat, linear-gradient(to bottom, #0e69be, #2a93ef);
    +    border: 1px solid #0048c8;
    +    box-shadow: inset 0 1px 2px rgba(0, 0, 0, .25);
    +}
    +.node-preview-backlink::before {
    +    content: '';
    +    width: 10px;
    +    display: inline-block;
    +}
    

    4 space indent here.

webchick’s picture

Great!! This is working exactly as expected now! :D

I did accidentally find one other bug in testing: if you're at admin/content, and select "Edit" next to a node in the list, then click "Preview," instead of being shown the preview, you will be taken back to admin/content and your changes lost. I suspect that's because the URL you end up on is http://8.x.local/node/1/edit?destination=admin/content and this somehow screws with the preview system's use of URLs? Hm.

I would love it if that could get fixed prior to commit since it's a data loss problem if it happens to you, but OTOH it's a pretty narrow edge case, so I'd also be comfortable with this going in without it and handling it as a follow-up.

So if it's easy, let's fix that here. If not, then I'm happy to mark RTBC. While I haven't given the code a thorough looking-over, I see that numerous other people have.

tim.plunkett’s picture

I think that was supposed to be handled already, but that code was removed in #274...

swentel’s picture

Oh yes, the destination parameter, we somehow lost that in the patch somewhere, I remember we used to store that property as well.
It's not /that/ hard to implement, but I'm AFK until monday. If we can wait that's fine, unless someone else takes that up. Also more than happy to fix that in a follow up. You can even assign it on me immediately :)

(crosspost after Tim)

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Ok, let's do this then. :)

Basically all core committers except me are out of pocket for the next week, so if catch hasn't committed this by the weekend, I'll do so.

swentel’s picture

  • webchick committed d72c0f9 on 8.0.x
    Issue #1510544 by swentel, Bojhan, Gábor Hojtsy, merlinofchaos, Cottser...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, 48 hours at RTBC has passed, let's get this sucker in. :) Gave it one more pass-through and everything looks copasetic apart from the issue we already have a follow-up for.

Committed and pushed to 8.x. HELL YES!!! :D

Let's all build swentel a statue in Amsterdam, ok?

jibran’s picture

swentel’s picture

Rock! My phone suddenly exploded with all the mentions, that was awesome :)

Added another tiny follow up, see #2326727: Delete tempstore entry of preview in submit - a very simple racing condition.
(note, I actually knew about this one already, but I was really scared to hold this one even longer. Assigned it to me, will fix next week, it's really easy)

Dave Reid’s picture

Do we have a strategy for hiding certain view modes that don't correspond to HTML output, other than RSS which is hard-coded to hide? Nearly every site I've touched has added an additional view mode like 'JSON' or some kind of XML standard that is used to display the node.

nod_’s picture

This is messing with the back to site link, will need to be fixed same as the block demo page I think (since we don't have a ?destination= for this).

andypost’s picture

Do we have a strategy for hiding certain view modes that don't correspond to HTML output, other than RSS which is hard-coded to hide? Nearly every site I've touched has added an additional view mode like 'JSON' or some kind of XML standard that is used to display the node.

There's no - please file one, currently core has the trouble with print and search* view modes

swentel’s picture

@Dave Reid Currently only it's hardcoded to rss and search index - could be done more elegant indeed.
@nod_ : ow, good catch.

Ping me the follow ups if they are created, I'll follow those up.

herom’s picture

Followup issue for RTL fix: #2326837: Fix content preview RTL CSS.

David_Rothstein’s picture

This is a really great improvement in Drupal's preview functionality! But I think we should have a followup issue to address the ideas of @sun and others (because this still isn't a "real" preview that actually shows people what their content will look like on the site). To address one of the last comments above related to that:

We've explored saving temporary revisions many times before. The main problem is only files have a garbage cleanup operation, where the files that were uploaded for a preview which was then not saved are later removed. Other items, like menu items or taxonomy terms, which would be created with this preview would need a garbage collection feature as well and be able to save as temporary as a consequence.

I don't see this as being necessary at all. If you create an unpublished revision in Drupal now and then delete it without ever publishing it, any taxonomy terms etc. which you created during the process don't get cleaned up. This new preview feature would be no different. The key is just that the content creator needs to be aware when their draft content is being saved (whether that's as simple as a "Save draft and preview" button or something more complicated involving autosave).

David_Rothstein’s picture

webchick’s picture

Thanks, David!

Status: Fixed » Closed (fixed)

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

bendikrb’s picture

I have just created a module which renders the actual page with the node currently being edited.
The UI is 100% javascript, and is attached to the default "Preview" button in the node edit form.

https://www.drupal.org/project/live_preview.

I see this task has been moved to 8.x, but I thought it was worth mentioning. Hope it's not misplaced.

nicholas.alipaz’s picture

Sorry to dig up this old issue, but was there ever a follow-up issue created regarding using Full in place of Default view mode whenever Full is enabled?