Jump to:

Problem/Motivation

The current behavior of edit-in-place forces a user to make single-field edits. Each field update results in an incremented revision on the field's entity.

By allowing changes to fields to be stored temporary, we let an author make multiple field edits that can be committed with a single save action. In this way, in-place editing will no longer create a flood of entity revisions that correspond to just single field edits.

Proposed resolution

We will move the save and cancel operations to a toolbar associated with the entity, not a field of the entity. A user will be able to put an entity in to edit mode, select a field of that entity to edit, make changes and move to another field. The changes to a field will be stored on the server side in a tempstore entry.

Pressing the save button in the entity toolbar will commit all of the changes from the start of the entity in-place editing session. Pressing the cancel button will return the original values to all the fields of an entity that had been editing in place and no changes will be maintained.

Tasks

Finished

Entity toolbar introduced with the EntityToolbarView and EntityView classes.

Field-related toolbars (such as a WYSIWYG toolbar) are now rendered with FieldToolbarView inside the EntityToolbarView element.

Users can move from an active, in-edit field to a candidate field. Changes to the previously active field are pushed to tempstore invisibly when switching fields.

PASSED | Enable in-place editing / activate a field / change the content of the field / press the entity toolbar cancel button / press the save button in the confirmation dialog

PASSED | Enable in-place editing / activate a field / change the content of the field / press the entity toolbar cancel button / press the cancel button in the confirmation dialog

PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field / change the content of the field / press the entity toolbar save button

PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field, do not change its content / press the entity toolbar save button

PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field / change the content of the field / press the entity toolbar cancel button / Press the confirmation save button

PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field / change the content of the field / press the entity toolbar cancel button / Press the confirmation cancel button

PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field, do not change its content / press the entity toolbar cancel button / Press the confirmation save button

PASSED | Enable in-place editing / activate a field / change the content of the field / activate another field, do not change its content / press the entity toolbar cancel button / Press the confirmation cancel button

PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / press the entity toolbar save button / The field should me marked as invalid

PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / activate another field / press the entity toolbar save button / Save should fail and the first field should me marked as invalid

PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / activate another field / press the entity toolbar cancel button / Press the confirmation save button / Save should fail and the first field should me marked as invalid

PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / activate another field / press the entity toolbar cancel button / Press the confirmation cancel button / All fields should revert to their original values

PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / activate another field / change the content in that field / press the entity toolbar cancel button / Press the confirmation save button / Save should fail and the first field should me marked as invalid

PASSED | Enable in-place editing / activate a text field with char limit 2 / change the content of the field to contain 3 characters / activate another field / change the content in that field / press the entity toolbar cancel button / Press the confirmation cancel button / All fields should revert to their original values

User interface changes

This patch introduces a floating toolbar that is anchored on the entity and its fields.

Original report by webchick

This came up in demo call today. This would allow for much faster data entry.

Here is a first patch for this. I think I'm very much "butchering it", or in other words, I feel like I'm an elephant in a porcelain shop pulling the pieces I need with little respect for the whole architecture. In retrospect I don't think I'm the right person for this, even though it seemed like it would be easy enough to do, it certainly requires lot more JS experience to do it well, of which I have least on the Spark team.

Anyway, the attached patch implements:

- addition of a gray/blue button titled "Save" on the toolbar's shortcut bar area
- click-action assignment of this to save the current editable as appropriate (I suspect this click event assignment might happen in a chain but I could not reproduce it actually ending up with multiple revisions saved, so not entirely sure)
- hiding of the per-editable save button (since some parts of the code referenced this and I did not want to remove those just yet)
- the global save button starts off as gray and becomes blue when you make changes according to the same logic as before
- remove the confirmation step when you click out of things and just doing the action instead of asking the user

I think the changes should be definitely enough to user test the new behavior but the code changes definitely need some more cleanup.

1) The functionality works well.
2) The save button is not very findable right now. I think it should be more visually prominent at all times?
3) The code indeed needs to be cleaned up, but that's probably at least partially also because all of Edit's JS should be refactored: features have been removed that have some left-behind cruft, and that will make this easier already.

I think it's a good first stab/step.

However, AFAIK the goal was to also remove the cancel button, and to have field changes save automatically (without the need to click this "Save" button), with each time a new *draft* revision being created. The cancel button would go away. We'd have a list of revisions and a way to revert to any of those revisions, as a replacement for the ability to cancel. And instead of "save" (which immediately publishes), we'd have a "Publish" button, to publish the draft revision the user is currently looking at.

All that being said, if this was just a first step to get the Save button into the toolbar, I think that's fine :)

We might want to hold off committing this, not because of code quality, but to not divert Edit's codebase from the Create.js-based fork of it on Github (https://github.com/wimleers/edit-createjs), until a decision is made on that front. If you prefer to commit this, that's fine too though :) But AFAICT, that doesn't "fix" this issue.

Well, the explanation at http://drupal.org/node/1702250#comment-6356512 did not really have details as to whether the cancel button should also be removed (and indeed currently there is no better way to cancel out of a field without the revisions dropdown implemented). As for the difference between Save and Publish that is only possible to distinguish if Edit module DOES require one of the workflow modules. If none of the workflow modules are installed (or the one specifically picked to be required), then it is not possible to have unpublished revisions with a published revision being active. So implementation for that would need to integrate with a workflow module like that.

I just want to make sure I fully understand this: the edit.module-UI will switch to a "Central Save"-button rather than providing per-field buttons.

This actually happens to be closer to how createjs handles it and will probably simplify the required (glue) code considerably.

I haven't looked particularly closely at the "Drupal-end of things", but how are we going to handle persisting multiple "fields" at once? Is there a more elegant way (e.g. provided by fape.module) than keeping the various loaded forms "around" and submit them in a sequence on "save"?

Nope. And that's the tricky part. As I've said before: we need to be able to have instant previews of the modified fields. So even if we don't "really save" a changed field, we really do want to show the user immediately what that'd look like. It seems the only way to achieve that in a reliable manner is to leverage TempStore, which will hopefully also power full node previews. If the full node preview based on TempStore problem is solved, then we should be able to piggyback on that work as well.

On November 19 (Monday), I conducted a usability study on the new iteration of edit-in-place for Spark that focused on:

Editing a piece of content

Saving/ Publishing a piece of content

Going back to a previous version of the content

The study was to be conducted with 5 content creators/maintainers. However, after 3 sessions, the issues were clear and the researcher is confident that more sessions would not yield any new insights.

Executive Summary:

Participants easily understood how to edit their content (title, body). The effectiveness rating for the task was the highest (5 out of 5). It is also worth noting that the task rating has gone up to 5 from 4.2 (previous design).

However, saving/ publishing content (rating of 2 out of 5) and revision history (rating of 2.1 out of 5) still needs work.Major issues:

Does “Revert” auto-publish my content?

Does “Save” auto-publish my already published content?

Minor issue:

Up/down arrow on revision control not useful

Detailed Findings

Participants were thoroughly confused if revert was changing their published content in real time and hence gave poor ratings to the task (2 out of 5, 5 being best). This is because of the following reasons:

Participants expectation is of two step process: view the revision, and then revert the change

The current prototype doesn’t provide any user feedback if the content is reverted

The “Current” and “Published” on this section of the UI is often confused or not noticed, adding more confusing to the problem

“I don’t know if it is published or is it just viewing [the revision]… I have no idea what is this telling me.” (P3)
“I want to see preview or show difference. Also, without show difference you can’t see the changes that you or someone made to the revision” (P2)

Requests to revision control:

Show difference

Username

Log message (nice to have)

2. Since this article is already published, is hitting “Save” auto-publishing my content?
Participants thought that saving fields is saving the page and since it is already a published page, it is also pushing the “saved” changes LIVE.
Reasons for this problem:

Unclear terminology: “Save as a draft” is more clear

“Publish” is buried below the “Save” drop down

“Save” sometimes is deactivated (perhaps, a prototype artifact)

"I don't want this on my prod site" [P2]

3. Up/down arrow on revision control not useful [Minor Issue]
2 of the 3 participants did not immediately understand what’s the purpose of up/down arrow on the revision control. Once they did understand, they thought it was “useless” as it is easier to click on a particular revision from an exposed drop box than to go to one revision up or down.

Other issues:

"Quotes" quotes the entire block. Participant expected it to quote only the selected area (1 participant)

What happens when there is more than one person editing a piece of content at the same time? (1 participant)

Observations

Participants do not know what “Advanced Edit” will do. However, since the cost of click and explore is low, it is not categorized as an issue yet.

Participants did not know notice “Saving title” or “Saving body” notifications on top of the page.

This is definitely worrying, I have seen earlier iterations and it seems like this is a tough nut to crack. The overall trend seems to be that it is hard to communicate to users, that your "front end" isn't necessarily what your visitors see but could just be a new revision. That whole concept, seems hard to communicate - and rightfully so, we generally keep the front-end reserved for the end result unless you make an explicit step to "demo" something.

I guess the whole idea of "please don't forget to press save" what we have on blocks, now just turns into "please don't forget to press publish". Partly because its not really clear, what the version is you are looking at "Current" (as of now does not signal that) and that you need to "Publish" to get it to show to visitors, "Save as draft" will do something to help there but there conceptually will still be a disconnect - there is no large "you are not looking at the live thing". The fact that you uncovered even more issues with reverting, to me only sounds natural that is quite tricky functionality. Concrete 5 for example, has a really flow around this all where there is a clear "preview my edits" and "publish my edits", where preview allows you to continue, publish just does what you mean and there are additional links and forms for revisions, deleting etc.

We use "advanced edit" as a label for the full editing interface, I am not sure about it could scare people of from the only interface you can add it to a menu, add revisions info etc. "Quick edit" is an established label, for inline editing I would love if we could use that.

In general I am wondering if this might be trying to tackle something, that simply doesn't fit this paradigm. Although I am sure we can come up with something that, makes it a little more clear. I am not sure if this inline editing fits audiences where its important to care about publishing workflows - e.g. how does this scale when you have many revisions, authors reviewing, several states the content can be in, and diffing (you would replicate a whole screen in one tiny area). Its probably worthwhile to ask those in such settings, whether they'd deploy this beyond sexiness.

Wouldn't we be much better of, just supporting inline editing and pushing straight to published? And allowing contrib to come up with a model to support more advanced workflows. I guess this is not a popular question, but I am having difficulty following who we are designing this particular part for and it could greatly simplify it for the other usecase.

I like the idea of in-place editing always putting alterations live. So 'advanced edit' might become 'revise' and 'edit' (i.e. IPE) becomes adjust/tweak/amend or whatever. So you have separate permissions for those who are allowed to amend, which allows one to make immediate published tweaks, as well as revise - revisions which might or might not be subject to a moderation workflow.

As far as 'save as draft' goes a 'draft' watermark might be helpful, I think a lot of people are accustomed to seeing those and it might help with the UX:

So it becomes a bit like the PDF workflow, you build your document in word (i.e. bit like the Drupal nodeform), once that's published and becomes a pdf, there's certain amendments and tweaks you can still make inline on the published pdf. If you want to do anything too spectacular though, you have to open up the original word document once more and make your changes, then re-publish it.

That's a process many people might be familiar with if you can make the workflow feel like that.

As mentioned in #16, Tempstore is a great way forward for holding work in progress. Then a user clicks on a button the toolbar to permanently store the changes in progress. To me, this is much preferable to storing many revisions while editing the fields in an entity.

#25: We'd first need to investigate whether that's feasible at all; we still must be able to re-render changes "saved temporarily" to the TempStore for the end user.
(Or: pretty much what I said in #16.)

Seems it should be possible to configure that, no?
A little checkbox that asks whether you want edit.module to create a revision each time you save something and if you enable it, a (token-enabled) text field appears where you can enter the automated revision log message.
I think we can't get away with hard-coding either option; i.e. there are use-cases where revisions are completely unneccessary and only clutter the DB and the UI, and use-cases where they are integral to the workflow.

@David_Rothstein: while it definitely looks like revision handling is not intended to be part of the scope of this issue, I can assure you it is. E.g. see the design in #6, at the top it has a "Last saved" dropdown, which are revisions, listed by the time they were saved. Also see the related issue in the issue summary, which were and are still to be consolidated.

Moving this to Drupal Core, also changing the status (although probably someone will revert it anyway). It is a critical bug, because currently essentially you cannot make changes using Edit without directly publishing them -this is a feature regression. Since it is enabled by default, we will expect a large part of our audience will use this - not supporting any kind of workflow, is quite a problem.

Yhea, honestly - for some reason that was opened as a new issue and its pretty much all about what we intended to fix here. I am open to closing this all and just adjusting the priority of the other one.

We just had an epic phone call with Dries on this issue and here was the outcome (I think; others on the Spark team, feel free to correct me if this is wrong):

- Saving should be moved per-entity, rather than per-field... UX tests already back that up and it didn't seem like there's much disagreement on that point.
- This means we'll need to save incremental edits between fields to a "tempstore" type of thing.
- Only upon clicking the "Save" button on a floating "entity" toolbar will actually commit the changes to the database. What happens here will depend on how the content type is configured: publish by default, create a new revision, and/or save directly over the existing revision. Edit module just does the save, and doesn't need to worry about it.
- There was also general agreement that Edit module shouldn't become a workflow management module... by itself, it should not be providing the capability to save things as draft or what have you. It's simply a "What you see is what you edit" kind of module. If you're viewing an published node, it will edit the published node. If you're viewing an unpublished draft, it will edit the unpublished draft.
- What it should do though, is warn you if there are unpublished revisions ahead of yours and invite you to visit the full edit form what that revision loaded. This is also the workflow if you want to save as draft. Don't use quick edit. Use the node edit form. Quick edit can make subsequent changes, however.

Kevin's been actively working on designs for this, should have something to show next week.

There have been several design iterations some testing, an invision prototype and a good deal of discussion in the usability group on this since my last post but I *think* we are coming to a consensus (at least in Spark and the usability group) on the current approach, described in this prototype: http://www.acquians.info/Public/Edit/spark/blocks/index.html

A few things to note:

The prototype is not cross-browser optimized. It should be viewed in chrome

This is not for code review—prototype code is throw-away

The only editable element is the node (Flannel banjo ennui)

Not all interactions or features are present in this prototype

The prototype is to validate the interactions of editing a node in place (not blocks or other things on the page)

Features that are not yet in the prototype (but should be soon)

The toolbar should be sticky to screen top and bottom on scroll.

Close without saving modal state should include a scrim over things outside the node to make it clear that it's a modal

We have been following this, in the group and IRC - thanks for keeping is so involved! I discussed this with yoroy, it seems like we arrived at the right concessions to create a easy to use interface. It's also very useful to have the frame of mind as you noted it down. We believe that we can push forward with this :) - I hope we can quickly get some data to see if we are in the right direction.

We shouldn't try to solve everything, I think a lot of features you note are also possible in followups or an actual patch if its too much work to prototype - with the time frame for release, we should keep in mind that this is one of many important UX issues that still need to be resolved.

I worry about the styling, because its not really in sync with what we have been doing with the style guide (round corners, connected elements, typography, color use age) - however that is also something that can be dealt with separately. The same goes for the new contextual link styling, it feels like that could be a separate issue.

Yes — IMVHO — we should focus on functionality now and worry about consistent styling/presentation later.

Edit has gone through so many evolutions that IMVHO we should first get all functionality right first, spend as little time as possible on styling while reaching that "feature complete" phase, and only then work on finalizing/improving the styling & animations after that.

I'm glad the response is so positive :) Hopefully we can now move forward very fast on this.

I worry about the styling, because its not really in sync with what we have been doing with the style guide (round corners, connected elements, typography, color use age).

This relates to another issue in that admin tools that appear on the front end need a systematic way to inherit the admin theme styles. Jesse solved this for toolbar but I'm not sure if that extends to other modules or not. I will ask today. If we can accomplish that then the module's theme css can be overridden by 7 (or any admin theme).

For the moment I will adjust the radius and colors to match the new 7 style guide.

That's right: the style guide Bojhan refers to is for Seven, i.e. the back-end/admin theme. But this is on the front-end, not the back-end! It's impossible to automatically inherit Seven styles, so it's at the very least very questionable whether we should enforce Seven-like styles in Edit. It would look weird when you were using a different (non-Seven) theme…

Both issues should be able to progress pretty far independently, in parallel.

Gábor will be working on the other issue, Jesse will be working on this one. I will be working on making Edit work with other entity types than nodes, with the "extra fields" (node title/author/date) that aren't proper Field API fields yet, and on making Edit work with Views.

I recently conducted a moderated usability study on the latest Spark prototype. The focus of the study was:

Can users find a way to edit to their content?

How do users edit the title and body of their content?

Do users understand how to make their edited content live?

How was the experience of using this prototype?

If there is anything missing or confusing?

Participant Information

Number of participants: 4 (New Drupal Users - Acquia Internal)

Participant profile: Participants who create/ edit/ maintain content on websites or have a need to do so in the near future. Although they can be site builders, participants who are only content maintainers will be preferred

Summary of findings

The prototype received an overwhelmingly positive response to the ease of use and effectiveness of inline editing and created a delightful experience for its participants and got an average effective rating of 4.8/5 (this is perhaps the first time I am using “delightful” in my Drupal reports). I would encourage you to read the participant quotes at the bottom of this comment.

Positives - Participants Understood

1. How to find “Edit” and pencil icons to get into the task (4 out of 4 participants). The use of universal symbols was appreciated.

2. How to edit a field (4 out of 4 participants)

3. That X is going to discard their changes (3 out of 4 participants)

4. How to get to full edit page (3 out of 4 participants)

Issues

High Issue: Participants who are new to Drupal/Gardens (3 out of 4 participants) either thought that save is going to generate a draft/not publish or weren’t sure what’s going to happen. This issue is exaggerated, as the UI doesn’t provide user feedback after the changes are saved.

“It has to be blatantly obvious that it is going to publish or not.” (P2)

Ideally, the “Save” on inline editing should mimic the “Save/Publish” interaction on the full node edit page. To re-iterate from previous studies, “Save” does not imply “Publish” to especially new/beginner Drupal users.

Medium Issue: Participants like that the UI provides the number of unsaved changes but they thought it shorts fall in terms of functionality since it doesn’t highlight the changes. (I understand there are technical limitations to do this but I think it is good for us to be aware of the issue.)

Minor Issue: The orange/blue outline to the fields that are changed/unchanged is too subtle and doesn’t receive user attention. Participants weren’t completely sure why they were in different colors. I would say this is a minor issue since this is aimed to be a more gradual over the period of time learning functionality.

Participant Quotes

1. “This is a huge advancement in comparison to Wordpress and Joomla.” (P3)

2. “I have never used anything like this before.” (P4)

3. “I have never done this before and was able to do it easily ...it’s pretty evident the people behind this project knew all the options and made it easily available to me.” (P4)

4. “I can jump into this and create a website within a week. It looks seamless … UI looks pretty good” (P1)

That save/publish distinction is indeed a biggie, which has come up in other tests, so we definitely do need to solve that. Kevin suggested a small tweak to the button wording of "Save and publish," which I think works. He's also playing around a bit with the visual affordances on the fields to make them a bit clearer.

It sounds though like this general direction is going to work UX-wise, so we should be able to start on implementation in the next day or two.

Thanks for sharing the results, its great to see testing so quickly. This does seem to confirm the direction, so lets move forward on the more detailed stuff.

I think the High issue, mirrors a problem that we kept finding on the content creation page. The "Save & publish" solved this problem. I think a issue, is other workflows - which we are not handeling here. I imagine however if the node is unpublished, it should just say "Save" ?

The medium issue. I really love this detail in the design, I agree that we would like to offer more information on which fields arn't saved and if possible using revision to show what changed. I guess, that is going to be real tricky - at the very least we can do a indication that shows which fields are not saved. Like a fade in?

The minor issue. Having watched the videos, I am not quite sure if using just color is a good indicator for this. Keep in mind for a11y reasons, we can't have just color as an indicator anyway. Ideally we have an indicator that we can use throughout core to indicate a change, has not been saved yet - for this we currently use a yellow background with a orange astrix. As Dharmesh mentions though, this pattern too requires some learning. I am not sure if for a feature like this, which is very front-facing that is a good end solution. But this is definitely not the only place we battle with this problem.

I am keen on seeing this implemented further, I imagine there are a bunch of issues that will come up. For example, needing to shorten node titles in entity toolbar because they might become super long (especially hurtful on mobile), integration with our existing pattern for contextual links (I am not quite sure we should break that), unpublished content, several content items on one page, etc.

re: "I imagine however if the node is unpublished, it should just say "Save""

I would think "Save Draft" would be clearer as to what it was saving (since 'Save' alone could mean publish too). Also not sure how Publish doesn't imply Saving, so why not "Publish" instead of "Save and Publish"

Great test, even greater to have the videos to draw conclusions ourselves.
Good choice you resorted to the pencil icon. Across all Content Management systems, web applications etc. it appears to have gotten some kind of standard. So having a pencil icon on something should give a lot of people the notion they can edit this particular piece of content.

I also liked the Save button becoming blue when there is something to save.
I wish the switch to edit mode on a particular field or entity would be that smooth in core :P
It is encouraging the first participant (only watched one) found the edit icon in the toolbar. I was always worrying the toolbar was too cluttered and people might not find that most important button.

The Publish / Save thing is not so easy. Maybe the Dropbutton is the best choice. There has been a lot of back and forth concerning if two buttons rather confuse than inform (I remember finding it confusing and scary on Wordpress when they had that big prominent "Publish" button.) With the dropbutton, you have "Save and publish" or some other words that turn out to be least confusing in the end. As the dropbutton is a UI element used throughout Drupal in a fairly consistent manner people will eventually get at ease with it (So I hope :) ).

I am more inclined to "Save Draft" and "Save and Publish". That being said, I am not opposed to "Save Draft" and "Publish" as it conforms to other CMS workflows and users do understand that "Publish" will save their content.

There's no ability to save a draft from edit in place so the only option is "Save & publish"

There has to be that ability actually (not as a separate option, but as one of the possible behaviors on clicking "Save", depending on the current state of the content). Like it says in #37:

It's simply a "What you see is what you edit" kind of module. If you're viewing an published node, it will edit the published node. If you're viewing an unpublished draft, it will edit the unpublished draft.

So it won't ever be able to take a published node and turn it into a draft, but I'm not sure that matters for the button label. The user probably still needs some kind of visual reinforcement since they won't know what rules the module is following behind the scenes.

#57: No! David Rothstein is saying that if you in-place edit a published node, that "Save" effectively means "Save and publish", and that if you in-place edit a non-published node, that "Save" effectively means "Save as draft". So: leave the node in the same "published or not" state.

It should *not* be possible to modify the "published or not" state from in-place editing. That's what "Full edit" is there for.

I think 'Save', or 'Save changes' is sufficient, people might have all sorts of weird workflows ('second draft', 'pre-publish' and on and on). If people want to give more clarity to what workflow state the content is in, doesn't seem unfair to expect them to hook and tuck and accomplish that themselves.

'Save changes' works well for me, i.e. I've just made some changes on existing content, do I want to save those. But Save would do ;-)

[]...but I'm not sure that matters for the button label. The user probably still needs some kind of visual reinforcement since they won't know what rules the module is following behind the scenes.

When it comes to IPE, I can see lots of what seem to me to be quite nice ways to communicate the workflow in the theme, rather than in the buttons. Different coloured backgrounds with a key, maybe a DRAFT watermark as I mentioned earlier. Basically, using the strength that we have content here and we're not bound to just fiddling with forms. That's out of scope here though obviously, but could pave the way for some nice admin-enhanced themes?

This patch is a shot across the bow to get us rolling. It's very very very very very very very rough. In fact, the only thing you'll see is a red dotted line around the entity when you enable quick edit on it.

I talked architecture with Wim this morning and we've decided to move forward with an attempt to refactor Create.js out of Edit. The entity-level toolbar designs are not supported by the assumptions in the architecture in Create.js. We would end up writing a lot of glue code to bond these designs to the current codebase and it would be nothing but brittle.

The refactor will take the hybrid jQuery-UI/Backbone approach in Create.js and move it to a pure Backbone solution. Create.js has made sense for our use case up until now, but as we continue to refine the interaction, we must always consider the UX we want vs. the code we have. The nature of agile programming is that each time you touch code, you refactor it to be better and meet your current use case. That's what we're doing here.

I really like that you can edit multiple things on the page without having to click save after each change, hopefully this will work properly when editing multiple pieces of content on the same page, for example two different nodes.

I don't like that when showing the editable areas that the node title, body and tags didn't each have a separate pencil icon and that after clicking the pencil a second click was required to select the specific field to edit. It should just be a pencil icon for each field and clicking it take you straight into editing that field. Or even better not having to click the pencil icon and just click exactly where you want to start typing. A single click, type, save, simples.

I also didn't like that at no point was the question asked "Would you like to save the changes as a draft or just publish the changes immediately?". Perhaps we could use the a workflow that users should already be familiar with and that is one of when saving a new word processing document. The first time an unsaved document is saved the question is asked "Where would you like to save this file to?", then subsequent saves will automatically use this same location. So we could have a similar flow, first time save is clicked ask the question, user edits some more and saves again, don't ask just use same location, user exits edit mode, then next time edit mode is entered again they must again answer the question on the first save.

I don't really mind that the save and close buttons follow the actively editing field around the page, but it would feel easier to use if the save button would just stay in the toolbar next to the edit button and when scrolling down the page the toolbar would always remain visible, pinned at the top. Then clicking save could even automatically take you out of edit mode.

This patch introduces an EntityModel that drives both the ContextualLinksView and the EntityView; it influences the EditableView. The EditableView now has a primary model, the EditableModel, that I am slowly porting from the Create.js widget.

I don't like that when showing the editable areas that the node title, body and tags didn't each have a separate pencil icon and that after clicking the pencil a second click was required to select the specific field to edit. It should just be a pencil icon for each field and clicking it take you straight into editing that field. Or even better not having to click the pencil icon and just click exactly where you want to start typing. A single click, type, save, simples.

Note that what we're implementing here was usability tested at #45. We will not deviate from what was tested there, definitely not such big conceptual changes.

To actually answer your suggestion: what you're suggesting requires a whole sea of pencil icons/contextual links. That's very, very bad for UX. You start to in-place edit an entity, then you can click on any of the fields to in-place edit them. Period.

Please just bear with us as we implement this; the current patch does not yet fully implement what was tested in #45. The issue was not marked as "needs review", so please save your energy for later, when we will actually need reviews :)

@Wim Leers, I think you're misreading @akamustang's comment. He wasn't reviewing the patch; rather, he was reviewing the videos. And although one of the alternatives he proposed would result in a sea of pencil icons, his preferred alternative would actually result in removing the sea of pencil icons (or maybe more like a large lake?) that is already there.

I watched some of the videos and participant #3 actually experiences the exact issue that @akamustang described: After clicking Edit in the toolbar, he tries to click on some text on the page to edit it, but can't. However, he does notice the pencil icon pretty quickly after that and figures out to click it, so maybe it's not a huge problem. (Also, this feedback is at least as much related to another issue - such as #1874664: Introduce toolbar level "Edit" mode that shows all contextual links - as it is this one. I have some expanded comments about the videos I will leave on that issue as well.)

***

However, the thing I couldn't figure out from the videos that is related to this issue is that the prototype that was tested doesn't have contextual links at all. So I didn't really understand how the design that's proposed here (for the floating entity-level toolbar with the node title and "full edit" link in it) is supposed to interact with the contextual links (in particular the "quick edit" link, but also any other contextual links that normally show up when you click on the pencil icon). Did I miss where that was discussed?

In other words, why can't the server-side code here just use Drupal's standard node preview functionality to render the updated node and return it to the client side (without ever saving it)?

I haven't looked deeply into the code so I don't know if that's a stupid question, but I'm asking it anyway on the off chance it's not... It seems like it would be a much simpler way to deal with the backend half of this critical task, compared to getting into all the complexity involving TempStore. TempStore is necessary if you care about "oops my browser crashed and I lost all my work and want to get it back" but that's not related to this issue, nor by definition is it that important for something labeled "Quick Edit" anyway :)

I've managed to get the entities and editables driving view changes from models for each. Activating quick-edit on an entity activates it. Clicking on a field activates the field. Only one of each can be active at a time and we're using change listeners on model collections to enforce this rule rather than keep a list of DOM elements or widgets and then invoking methods to disable them. All disabling happens by manipulating the state of the entity and editable models.

I've managed to cut Create.js out of the widget business. It's still in there for the temp storage, but I don't think this will last long either. My next task is to get an editor and toolbar view to render when an editable is activated. I'm generating the Views for them and they're hooked into the right models. I just need to port the methods and update the rendering. Soon we'll no longer have code like this.options.widget.options.editor to decipher.

This patch completely removes create.js and the jquery.ui dependency from Edit.

It's still not clean. oh no. There's lots of code to remove. But even now, that stats are looking good.

22 files changed, 2962 insertions(+), 3470 deletions(-)

I've managed to get the editor and toolbar views to appear. Fields can be changed and saved and the save does succeed. I haven't plumbed through the model updates for when a save is finished or when an editable is turned off. All of the code to do that is there, I just need to wire it up to a triggering event and remove some vestigial create.js code.

The file structure can probably be improved, too. Right now most of the classes just live in edit.js. Once the refactoring solidifies I'll break this into smaller files.

Now that we're on a proper Backbone framework and out of the land of jQuery UI widgets, we can start moving on the entity-level toolbar view.

It depends on VIE.js, which does five zillion things we don't need; it assumes RDFa (which is "not correct/accurate enough" in Drupal, see earlier issues) and JSON-LD (which won't be in D8 core after all). It's *huge*. 34 KB minified! We're currently only really using a tiny, tiny subset of VIE.

Create.js deeply depends on jQuery UI Widgets, which are a huge PITA to use. It's extremely confusing. We've been wanting to refactor that out of Create.js, but that is a *huge* undertaking. And it's not clear how we'd retain backwards compatibility, which is a requirement.

https://github.com/bergie/create/issues/133, https://github.com/bergie/create/issues/137 -> Create.js doesn't have per-field state change handling, like we need. Instead, we've been pretending for all the time that Create.js has been in core that each field is an entity, which is obviously not the case. Now that we need entity-level state AND field-level state, that breaks down. It's possible to hack around, but… it requires a significant change in Create.js and it will break backwards compatibility. Bringing those changes to Create.js (in a BC way, mind you!) is likely going to be at least equally complex as moving away from Create.js. And that's not the only issue. And we'd still be fighting jQuery UI Widgets. Etc.

Overall, Create.js makes *way* too many assumptions. Because it was originally built as a specific implementation of in-place editing, with its own UI. Whereas it now claims to be a platform for in-place editing. That's not really true yet.

It's abstraction for in-place editors ("PropertyEditor widgets") is not really an abstraction. It, too, makes way too many assumptions. It does event binding+handling, which it should not do *at all*. That's why we've been unable to reuse or even just subclass Create.js' PropertyEditor widgets.

With all of the above, the removal of Create.js and particularly the removal of VIE.js would result in a huge decrease of the amount of JS to be downloaded, parsed and executed for in-place editing.

I don't think it's final yet, we're seeing what it would look like if we refactored Create.js out of Edit.

It is almost completely broken for me, but that is to be expected. Saving works, but the re-rendered result does not show up. All of the state handling is broken.

Remarks

I'd like to move back to smaller files instead of one massive file ASAP. This really impedes fast development. :(

I think the (red dotted) outline around the entity is not intended to become part of the end result; it's just here temporarily?

We should rename "editables" and "EditableView" to something more descriptive, that indicates that these are fields of an entity.

Conceptually and also from a code POV, it makes little sense to have Drupal.behaviors.edit.collections.editables; a collection of all editable fields on the page. An independent field is never editable; a field within an entity is. So I got rid of this collection, and instead created a "fields" collection inside each EntityModel: a list of all editable fields for this entity.

Changes in attached reroll

I brought back the views already, which has brought down the size and number of changes quite a bit.

State change handling is working again. Editor, Toolbar and Property Decoration views are now all instantiated JIT.
Note that I explicitly omitted the "if the node 1 title changes, then it should also be updated elsewhere on the page" part. It's only relatively rarely seen, and it complicates things. Let's do that in a later stage of this refactoring.

Got rid of VIE.js, but only partially, by obsoleting editable.js (i.e. getting rid of Drupal.edit.Editable, as you suggested in chat). The "rerender the changed field" part has not yet been implemented. Saving is broken. I focused on fixing the state handling, which needs to work first before we can really fix this properly.

The "form" in-place editor now works completely: it loads the form, reacts to modifications in the form, the save/close buttons work, the modal for confirmations works.

Next: make the other in-place editors work, work on getting the saving to work again, explore the use of Backbone.Model.validate to implement the acceptStateChange callback, so I can get rid of the EditableModel.setState(X) method I introduced in the previous patch, whereas you should be able to just do EditableModel.set('state', X).

Cleaned up active entity handling/enforcing that there is only one active entity; now back in line with what Jesse did in #71. Much cleaner :)

The other in-place editors are working again; or at least the "editor.module" one is, so CKEditor shows up properly, change events propagate.

I still have to work on the distinction of some editors working at the field wrapper level (when cardinality > 1 or a non-textual field) or at the field item level (when cardinality === 1 and a textual field that can be edited in-place). Only then will the "direct" editor work well again.

This patch moves the rest of the Models and Views out of edit and into their own files. I cleaned up a lot of comments as well as we start to look towards getting this ready for commit down the line.

I renamed the files to match their class names. Maybe this is not ideal, but it seemed like a good idea at the time.

I'm looking into fixing saving. We've not quite extricated VIE completely. As far as I can tell it's doing a little local storage of value that get push to sync. We can definitely do this on our own. I just need a few more hours to puzzle out what the previous behavior was and make sure I reconstruct it. I'm confident I'll get it solved.

I'm also beginning to work on an entity-level toolbar now that the major refactoring is over.

Each editable field might have special toolbar groups (like WYSIWYG buttons) that need to be rendered. I'm not sure if these should go into another toolbar or be jammed into the entity toolbar.

The toolbar positions itself against an activated field, but resize the window or scroll and it positions itself against the entity again. Plus it really should be positioning against the editor, not the field.

I'm still trying to tease apart methods in ToolbarView and EntityToolbarView. Much of this is determined where existing methods should live and how to hook them up to model changes.

The EntityModel and FieldModel attributes and values still don't represent the interaction that we're trying to build here.

Also, I added some code in the Drupal.behaviors.editor.detach method to staunch an error that was arising when I tried to close an editor. I'm hiding an error that should be fixed upstream.

When the textarea formats value is text_plain, an error is thrown because settings.editor.formats doesn't contain this key. Because the text_plain editor isn't detaching, sometimes you get two editors loading for the same field.

Just a question, feel free to say no... Since this patch is pushing 250K and still ain't done yet, does it make sense to split the "refactor Edit module to not rely on VIE/Create" to its own issue, and build the entity toolbar part of it here, with that patch as a dependency? That might also help provide transparency around what's happening, since I'm quite sure from just scanning the issue title, no one would have any idea that the former is what this patch actually did.

I have to run now, but I'll create an issue later today. I've continued working off of #77, because #79 introduces entity level toolbar specific changes. We don't want those in a refactoring, in a refactoring we want to retain the exact same behavior.

Update:

saving of fields using the 'form' editor works again locally! Making it work with the other editors should not pose major obstacles.

the codebase in core/modules/edit/js before vs. after: 89 KB vs. 81 KB, and we're not even done yet. I'm now confident that: 1) we can get the same functionality, but without Create.js and VIE, 2) with LESS code!

These two points are the last bits of confirmation we need IMHO to 100% justify the move away from Create.js and VIE.

@jessebeach. I'm just joining this discussion now. How can I apply your patch for #83? Do I need to checkout 8.x HEAD then apply patch #1979784-6 and then apply patch #1678002-83? Do I apply the patch in the directory created with drush make? Thanks.

Hi arosboro! I'm psyched that you're interested in review this patch. I agree with Wim Leers that it's still quite messy. I encourage you to jump in whenever you feel comfortable with the setup and state of it. Right now we're building a few interrelated patches which means we're constantly rebasing them against each other and against D8 HEAD, which itself is moving quickly. Each time I post a patch here, I try to indicate exactly what patches and version of D8 HEAD I created it on, so you can recreate the context for the patch without running into apply conflicts.

This patch allows for entity to be saved. Once a field is changed and saved, the save button can be clicked again to save the entity.

Current issues (among many)

The EditorDecorationView._pad and EditorDecorationView._unpad methods are not acting on the right element any more. They may not even be necessary, but I haven't investigated enough to know yet.

When a field is changed and saved, it is not updated correctly.

Canceling changes to field does not restore the field's previous value.

I moved acceptEditorStateChange from AppView to FieldModel (as acceptStateChange). I did this because this is really a validation method of FieldModel and its collection and not an app-level restriction. We might also need the same sort of validation for the EntityModel as well and it made sense (to me at least) that these validation checks live in the model. I'll discuss with Wim before really committing to this approach.

What it means to be an editing field is determined by the FieldModel. EntityModel just wants to know if the field is in an 'editing' state which in this case means any of the following states: ['activating', 'active', 'changed']

This patch cleans up the positioning of the . I took a hint from #1979784-16: Factor Create.js and VIE.js out of the Edit module and I'm now positioning against the EditorView.getEditedElement() element when it exists. This is the field element being edited and not the wrapper around it, which is what the fieldModel.el attribute points to.

After chatting with Wim, I determined that I was going too far with the changes I was introduces, so I paired them back. We're now targeting a minimally changed design in this issue that introduces an Entity Toolbar with minimal changes to the underlying behavior of inline-editing.

Still left to do:

Clean up the saving code in the EntityModel.

The 'save' button in the Toolbar needs to say something other than 'save' when saving a field because it isn't really saved at that point, just temporarily stored.

The title of the element should be displayed in the entity toolbar when no field is active. We'll probably need an AJAX callback to get the title or it'll need to be passed with one of the existing metadata calls.

We need unload warning messages that keep a user from navigating away if they have unsaved changes.

When a field is changed and 'saved' but the entity hasn't been saved, we need to color the fields border orange to indicate that it's dirty.

Seems to make sense that changes to fields can be 'confirmed' by simply moving between fields, and it's not necessary to have a button for it. So if I change a field, and click in the body, the changes to the field are 'confirmed'. If I click on a completely different entity thinking I can continue working on the page without yet saving, I should perhaps be asked if I wanted to save the previous entity, or whether to cancel all changes?

If anything rather than confirming or cancelling changes to a field, a user may desire fine-grained control over undoing individual changes, so the X in the toolbar could be relabelled 'Undo last change', with another button 'undo all changes' that scrubs all changes to the entity. I guess with the way this is working, it would even be possible to undo a number of sequential changes, but 'undo last' or 'undo all' seems a reasonable starting point, if it's needed at all for good user experience.

I think as far as 'saving' goes, it's actually quite a nice safe button that will give people confidence, so long as as with traditional offline editors it is 'governing' the complete entity (or the whole page), and we may lose that confidence by befuddling it by adding a curve ball in the form of an between-fields 'confirm changes' button.

Realise it sounds like I'm saying this whole issue is misguided... I'm not! Just trying to point out that the 'entity-level toolbar for saving fields' shouldn't have to be about literally saving fields, but some form of interface for confirming or not confirming changes.

I just don't think the 'confirming' is entirely necessary, because if you don't want to confirm it, you can 'unconfirm' it by clicking undo. If people have made changes, they've made changes, they don't know that these changes haven't yet flowed into a temporary store that's the precursor to being fully saved. So if they've made those changes, if they don't like them they click 'undo', if they're happy they continue editing.

1. Save - present if there are atomic edits in tempstore, or a field is activated and changed
a) If a field is activated and changed, confirm the change and save all atomic edits in tempstore
b) Otherwise if no field is activated and changed, save all atomic edits in tempstore

2. Undo last - present if a field is activated and changed, or there is an atomic edit in tempstore
a) If a field is activated and changed, revert the field to that saved
b) Otherwise clear last atomic change in tempstore and undo the change or the whole entity if only 1 change

3. Undo all - present if there is >1 atomic edit in tempstore, or field is activated and changed and there is 1 atomic edit in tempstore
a) Clear tempstore and revert the whole entity

lightsurge, these are exactly the issues I've been struggling with here. As we move from a per-field editing experience to a per-entity editing experience, it's right to point out that the interaction should change. A user would expect to move between fields easily and then commit those changes once by saving the entity. And then, as you point out, once we allow changes to stack up, we also need a way to unwind/rewind that stack i.e. undo/redo.

We will most likely need to address these changes in stages. The first here is to introduce an entity toolbar that just barely alters the current field saving behavior. We know the current behavior is functional. The entity toolbar has enough complexity that introducing it is a considerable coding effort. Changing the UX of in-place editing in a way that meets the expectations you describe above would become a follow-on effort. There's also the current architecture of Drupal to consider. We're working on #1979784: Factor Create.js and VIE.js out of the Edit module and #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits in tandem to improve our ability to model an entity on the front end and edit it atomically on the backend with some kind of temporary storage layer in between. None of this, of course, precludes us from defining what the ideal experience should be.

I'll chat with Kevin and see if we can put together a vision of what the short term goal for this issue looks like and what a longer-term, entity-based.

lightsurge, do you feel like you could mock-up the ux you describe above in some images?

I don't feel like we should expand the scope of this issue to include "undo", although yes - if you are moving across fields with temp storage - you might want this. I feel like that is a reasonable UX trade off for getting this into core, I'd like us not to complicate this issue much further. If we wish to do this a separate issue should be opened.

Given that it took a lot of time, in prototyping, discussing and finally data, to arrive at this direction - lets not deviate before we have a fully working patch that we can actually evaluate. As previously stated the work here, is very much work in progress - I dont think we can fully evaluate it, untill we have an actual working patch (right now its too bugged, to really evaluate as pointed out in #94).

That's fair enough, this was in response to #98 with the meaning/context problem in terms of 'how do we make the save button make sense now'.. but if my feedback would be a deviation rather than a possible solution, I'm +1 for something else as well ;-)

@lightsure No worries, I just want to make sure, we get to the point where we can evaluate it fully. The problem you identified, exists - but it might be better easier to find solutions to once we have a good baseline interaction. I'd also be a better point of determining whether its part of this issue, or whether it should be a separate one.

Of the things left to do listed in #98, here's what this patch addresses:

Clean up the saving code in the EntityModel.

The title of the element should be displayed in the entity toolbar when no field is active. We'll probably need an AJAX callback to get the title or it'll need to be passed with one of the existing metadata calls.

These have been addressed, but with caveats.

We need unload warning messages that keep a user from navigating away if they have unsaved changes

There's a warning if the user closes the entity toolbar with the close button, but no on window.unload yet.

When a field is changed and 'saved' but the entity hasn't been saved, we need to color the fields border orange to indicate that it's dirty.

When a field saves to tempstore and rerenders, the class that marks it as changed gets blown away. As soon as I get that class to stick through the rerendering, it will work.

And I haven't addressed this one yet.

The 'save' button in the Toolbar needs to say something other than 'save' when saving a field because it isn't really saved at that point, just temporarily stored.

The big change is that you can now start quick editing, change a field, click into another field without clicking the save button, and your changes from the previous field will be saved to tempstore. Clicking the save button writes everything to the database.

When a field is changed and 'saved' but the entity hasn't been saved, we need to color the fields border orange to indicate that it's dirty.

The save button issue is also cleaned up:

The 'save' button in the Toolbar needs to say something other than 'save' when saving a field because it isn't really saved at that point, just temporarily stored.

The last thing I need to introduce is the full edit link that brings the user to full node editing. I foresee some difficulty transferring data from edited fields to the full edit node form. If this takes me more than 45 minutes to solve, I'm going to drop it and push that behavior to a future issue in favor of getting something reviewable completed this weekend.

There are numerous regressions from #110, but the patch at least applies and given the number of code conflicts, this is a definite improvement. The regressions should be easy to hunt down. They include.

Saving an entity no longer works. Not sure if this is a tempstore problem or a entity save problem.

The entity toolbar no longer repositions itself to the field being edited.

(This is not strictly a bugfix reroll, it's part of my review process, so there's also changes in there unrelated to solely fixing the problems pointed out in #111.)

Changes

Fixed: entity metadata retrieval was node-specific (or at least it failed for CustomBlocks). I made it work for all entities. I also renamed it from "entity title" to "entity label" everywhere, because that's what it's called in the PHP code too. Also updated the interface.

Fixed: field labels didn't work due to changes in the refactor patch.

Fixed: The entity toolbar no longer repositions itself to the field being edited. — Also due to changes in the refactor patch.

Changed: the label in the entity toolbar. When a field is highlighted or active, it would be separated from the entity label by ::. That's hard to parse. I modified it a little bit, but of course we need to refine this later on.

Fixed: drupalSettings.edit.entitySaveURL is not used. That's fine (even better, actually: less settings to pass!), but the path is hardcoded to be root-relative, even though Drupal may be installed in a subdirectory. So now it's using drupalSettings.basePath.

EDIT: forgot to say: CKEditor's toolbar doesn't show up yet, but it *does* load. So it's definitely not yet fully fixed.

Until now, the !reset_tempstore part of the URL was not being set properly (it's a mystery to me how this ever worked before — the parameter wasn't being parsed on the server side nor was it being set on the client side). This moves that from the path to a POST parameter, and ensures it's set correctly when appropriate. Only now the queueing of changes and clearing of changes will work correctly. As soon as at least one change made within the current page load is stored in the TempStore, then we'll just keep on queuing.

Changes

Fixed: The !reset_tempstore part of the URL is not being used at all! As a consequence, it's impossible to queue multiple field changes and save them all at once.

Review

Entity metadata handling: all ok, and this makes the whole actually more consistent :) However, I think I'd rather see us explicitly passing the entity ID to the server, instead of always generating the metadata for every entity of every field. The current patch answers more questions than the request asks :)

Shouldn't FieldToolbarView be removed altogether? Doesn't it make more sense to let EntityToolbarView also take on its tasks?

The removal of _ignoreHoveringVia implies that if you hover away from a field onto the toolbar (which used to be the field toolbar and now is the entity toolbar), that will in fact count as a mouseleave. We want to allow for hovering the toolbar, because otherwise the label in the entity toolbar is ADHD-like :)

Rendering of validation error messages is busted. (I typically test this by having a plain text field that is required and trying to make it empty and then save it.)

The "close" handling breaks in two ways:

if an editor is active and you close, then none of the contextual links on the page with a "Quick edit" link in them reappear, because the fields transitioned directly to the inactive state instead of first going to the candidate stage

if one of the previously edited fields of the entity have been changed, those will not be restored properly

I think EntityModel.attributes.state is intended to "mirror the state of the currently being edited field"? But at the same time, it is not being used at all AFAICT? Can't this be removed?
I think EntityModel.attributes.is(Active|Dirty) are sufficient?

EntityModel's listening to the viewChanged event, which calls the viewChange method, which triggers the fieldViewChange method on the EntityToolbarView, which in turn triggers the render method… can't that be simplified? No matter the answer, the names should become more descriptive than "view changed" :)

EditorDecorationView sets the edit-changed class, but never removes it, it's AppView that removes it. The same object should be responsible for setting and removing it. Furthermore, in the case of closing an entity without saving, this is why those fields remain marked as changed afterwards.

In AppView.enableEditor(), special care is taken if the previously active field's state is changed. But the similar care should be applied when the state is invalid: that's also a changed field, that was attempted to be saved, but turned out to be invalid. So the user should get the choice: modify it to be valid and save again (i.e. disallow the user to go to another field), or discard the current value. This is the only case where it's acceptable to have a modal in between moving from field A to field B, AFAICT.

Also in AppView.enableEditor(), I'm missing a comment to explain that it's okay to set the field whose editor will be activated to the activating state immediately, without first setting it to highlighted. The reason being that the current state will already be highlighted, because you had to click (and thus hover) it first. The point is: we should explain that we're not skipping any states!

We should never allow a field to transition directly to the inactive state. All code is currently built on the assumption that a field first must go to candidate before it can go to inactive. We can change that in the future, but not in this patch IMHO.
Related to that is this change in formEditor.js:

switch (to) { case 'inactive':+ this.removeForm();

AppView.rerenderedFieldToCandidate() is making DOm changes that should happen in EditorDecorationView. AppView should never modify the DOM directly.

First, this.$field doesn't exist anymore, second, the positioning is gone. Quite possibly the positioning isn't needed anymore because FieldToolbarView is contained within EntityToolbarView. But does EntityToolbarView already support display:inline elements?

This hack in EditorView.stateChange() is absolutely unacceptable:

+ // Attempt to save if the field was previously in the changed state.+ if (from === 'changed') {+ this.model.set('state', 'saving');+ }

This is most definitely the wrong place to be doing this.

I dislike the passing around of stateChange()'s options parameter into EditorView.save(), but I don't see any other way. It should be explicitly documented on EditorView.stateChange() though that it is essential that the save method must options.

Love the way you're setting aria-hidden=false as soon as the entity becomes dirty :)

AppView.save() needs to be simplified.

Modified fields might trigger entity label modification (e.g. auto_nodetitle, or the title field itself). So upon every field modification, the entity label should also be retrieved again, from the entity in the TempStore?

Changes

active/single editor states don't belong in the EntityModel (where they are) nor in the FieldCollection (where the comment said they should be moved), they belong in the AppView. Since only the pre-existing one in AppView was

Fixed: The code responsible for updating AppView.changedFieldsInTempstore was still using fieldModel.get('editID') instead of fieldModel.id.

the only display:inline fields that exist in core are the author and date fields on nodes — in that case, the toolbar needs to be positioned in the right location using a different approach. Although with the entity toolbar, that might no longer be necessary, as I said.

the screenshot that you use as an example was probably made on D8's default front page. There, not the entire body text (the "Default" formatter) is rendered, but only a part (the "Summary or Trimmed" formatter). See admin/structure/types/manage/article/display/teaser. In any case, the selection of the in-place editor is completely unrelated to the entity toolbar work and hence is irrelevant here :)

the only display:inline fields that exist in core are the author and date fields on nodes — in that case, the toolbar needs to be positioned in the right location using a different approach. Although with the entity toolbar, that might no longer be necessary, as I said.

Ok, I think we can pull the inline distinction out then. The EntityToolbar positions against the edited element of a field.

// If a field in this entity is active, position against it.var activeEditor = Drupal.edit.app.model.get('activeEditor');var activeEditorView = activeEditor && activeEditor.editorView;var activeEditedElement = activeEditorView && activeEditorView.getEditedElement();

Entity metadata handling: all ok, and this makes the whole actually more consistent :) However, I think I'd rather see us explicitly passing the entity ID to the server, instead of always generating the metadata for every entity of every field. The current patch answers more questions than the request asks :)

Rebased on D8 3ad81ec0a6b8cba88cabd0b422c3f9cd5f750f91.

Built on

#1979784-36: Factor Create.js and VIE.js out of the Edit module
#1901100-27: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

The removal of _ignoreHoveringVia implies that if you hover away from a field onto the toolbar (which used to be the field toolbar and now is the entity toolbar), that will in fact count as a mouseleave. We want to allow for hovering the toolbar, because otherwise the label in the entity toolbar is ADHD-like :)

I don't think this is the case. When an editor is active, the toolbar always reflects the label of that field. Otherwise, when you're hovering a field, the label reflects that hover.

When an editor is not active and you hover from a field to the entity toolbar, the field label disappears and just the entity label is present because at this time, you are acting on the entity, not a field, if you press save or close.

@jesse I am slightly confused, I have been following this perhaps misunderstanding for a while but from my point of view, there won't be a entity toolbar active unless you are editing a field. When you are not editing a field, there is no reason to have an entity toolbar? All we need is some signalling when you go out of "quick edit mode".

@Bojhan, the entity toolbar will be present while an entity is in "quick edit mode". The actions available in the entity toolbar will be dependent on the field that is actively being edited. When no field is being edited, the entity toolbar will provide a save action (if a field has been changed) and a cancel action to quit "quick edit mode".

@jesse I get that the patch currently does this, but where was this design decision made. And more importantly why? It is my understanding we didn't want a "hovering" bar, when you are outside of editing a field because;

1) its not really relevant when you are "overviewing"
2) we want to push towards clicking "save" when you are editing your last field
3) the bar would hover over other elements, potentially obstructing the view to parts of the entity? In the case of editing a block/menu inline

I do see a lot of advantages too, but I'd like to know when and how this decision was made because this was not reflected in the prototype and/or design that we agreed upon in the direction provided in #39 and #41. I have been trying to follow this thread, but failed to spot this UX decision.

@WimLeers Hmmpf, my mistake. I thought that was simply because it was still a raw prototype. I will have a chat with Kevin, because the design will need to be adjusted if we keep this in - right now (in the prototype) it looks like you are always on the top field - even outside of edit mode.

This patch contains some experimental code exploring how we deal with the managing the state of the entity as a function of the state of its fields. An entity's fields' states determine if the entity edit session can be closed or opened. We're running into race conditions, so more work is needed.

Currently, only text fields are saving. Image fields and tag field changes are not saving. This appears to be server-side errors, but my bet is it's something we're passing to entity save, since a full node edit works just fine.

Also, mousing off the entity toolbar makes the save button disappear. Small issue, will be addressed in the next patch.

This code contains comments from my discussion with Wim at DrupalCon Portland. It doesn't push functionality much farther than #130. I wanted get this code off my computer though and made public because I will not be able to focus on it for the next 2 days.

Also, mousing off the entity toolbar makes the save button disappear. Small issue, will be addressed in the next patch.

I've also begun to address treatment of invalid field data. Most of the pre-refactor code worked just find. I just need to wire the field states up to the entity toolbar properly and change some CSS. This was a lot less work than I had predicted.

With the major pieces of functionality in place, I'm going to start writing tests for this feature into the FAT module.

I realized while testing that reverting changes does not work because placing a field in candidate state destroys its EditorView. This happens when

Making edits to a field, canceling from the entity toolbar, then canceling the confirmation dialog.
Making edits to a field, switching to another field, canceling from the entity toolbar, then canceling from the confirmation dialog.

The first scenario can be fixed, perhaps, by tweaking the flow of how when we call EditorView.revert in the entity edit session shutdown process.

The second scenario leads me to believe that we can't destroy the EditorView on a field once it has been established, because we need something that will know how to revert the changes even when that field no longer controls the active editor on the page.

Definitely Bojhan. I wish I could make this move faster. I've very sensitive to the fact that this UI feature has data-loss potential, so getting the internals right is causing me some grief. Wim and I are going to discuss the current state today.

Any help filling out the manual testing scenarios in the issue description would be much appreciated. There are so many paths through this UI. I want to be sure we haven't missed any of them.

I cleaned up a lot of small things, added docs, fixed some subtle but fundamental problems, and ensured the fluent UX (always allow clicking from one field to the next) we want is really there.

TODO

I expanded entity state validation, but we still must make sure there are no edge cases I forgot. (time constraints)

I worked on improving EntityModel.save(), but it's not fully there yet. (time constraints)

I had to muck with Drupal.ajax to make it work for concurrent use cases (which we need for fluent UX, see below), but it seems that if a field that uses the "direct" editor (a plain text field) that is required is first mad empty, to trigger the 'invalid' state, then is filled in again and then saved, causes the submit to be an *regular* submit rather than an AJAX submit; result: AJAX barf presented to the user.

I tried, but don't understand jQuery.ui.position() well enough to integrate it with Drupal.displace

(This is #134.4) Once you've finished in-place editing an entity (having saved it) and then in-place editing it again, things are kinda broken.

Code review

CSS

Not the right time to review, plus I trust your judgement blindly there :)

edit.js

FIXED IN PATCH: Problem: entity metadata is being piggybacked on field metadata requests, which can lead to duplicate requests (plus docs haven't been updated).

Problem (that was kind of introduced by me): no language handling for entity metadata! Field metadata is per-language, entity metadata currently isn't. You infer the language for the entity on the server side from the fields, but it's stored on the client-side without the language specifier. I caused this problem because the data-entity-id attributes also lack the language identifier. The difference is that before, this was just for internal use, but now there is metadata so now it *should* include the language code.

formEditor.js

FIXED IN PATCH: simplified the code and documented it so that this TODO was now obsolete: @todo This reset param shouldn't be associated with a single field commit. I can't quite figure out how it works yet, though.

FIXED IN PATCH (and already reported in #115): Calling removeForm() when entering the 'inactive' state. I checked with Jesse and this was just a leftover from earlier.

EntityModel.js

FIXED IN PATCH: s/tempstore/TempStore/gi :)

FIXED IN PATCH: Talked to Jesse, the TODO to introduce an EntityController and an EntityFieldController is a nice-to-have refactor. It's not essential. Removed the TODO.

FIXED IN PATCH: ""the app should be responisble for destroying the fields." was an old leftover TODO, now removed.

FIXED IN PATCH (also fixes #134.2): The isDirty attribute on EntityModel and FieldModel conflated things. Lots of subtle bugs ensued. I've split this into isChanged and inTempStore on FieldModel, the OR'd result determines whether the field gets the edit-changed class. Similarly, I've split it into isDirty and inTempStore on EntityModel. The former works identically to the isDirty attribute that you had introduced (but it now has corrected docs) and the latter is used to determine the value for the reset POST parameter. All relevant docs have been updated to be very specific.

NOTE: following on the previous statement, ideally we wouldn't have the EntityModel.attributes.changedFields (now renamed to fieldsInTempStore attribute which is more accurate), since it is derivable at any time from an entity's fields' inTempStore attribute. The only complication here is that whenever a field is rerendered (i.e. when it is saved to TempStore), the corresponding FieldModel is destroyed and hence the field-level inTempStore attribute's value is also lost. A solution would be to keep a field's FieldModel forever, also after rerendering.
So, I started implementing this ("keep FieldModels around forever"), but found several problems:

non-required fields may accept to have the empty value, which would cause them to not be re-rendered, which means we now have a FieldModel that exists forever but no longer corresponds to a field, which makes no sense; and would likely lead to more problems down the road

keeping the same FieldModel instance around implies that the associated element must be updated, but that in turn means that this updated element must propagate to all views attached to the FieldModel: the EditorView, the FieldToolbarView, and so on. It's possible, but it's a lot of extra handling.

So, point 2 says it's hard but not impossible, point 1 gives a very good reason to not do it. Overall, it gains us very little: it makes sense that whenever a field was successfully saved to TempStore and is thus rerendered, that all related Views are also purged and rebuilt; the state is the initial clean state again and can be advanced to the target state (i.e. state = 'candidate' and inTempStore = true) very easily. So let's stick to that for now, until we have plenty of free time to refactor, if it still makes sense then :)

FIXED IN PATCH: EntityModel.fieldStateChange() was changing the state of fields that had just entered the 'saved' state to the the 'activating' state. Not only is this wrong for multiple reasons, it also was effectively a no-op: the field state change validation system prevented this from happening… :) Now I just removed this piece of code.

FIXED IN PATCH: EntityModel.validate() was validating the *current* state of the entity, not the state it was trying to be set to.

FIXED IN PATCH: expanded entity state change validation, similar to FieldModel's. This addresses these two TODOs:

* @todo We need to restrict the state progression of an entity. * @todo We need a special exception to go from anything to deactivating.

FIXED IN PATCH (at least mostly): EntityModel.save() was performing AJAX requests multiple times. Introduced a mutex to mitigate that: EntityModel.isCommitting. This also removes the hack to use BB.validate() whenever EntityModel.save() is called.

FIXED IN PATCH: document all EntityModel.states.

FieldModel.js

It's both bizarre and understandable that the isDirty attribute that you added is not updated by FieldModel itself, but by EntityModel. BUT: more importantly: is it really necessary? I doubt it is. There already is EntityModel.changedFields: why not just use that instead?

FIXED IN PATCH (as said in #115): 'invalid' field state handling:

special care is taken if the previously active field's state is changed. But the similar care should be applied when the state is invalid: that's also a changed field, that was attempted to be saved, but turned out to be invalid. So the user should get the choice: modify it to be valid and save again (i.e. disallow the user to go to another field), or discard the current value. This is the only case where it's acceptable to have a modal in between moving from field A to field B, AFAICT.

AppView.js

FIXED IN PATCH: instead of storing the EntityToolbarView instance at EntityModel.attributes.entityToolbar (i.e. as a Backbone Model attribute), store it as a property on the object itself. Which one is better doesn't matter at this point, it's just for consistency with how Views related to FieldModel are handled.

FIXED IN PATCH (and already reported in #115): the following field state progression restrictions were removed unnecessarily:

Also, this should be handled by the state handling code, the calling code shouldn't be aware of application details, it should only declare what state it wants to achieve.
To solve this, the original LoC was restored, I modified AppView.acceptStateChange() to embed the logic that lived in AppView.enableEditor() and removed that method. I then discovered that the cause and solution to #134.3 was actually right there: when the active field is in the 'changed' state, then its state is changed to 'saving', but the field that tried to get to the 'activating' state would not be enabled. I fixed that now.
By doing that, I've achieved the fluent UX we want, BUT:

by doing so, the field that we just moved away from might turn out to be invalid (i.e. enter the 'invalid' state), so we need to handle that too. I implemented this.

it turns out that Drupal.ajax's documentation is *utterly wrong*, which led to nightmareish race conditions because we can now have two concurrent AJAX requests that result in Drupal.ajax handling the response. The example provided by Drupal.ajax overrides the *prototype implementation* of the AJAX command, so it affects all other Drupal.ajax instances too! Hence our code did that too. It's pure luck that we didn't run into this problem before, it's the concurrency that surfaced this. I spent at least 4 hours debugging this.

FIXED IN PATCH (fixes #134.1): the _pad() and _unpad() methods were modified incorrectly, hence the problems :) I think the changes were leftovers from the early days of this patch, where these functions were acting on the wrong element.

EditorView.js<?dt>

FIXED IN PATCH (already reported in #115):

+ // Attempt to save if the field was previously in the changed state.+ if (from === 'changed') {+ this.model.set('state', 'saving');+ }

EntityToolbarView.js

FIXED IN PATCH: added more docs, very minor cleanup.

FieldToolbarView.js

FIXED IN PATCH: a memory leak was introduced there.

util.js

FIXED IN PATCH: no changes were made here, but one change was necessary: the reset POST parameter that is used in EditController::fieldForm() to decide whether to render forms based on the entity stored in TempStore or not was not being passed on in Drupal.edit.util.form.load(), causing an unfinished previous in-place editing session's changes to show up in forms in the current in-place editing session.

The container around the whole node before the user focuses on a field is not there.

Kevin and I had a long discussion about this and we've decided to table this detail for further thought. Essentially, a user would see a border around the entity when quick edit is enabled. Upon clicking an editable field, the border around the entity is removed and borders around all the editable fields are revealed. We noted two problematic cases under this model:

Within an entity, there may be non-in-place-editable content that when clicked, would fail to launch the borders around the editable fields, so a user will have a negative learning experience. Finding an editable field becomes a hunt-and-peck game.

Editable fields might not be visually located within the border of its entity's HTML element for many reasons: the field is floated; the field is absolutely positioned; the field has been moved with JavaScript.

I fixed a very minor JS issue that was throwing errors when the entity went into the committing state. See the interdiff. I had to reroll #1901100 and then I ran into a 500 error upon trying to save the entity. I have no idea what to do with this error and it's too late where I am to start digging in tonight:

Uncaught PHP Exception InvalidArgumentException: "No check has been registered for access_check.edit.entity" at /Users/jbeach/code/drupal/core/d8/core/lib/Drupal/Core/Access/AccessManager.php line 193

@jessebeach: I tried to reproduce the "No check has been registered for access_check.edit.entity" issue with #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits but neither manually going through creating a node, in-place editing and calling the save callback reproduced that, nor running the tests for it did (with latest head). The access_check.edit.entity is a service registered from edit.services.yml and associated with a class that has access checking implemented.

This patch cleans up the placement of the toolbar. It now won't get lost under the toolbar. It also positions itself against the active form editor in addition to the active field, the highlighted field and the entity.

This patch resolves an issue with the _pad() and _unpad() methods. They had been called under unwanted circumstances, which would cause fields to appear to crawl across the page. Now, _pad() can only be called once until _unpad() is called.

When canceling changes and then canceling the confirmation dialog box, the page refreshes in order to get the original values of the fields back on the screen. This isn't an ideal solution, but it's the cheapest one at the moment. What we really need is a refactor of the fieldModel rerender pipeline, but that isn't something we should try to tackle in this patch.

And sometimes you'd get a stray 'edit-highlighted' class when closing an edit session. This is fixed.

The flows concerning invalid fields are not functioning well again. Fixing those flows will be the final bit of functionality to correct before we can start looking at this patch for commit.

Review of #150: looks good — I noticed that the positioning is now much more smooth overall :)

Review of #151: the page reload logic was flawed: it was not being reset to its initial state when starting to edit a new entity, plus it was not using .revert() when it could. Fixed that, and clarified the comment.

I'm still working on a fix for

The flows concerning invalid fields are not functioning well again. Fixing those flows will be the final bit of functionality to correct before we can start looking at this patch for commit.

The flows concerning invalid fields are not functioning well again. Fixing those flows will be the final bit of functionality to correct before we can start looking at this patch for commit.

! :)

The cause: old Drupal.ajax instances used by Drupal.edit.form.ajaxifySaving() weren't being GC'd by the JS engine, because the jQuery event handler that triggers them contained a reference to them. Said jQuery event handler was not being unbound. Simply unbinding the jQuery event handler hence solved the problem.

After reviewing with Dries and Kevin, they pointed out that the contrast between the toolbar (formerly with a white background) didn't contrast enough with other elements on the page to pop out. So I've added some coloring. It's probably not the right color, but it suggests what the toolbar would look like with a better color choice.

The toolbar also now has a little point that points to the field or entity that is currently its context.

I'm not sure if this is desired, but the comments and comment form were quite confusing during an entity inline editing session. The bottom of the entity is actually after the comment form, so the toolbar would zip down to the bottom of the screen, trying to get to the bottom of the entity if you scroll down far enough. I've hidden the comments with CSS with its entity is being edited. We'll probably want to discuss that decision.

this.timer = setTimeout(function () { // Render the position in the next execution cycle, so that animations on // the field have time to process. This is not strictly speaking, a // guarantee that all animations will be finished, but it's a simple way // to get better positioning without too much additional code. _.defer(function () { that.$el .position({ my: edge + '-2 bottom', at: edge + ' top', of: of, collision: 'flipfit', using: function (suggested, info) { info.element.element.css({ left: Math.floor(suggested.left), top: Math.floor(suggested.top) }); // Determine if the pointer should be on the top or bottom. info.element.element.toggleClass('edit-toolbar-pointer-top', info.element.top > info.target.top); }, within: that.$fence }) // Resize the toolbar to match the dimensions of the field, up to a max // width that is equal to the field's width. .css({ 'max-width': ($(of).outerWidth() * 0.9), // Set a minimum width of 240px for the entity toolbar, or the width // of the client if it is less than 240px, so that the toolbar // never folds up into a squashed and jumbled mess. 'min-width': (document.documentElement.clientWidth < 240) ? document.documentElement.clientWidth : 240, 'width': '100%' }); }); }, delay);

That's kinda painful to read, any chances you can make intermediate functions to keep the nesting down?

Nothing in particular about EntityModel.js

So yeah, tiny details. No major problems on my end I suspect the todos are too big to solve now, there are follow-ups? Little position problem probably because of some border with or maybe it's by design, don't know, not a big deal.

@Bohjan and @tkoleary, we can iterate on the specifics of the UI once the major code changes are in place, those being the changes represented in this issue. I will open followups for any that are missing following the comments in #167. We can have one for UX refinements as well.

// Hides the contextual links if an in-place editor is active.this.$el.parents('.contextual').toggle(!isActive);

// Hides the contextual links if an in-place editor is active.this.$el.closest('.contextual').toggle(!isActive);

no?

yes. that should indeed be closest()

There are 3 JSHint errors. Forgot a couple of semicolons, cmon :D

Cleaned up.

make a$body variable.

Added.

That's kinda painful to read, any chances you can make intermediate functions to keep the nesting down?

Pulled the anonymous functions out into named functions scoped to this method.

Regarding the @todos in the code, I removed most of them. Upon further reflection, they really didn't describe any refactoring that is strictly necessary. The couple @todos that are still left over have been given a specific task for followup. The tasks are dependent upon other fixes being committed to Drupal, so they are blocked by dependencies.

I corrected the failing tests as well. I'm posting this patch to have the bot run tests. tkoleary has a few small styling tweaks that he'd like to apply, so there will be one more patch later today before we can start considering this for RTBC.

Validation of this patch

The UX changes introduced by this patch (which solely affect in-place editing) were validated in usability testing in the beginning of April: #45.

The usability team asked us to go ahead and implement the prototype used in the usability tests: #50.

jessebeach worked on this for the better part of two months (!!!) — because this is so inherently complex under the hood.

I reviewed the patch in depth at several points in time.

nod_ — the Drupal 8 JavaScript maintainer — reviewed the JS in #167. He had almost no remarks.

Testing of this patch

Since this is almost exclusively JS code, and we have no automated JS testing in Drupal 8 core (sadly), and the number of possible scenarios is mindbogglingly large, Jesse went through great efforts in ensuring correctness/preventing breakage:

She came up with twenty manual tests (see the issue summary), all of which now pass.

Conclusion

This patch has received an extraordinary amount of scrutiny and review, to ensure maintainable code and correct functioning.

As I said, I reviewed (and rerolled) the patch at several points in time. However, I did not play a leading role in driving this home. Therefore, I consider myself to be in a position where I can credibly RTBC this.