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

  1. Entity toolbar introduced with the EntityToolbarView and EntityView classes.
  2. Field-related toolbars (such as a WYSIWYG toolbar) are now rendered with FieldToolbarView inside the EntityToolbarView element.
  3. 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.
  4. edit.css is broken out into: edit.module.css, edit.theme.css, edit.icons.css
  5. The JavaScript has been combed through for needed commenting and JSHinted.
  6. Front end test for editing and saving the body field of an article: http://drupalcode.org/project/fat.git/blob/HEAD:/tests/edit.tests.js
  7. Front end test for editing and saving the tags field of an article: http://drupalcode.org/project/fat.git/blob/HEAD:/tests/edit.tests.js
  8. Styling of the entity toolbar components e.g. the control buttons do not match core styling yet.

Remaining

  1. Testing of control flows.
Manual testing scenarios
  1. PASSED | Enable in-place editing / activate a textarea field / change the content of the field / press the entity toolbar save button
  2. PASSED | Enable in-place editing / activate a taxonomy field / change the content of the field / press the entity toolbar save button
  3. PASSED| Enable in-place editing / activate an image field / change the content of the field / press the entity toolbar save button
  4. PASSED| Enable in-place editing / activate a text field / change the content of the field / press the entity toolbar save button
  5. PASSED | Enable in-place editing / activate a taxonomy field / change the content of the field / press the entity toolbar save button
  6. PASSED | Enable in-place editing / activate a field / press the entity toolbar cancel button
  7. 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
  8. 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
  9. 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
  10. 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
  11. 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
  12. 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
  13. 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
  14. 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
  15. 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
  16. 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
  17. 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
  18. 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
  19. 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
  20. 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.

Screenshot of the entity toolbar, focused on the entity.

Screenshot of the entity toolbar, focused on an image. The image is being edited.

Screenshot of the entity toolbar, focused on the body field. The toolbar is in the middle of saving the field.

Original report by webchick

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

Related issues in the Spark issue queue (still to be consolidated into this issue):
- #1586416: [Meta] Make Save/Publish distinction clearer
- #1702250: [META] As a content editor, I want to make my edits quickly and then hit save
- #1780984: Test prototype for Save/Publish in one place + revision selection
- #1494640: Add a workflow/revisioning module to Spark

CommentFileSizeAuthor
#175 Screenshot of the entity toolbar, focused on the entity.189.26 KBjessebeach
#175 Screenshot of the entity toolbar, focused on an image. The image is being edited.129.14 KBjessebeach
#175 Screenshot of the entity toolbar, focused on the body field. The toolbar is in the middle of saving the field.156.85 KBjessebeach
#174 entity-toolbar-1678002-174.patch139.17 KBjessebeach
#170 entity-toolbar-1678002-170.patch138.99 KBjessebeach
#170 interdiff_162-to-170.txt18.75 KBjessebeach
#164 interdiff_158-to-162.txt5.29 KBjessebeach
#164 entity-toolbar-1678002-162.patch141.39 KBjessebeach
#162 Screenshot of the entity toolbar. The entity&rquot;s background color is darker. The small, triangular point is indicated. The point is pointing down.104.82 KBjessebeach
#162 Screenshot of the entity toolbar. The small, triangular point is indicated. The point is pointing up.63.95 KBjessebeach
#158 entity-toolbar-1678002-158.patch138.12 KBjessebeach
#158 interdiff_156-to-158.txt6.28 KBjessebeach
#156 entity-toolbar-1678002-156-do-not-test.patch135.77 KBjessebeach
#156 interdiff_154-to-156.txt20.1 KBjessebeach
#155 Screenshot_6_18_13_1_57_PM.png228.19 KBjessebeach
#155 Fullscreen_6_18_13_1_57_PM.png232.56 KBjessebeach
#154 entity-toolbar-1678002-154-do-not-test.patch125.02 KBWim Leers
#154 interdiff.txt3.51 KBWim Leers
#153 entity-toolbar-1678002-153-do-not-test.patch124.66 KBWim Leers
#153 interdiff.txt2.97 KBWim Leers
#151 entity-toolbar-1678002-151-do-not-test.patch126.51 KBjessebeach
#151 interdiff_150-to-151.txt5.93 KBjessebeach
#150 entity-toolbar-1678002-150-do-not-test.patch124.33 KBjessebeach
#150 interdiff_149-to-150.txt12.59 KBjessebeach
#149 entity-toolbar-1678002-149-do-not-test.patch118.32 KBjessebeach
#147 entity-toolbar-1678002-147-do-not-test.patch117.32 KBWim Leers
#147 interdiff.txt6.21 KBWim Leers
#146 entity-toolbar-1678002-146-do-not-test.patch119.84 KBjessebeach
#146 interdiff_142-to-146.txt1.05 KBjessebeach
#144 Screen Shot 2013-06-13 at 3.35.46 PM.png71.04 KBtkoleary
#142 entity-toolbar-1678002-142-do-not-test.patch118.39 KBWim Leers
#142 interdiff.txt74.21 KBWim Leers
#137 entity-toolbar-1678002-137-do-not-test.patch99.06 KBjessebeach
#136 Screen Shot 2013-06-10 at 11.57.54 AM.png565.74 KBtkoleary
#136 Screen Shot 2013-06-10 at 11.47.05 AM.png298.99 KBtkoleary
#136 Screen Shot 2013-06-10 at 11.43.42 AM.png262.13 KBtkoleary
#136 Screen Shot 2013-06-10 at 11.43.36 AM.png12.42 KBtkoleary
#134 entity-toolbar-1678002-134-do-not-test.patch98.97 KBjessebeach
#133 entity-toolbar-1678002-133-do-not-test.patch83.24 KBjessebeach
#132 entity-toolbar-1678002-132-do-not-test.patch80.02 KBjessebeach
#131 entity-toolbar-1678002-131-do-not-test.patch82.22 KBjessebeach
#130 entity-toolbar-1678002-130-do-not-test.patch81.3 KBjessebeach
#129 entity-toolbar-1678002-129-do-not-test.patch80.47 KBjessebeach
#121 interdiff.txt3.32 KBjessebeach
#121 entity-toolbar-1678002-121-do-not-test.patch72.84 KBjessebeach
#120 entity-toolbar-1678002-119-do-not-test.patch70.48 KBjessebeach
#120 interdiff_115-to-119.txt3.55 KBjessebeach
#116 Screenshot_5_14_13_11_43_AM.png103.92 KBjessebeach
#115 entity-toolbar-1678002-115-do-not-test.patch71.74 KBWim Leers
#115 interdiff.txt2.68 KBWim Leers
#114 entity-toolbar-1678002-114-do-not-test.patch72.23 KBWim Leers
#114 interdiff.txt5.71 KBWim Leers
#113 entity-toolbar-1678002-113-do-not-test.patch67.72 KBWim Leers
#113 interdiff.txt1.51 KBWim Leers
#112 entity-toolbar-1678002-112-do-not-test.patch67.66 KBWim Leers
#112 interdiff.txt9.5 KBWim Leers
#111 entity-toolbar-1678002-111-do-not-test.patch67.32 KBjessebeach
#110 entity-toolbar-1678002-110-do-not-test.patch74.27 KBjessebeach
#107 entity-toolbar-1678002-107-do-not-test.patch60.03 KBjessebeach
#98 entity-toolbar-1678002-98-do-not-test.patch38.79 KBjessebeach
#97 entity-toolbar-1678002-97-do-not-test.patch60.58 KBjessebeach
#90 entity-toolbar-1678002-90-do-not-test.patch54.66 KBjessebeach
#89 entity-toolbar-1678002-89-do-not-test.patch39.57 KBjessebeach
#87 entity-toolbar-1678002-87-do-not-test.patch36.72 KBjessebeach
#83 entity-toolbar-1678002-83-do-not-test.patch36.31 KBjessebeach
#82 entity-toolbar-1678002-82-do-not-test.patch25.93 KBjessebeach
#79 interdiff_77-to-79.txt25.09 KBjessebeach
#79 entity-toolbar-1678002-79-do-not-test.patch221.23 KBjessebeach
#77 entity-toolbar-1678002-77-do-not-test.patch210.15 KBWim Leers
#77 interdiff.txt26.09 KBWim Leers
#76 entity-toolbar-1678002-76-do-not-test.patch222.47 KBjessebeach
#76 interdiff_75-to-76.txt93.32 KBjessebeach
#75 entity-toolbar-1678002-75.patch175 KBWim Leers
#75 interdiff.txt23.51 KBWim Leers
#74 entity-toolbar-1678002-74.patch174.55 KBWim Leers
#74 interdiff.txt9.79 KBWim Leers
#73 entity-toolbar-1678002-73.patch174.14 KBWim Leers
#73 interdiff.txt4.67 KBWim Leers
#72 entity-toolbar-1678002-72.patch171.07 KBWim Leers
#72 interdiff.txt109.16 KBWim Leers
#71 entity-toolbar-1678002-71-a.patch218.17 KBjessebeach
#71 editor-bug.txt939 bytesjessebeach
#70 entity-toolbar-1678002-71.patch156.33 KBjessebeach
#66 interdiff_64-to-66.txt105.96 KBjessebeach
#66 entity-toolbar-1678002-66.patch129.66 KBjessebeach
#64 entity-toolbar-1678002-64.patch65.33 KBjessebeach
#63 entity-toolbar-1678002-63.patch6.1 KBjessebeach
#10 edit-save-button-on-toolbar.patch6.1 KBGábor Hojtsy
#10 EditCentralSave.jpg39.15 KBGábor Hojtsy
#6 Screen shot 2012-09-06 at 9.51.15 AM.png400.83 KBtkoleary
#6 Screen shot 2012-09-06 at 9.39.47 AM.png78.33 KBtkoleary
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Assigned: Unassigned » tkoleary
webchick’s picture

Another option was to get rid of save buttons altogether, save a "draft" revision upon leaving each field, and then have a "Publish" button to commit all "local" changes.

Wim Leers’s picture

The latter makes most sense, I think.

Wim Leers’s picture

Wim Leers’s picture

Related issue in the Spark issue queue: #1586416: [Meta] Make Save/Publish distinction clearer.

tkoleary’s picture

Here are the current designs showing how it works with navbar. In the context of D7 if navbar does not exist in the site I'm not sure what happens. Wim, can you comment?

in theme

Wim Leers’s picture

As of last week, the Edit module will automatically integrate with either D7 core's toolbar module or our navbar module, whichever is available! :) (Thanks to Gábor.)

Wim Leers’s picture

klonos’s picture

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
39.15 KB
6.1 KB

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.

EditCentralSave.jpg

Wim Leers’s picture

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.

Gábor Hojtsy’s picture

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.

If we want to solve the whole thing here, then I'm not sure what is the difference between this issue and #1702250: [META] As a content editor, I want to make my edits quickly and then hit save. I thought this would be a somewhat limited issue to some first steps and then we can open new ones for workflow integration, etc. where we can introduce publish/save button differentiation (and rollback/cancel).

Wim Leers’s picture

You'e right, in my comment I was expanding this issue to the whole problem space. Sorry.

It's a very tricky thing to solve, and I agree, this issue is intended to be somewhat limited. But again, I do think it's a good first step!

tkoleary’s picture

@wim @Gabor Preston is now working on updating the prototype of this (per our consensus from Acquia engineering week). Should be ready to test by 10/23.

frega’s picture

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

Wim Leers’s picture

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.

I don't think we should try to solve this before feature freeze.

Wim Leers’s picture

Title: Explore moving save/cancel to toolbar rather than per-field basis » Move save/cancel to toolbar rather than per-field basis, auto-revisions on edit (with automatic log messages)
Version: 7.x-1.x-dev » 8.x-1.x-dev
Assigned: tkoleary » Unassigned
Priority: Normal » Critical
Status: Needs review » Postponed

#1678034: Auto-save revisions on edit + generate log message merged with this issue, plus revamped the issue summary. This is now the sole issue for this topic in the Edit issue queue, the relevant issues in the Spark issue queue are linked to.

Wim Leers’s picture

Project: Edit » Drupal core
Issue summary: View changes

Updated issue summary.

Wim Leers’s picture

Project: Drupal core » Edit

Also merged #1741692: Discard via ESC button with this issue; issue summary updated.

Bojhan’s picture

Copied from #1780984: Test prototype for Save/Publish in one place + revision selection another duplicate.

#19.1

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

  1. Editing a piece of content
  2. Saving/ Publishing a piece of content
  3. 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

1. Does “Revert” auto-publish my content?

Refer Skitch Image attached in the next comment.

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.

#19.2

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.

lightsurge’s picture

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:

http://www.memo-template.com/memo-with-draft-watermark/

lightsurge’s picture

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.

Bojhan’s picture

Status: Postponed » Active

Per https://drupal.org/node/1824500#comment-6802542 , I am opening this. Lets stop with the silly postponed issues, its clear people want to discuss this.

webchick’s picture

Priority: Critical » Major

Fine, but reducing it from critical, because it shouldn't block initial commit.

David_Rothstein’s picture

For what it's worth, the thing I brought up in #1824500-59: In-place editing for Fields doesn't seem related to the original purpose of this issue, just comments #19 onward.

moshe weitzman’s picture

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.

Wim Leers’s picture

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

klonos’s picture

Yeah, I too really don't like the idea of multiple revisions for various fields edits. If it can be done with Tempstore and then in one go by hitting the/a "Save" button, then that would be ideal.

tstoeckler’s picture

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.

Wim Leers’s picture

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

klonos’s picture

tkoleary’s picture

Status: Active » Postponed

Put this issue on hold since revisions to the designs may render it unnecessary.

Bojhan’s picture

Status: Postponed » Active

Let's make sure others can discuss, if you put out a design that should fit right into the discussion. No reason, to stop others from discussing.

Bojhan’s picture

Project: Edit » Drupal core
Version: 8.x-1.x-dev » 8.x-dev
Component: User interface » edit.module
Category: task » bug
Priority: Major » Critical
Issue tags: +Usability

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.

Wim Leers’s picture

Title: Move save/cancel to toolbar rather than per-field basis, auto-revisions on edit (with automatic log messages) » Move save/cancel to toolbar rather than per-field basis, auto-revisions on edit (with automatic log messages), workflow support
Category: bug » task
Issue tags: +Spark

How can this be a regression if it's new functionality?

I agree that it's critical, but it's not a bug. Rest assured, we will address this.

tkoleary’s picture

See this related issue on save and publish http://drupal.org/node/1898946

Bojhan’s picture

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.

webchick’s picture

Issue tags: +sprint

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.

tkoleary’s picture

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
  • The state for opening a node with a "forward revision exists"
  • Fields other than title, body and tags that use other widgets
  • Click away from node triggers "unsaved fields" modal
Bojhan’s picture

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.

Wim Leers’s picture

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.

tkoleary’s picture

Bojhan:

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.

Wim Leers’s picture

Great point Kevin!

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…

Wim Leers’s picture

#1898946: [Meta] Simplify and reconcile the behaviors of edit-submit actions in node/edit and edit (edit-in-place) has been closed as a duplicate in favor of this one because of the direction explained in #38 and supported in #39 by the Usability folks. It does contain useful background information though (and helps understand why the direction in #38 has received so much support).

Wim Leers’s picture

Title: Move save/cancel to toolbar rather than per-field basis, auto-revisions on edit (with automatic log messages), workflow support » Entity-level toolbar, rather than saving per-field, auto-revisions on edit (with automatic log messages)
Assigned: Unassigned » jessebeach

This is the front-end part of the entity-level toolbar for Edit as proposed in #38, and the +1 from the usability team at #39.

The back-end part (to be able to create one revision for all changes, and have the field edits "queue up") is going to be tackled at #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits, see #8 specifically.

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.

dcmistry’s picture

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

  1. Can users find a way to edit to their content?
  2. How do users edit the title and body of their content?
  3. Do users understand how to make their edited content live?
  4. How was the experience of using this prototype?
  5. 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
  • Prototype: http://www.acquians.info/Public/Edit/spark/blocks/index.html# (Chrome)

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)

5. “It looks painless” (P1)

6. “This will help the marketing team... inline editing will drastically improve performance” (P1)

7. “This will have a smaller learning curve compared to Drupal Gardens” (P1)

8. “It is user friendly to someone who is not a developer” (P2)

9. “Very easy to go and make quick edits” (P2)

Other Documents

Spark Moderator’s Guide

Recording# 1

Recording# 2

Recording# 3

Recording# 4

webchick’s picture

Wow, I'm really excited about the positive emotional reactions from people. :D Awesome news!!

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.

webchick’s picture

OMG AND RECORDINGS, TOO!! THANK YOU!!!! :D

dcmistry’s picture

I am excited too! :)

Regarding save/publish - I had a chat with Kevin too and I also thought that his approach works.

ry5n’s picture

This is great news. Congrats to the Spark team and thanks @dcmistry for doing this testing!

Bojhan’s picture

Title: Entity-level toolbar, rather than saving per-field, auto-revisions on edit (with automatic log messages) » Edit should provide a usable entity-level toolbar for saving fields

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.

Anyways, quite excited too :)

cbrookins’s picture

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"

So recommending the two states be "Save Draft" and "Publish"

eigentor’s picture

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

dcmistry’s picture

Glad to hear that you found the study and the videos helpful.

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.

tkoleary’s picture

@cbrookins

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

David_Rothstein’s picture

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.

Wim Leers’s picture

#55: correct. I'm fairly certain that's what Kevin (@tkoleary) meant :) It's just going to leave the node in the same state.

dcmistry’s picture

Agree to #55. Perhaps, "Save and Publish" or "Publish" (or whatever we plan to call it) could be the primary action and the "Save as Draft" be the secondary action.

Wim Leers’s picture

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

lightsurge’s picture

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

Wim Leers’s picture

+1 for #59.

lightsurge’s picture

#55

[]...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?

jessebeach’s picture

That's out of scope here though obviously, but could pave the way for some nice admin-enhanced themes

Right, I'd much rather see contrib address complex workflows.

jessebeach’s picture

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.

A screenshot of a node being in place edited. The fields are highlighted blue. The entity is highlighted with a red dotted outline.

This patch requires that you apply the following first #1818560-40: Convert taxonomy entities to the new Entity Field API.

jessebeach’s picture

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.

I took the changes from this patch, http://drupal.org/files/refactor-create-out-1966704-11-do-not-test.patch, combined them with the patch in #63 above and based them all on #1818560-40: Convert taxonomy entities to the new Entity Field API. I did this with an approach that could only be described as "code pulverizing". I want to get this patch posted though so that the progression is captured in the issue.

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.

MustangGB’s picture

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.

These are what my thought were whilst watching the four UX videos.

jessebeach’s picture

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.

Wim Leers’s picture

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

Thank you :)

David_Rothstein’s picture

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

David_Rothstein’s picture

I also have a question on the implementation end: Why is this issue related to #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits at all? (I'm asking mainly because that one seems to have some very thorny problems to solve.)

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

jessebeach’s picture

This is another progress-update patch. Expect another tomorrow.

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.

Edit: This patch requires that you apply the following first #1818560-40: Convert taxonomy entities to the new Entity Field API.

jessebeach’s picture

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.

This patch requires that you apply the following first #1818560-40: Convert taxonomy entities to the new Entity Field API.

n.b. that there's a small issue in core with the Editor module. You may need to apply the attached editor-bug.patch for field saving to work.

Wim Leers’s picture

FileSize
109.16 KB
171.07 KB

Heroic work, Jesse! There are no other words for this :)


On the move away from Create.js:

  • 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.
  • https://github.com/bergie/create/issues/142
  • 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.

Note that:

             // Update the metadata cache.
             _.each(results, function(metadata, editID) {
               Drupal.edit.metadataCache[editID] = metadata;
+              // @temporary HACK: use the 'form' editor for all fields
+              Drupal.edit.metadataCache[editID].editor = 'form';
             });

(for simplified refactoring for now; I'll fix that aspect later)

Wim Leers’s picture

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

Wim Leers’s picture

Now using Backbone.Model.validate, and hence got rid of setState(X) in favor of set('state', X). Now also ensures only valid states are accepted.

(I wanted to post this separately because it's such an important interdiff.)

Wim Leers’s picture

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

Jesse, you can already work on getting saving to work again.

jessebeach’s picture

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.

Wim Leers’s picture

#76: WOW, that looks *so* much cleaner already!

Turns out I'm too tired to start working on the save stuff, for which I need my full attention. That's for tomorrow morning.

Simple changes here:

  • a bunch of clean-up
  • fixes in findEditableProperties()
  • removed contextual-old.js, which I accidentally snuck in in one of the above patches — sorry :(
webchick’s picture

Status: Active » Needs work

Moving to needs work to reflect that there are patches here, but they're definitely not ready for real review yet.

jessebeach’s picture

This patch introduces an entity-level toolbar.

These are the issues I'm trying to wrestle down.

  1. 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.
  2. 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.
  3. 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.
  4. 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.

if ('activeFormatID' in settings.editor.formats) {
  Drupal.editorDetach(field, settings.editor.formats[activeFormatID], trigger);
}
else {
  console.log('%c editor.js: The format ' + activeFormatID + ' does not have an editor.', 'background-color: red; color: white;');
}

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.

webchick’s picture

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.

Wim Leers’s picture

Agreed with #80.

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:

  1. saving of fields using the 'form' editor works again locally! Making it work with the other editors should not pose major obstacles.
  2. 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’s picture

I've moved the Create/VIE changes into their own issue #1979784: Factor Create.js and VIE.js out of the Edit module.

This issue is now concerned with only the changes necessary to introduce an entity toolbar.

jessebeach’s picture

Progress update

  1. The entity toolbar now loads field toolbars into its DOM node.
  2. The entity toolbar shifts to be on-screen when it collides with a viewport edge.

For the next update

  1. Canceling the edit of a field should not cancel the active state of an entity that is being edited. The entity toolbar should return to being positioned above the entity.
  2. The sizing and position of elements in the Toolbar needs refinement.
  3. Clicking save should save all the changed fields in the entity.

I made a short screencast that illustrated how the entity toolbar shifts to be on screen if a user scrolls it off screen.

http://screencast.com/t/vHWQMTYhR

This patch is rebased on #1979784-6: Factor Create.js and VIE.js out of the Edit module applied to 8.x HEAD.

arosboro’s picture

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

Wim Leers’s picture

#84: Please note that this is not yet in a reviewable state. And your first answer (first the patch in the other issue then this one) is the correct one.

arosboro’s picture

@Wim Leers,

Thanks I'll work on other issues in the meantime and follow this thread until it's marked for review.

jessebeach’s picture

Rerolled on

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

applied to 8.x HEAD (891f4c5d6928d122bc1b255327f7077599f3eab3).

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.

Wim Leers’s picture

Once this is complete, we should also make this functionality accessible: #1844220-15: Make in-place editing keyboard and aurally accessible. Or maybe that can or should be done as part of this issue, that's TBD.

jessebeach’s picture

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)

  1. 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.
  2. When a field is changed and saved, it is not updated correctly.
  3. Canceling changes to field does not restore the field's previous value.
  4. 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.
klonos’s picture

I gave it a quick spin and had a couple of issues (Win7 x64 on firefox x64 nightly). These issues would have been more obvious if only I had taken a video screencast...

I created a simple article node with a "test article" title, a "test" tag and a "test body" body. Then, while in view mode, I clicked on the contextual pencil icon and selected "Quick edit".

- Hovering over the tag and body displayed the floating toolbar with a proper label for each of the tag and body fields.

- Clicking on the body field expanded the toolbar to display the buttons, but it was actually hiding the body text underneath it. I couldn't edit it.

- Clicking on the tag field brought the field in edit mode but it also enabled the "save" button even without having typed anything.

- The tag field height was a few pixels taller than it should, so it showed part of the blue spinning throbber along with the gray static one.

- I was locked in the edit state with the toolbar disappeared and I couldn't click out of the tag field edit mode.

Wim Leers’s picture

#92: from #90:

Current issues (among many)

This patch is *not* marked as "needs review", so please don't waste your time on doing that :) We're very much aware that this is is still very rough and buggy :)

jessebeach’s picture

klonos, we'll very much appreciate your detailed eye for review when this code is working. Look for it to change to needs review and then go crazy!

Wim Leers’s picture

Yes, that's what I tried to say but did so pretty poorly :)

klonos’s picture

Yes, I failed to see status. My bad.

jessebeach’s picture

This patch further moves state management of the fields into FieldModel. This allows the EntityModel to get a list of 'editing' fields like this:

var editingFields = this.get('fields').filter(function (model) {
  return model.isStateOfType('editing');
});

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']

Built on D8 (50ae3296382669baebee8b9afa909361eb5aa412) with following patches applied:
#1979784-12: Factor Create.js and VIE.js out of the Edit module
#1901100-24: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

jessebeach’s picture

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:

  1. Clean up the saving code in the EntityModel.
  2. 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.
  3. 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.
  4. We need unload warning messages that keep a user from navigating away if they have unsaved changes.
  5. 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.

Built on D8 (686c269bae7de4d84d4dd241c9b9f9092ece4f01) with following patches applied:
#1979784-18: Factor Create.js and VIE.js out of the Edit module
#1901100-24: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

lightsurge’s picture

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.

Shouldn't 'Save' in the toolbar actually always save all changes i.e. save the complete entity?

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.

lightsurge’s picture

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.

Hope that makes sense.

lightsurge’s picture

This would be ideal as I see it:

3 buttons.

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

jessebeach’s picture

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?

Bojhan’s picture

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

Wim Leers’s picture

Bojhan++ :)

The feedback is well-intended, but this is just the wrong time :)

lightsurge’s picture

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

Bojhan’s picture

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

jessebeach’s picture

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

  1. Clean up the saving code in the EntityModel.
  2. 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.

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

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

Built on D8 (75c407547fab8db09022479facf69460ab096b92) with following patches applied:
#1979784-18: Factor Create.js and VIE.js out of the Edit module
#1901100-24: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

falcon03’s picture

So I think this patch is not ready for an accessibility review, is it?

@Jessebeach, you wrote:

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

I didn't review the patch at all, but the toolbar should convey this information also without relying on colors to be accessible. E.G. Using colors and a (eventually invisible for sighted users) text.

jessebeach’s picture

I didn't review the patch at all, but the toolbar should convey this information also without relying on colors to be accessible. E.G. Using colors and a (eventually invisible for sighted users) text.

Indeed falcon03, what I should have written is: "When a field is changed and 'saved' but the entity hasn't been saved, we need to indicate in the markup that it's dirty."

This patch isn't ready for accessibility review, but it is definitely ready for accessibility development.

jessebeach’s picture

This path should clear up the issue #2 for comment #107

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.

jessebeach’s picture

This is a rebase on top of the following patches.

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

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.

  1. Saving an entity no longer works. Not sure if this is a tempstore problem or a entity save problem.
  2. The entity toolbar no longer repositions itself to the field being edited.
Wim Leers’s picture

Posting an initial reroll already, because it fixes #111.1 & #111.2 — it's fully functional again.

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

Wim Leers’s picture

And now the CKEditor toolbar also shows up again — everything is working as in #111 again AFAICT.

Wim Leers’s picture

(This is the last small update, I promise.)

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.

(I know I'm stepping on #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits's territory here, we can move changes around as is appropriate later.)

Wim Leers’s picture

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:
    1. 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
    2. 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!
  • I think this is wrong:
    @@ -292,7 +420,7 @@ Drupal.edit.AppView = Backbone.View.extend({
         if (_.indexOf(this.singleEditorStates, to) !== -1 && this.model.get('highlightedEditor') !== fieldModel) {
           this.model.set('highlightedEditor', fieldModel);
         }
    -    else if (this.model.get('highlightedEditor') === fieldModel && to === 'candidate') {
    +    else if (this.model.get('highlightedEditor') === fieldModel && to === 'candidate' || to === 'inactive') {
           this.model.set('highlightedEditor', null);
         }
    

    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.
  • Toolbars for display:inline fields are broken:
    -      this.$el.prependTo(this.$editedElement.offsetParent());
    -      var pos = this.$editedElement.position();
    -      this.$el.css('left', pos.left).css('top', pos.top);
    +      this.$el.prependTo(this.$field);
    

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

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?

I'm not sure what an "inline toolbar" is. I think this is what gets rendered on a plain text field, right? When the editor is rendered in overly, e.g.

Screenshot of what I think is an inline toolbar on a plain text field in in-place editing mode. But I am not sure.

I've never quite understood why this mode of editing is necessary. In other words, why doesn't the editing happen like it does with CKEditor?

Wim Leers’s picture

#116:

  • 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 :)
jessebeach’s picture

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();

// If a field is highlighted, position against it.
var highlightedEditor = Drupal.edit.app.model.get('highlightedEditor');
var highlightedEditorView = highlightedEditor && highlightedEditor.editorView;
var highlightedEditedElement = highlightedEditorView && highlightedEditorView.getEditedElement();
Wim Leers’s picture

Simplification! Awesome :) Very, very happy to be relieved of that edge case!

jessebeach’s picture

I took the changes to fix tempstore in #114 and moved them to #1901100-27: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits.

This patch also fixed the contextual visibility. It is now based on an active entity, not an active editor.

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

jessebeach’s picture

This address Wim's first comment in #115:

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

jessebeach’s picture

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.

Bojhan’s picture

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

jessebeach’s picture

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

Bojhan’s picture

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

Wim Leers’s picture

#125: That's how the prototype works: http://www.acquians.info/Public/Edit/spark/blocks/index.html :)

Bojhan’s picture

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

Wim Leers’s picture

@Bojhan Maybe we can do a quick call so I can explain this? I still think there is some misinterpretation going on.

jessebeach’s picture

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.

Built on ff6e1eea6ea2b2b343a4e03e172e6f5a05079dcb
#1996378-6: Edit broken because of #1043198 and routing system bug + missing test coverage
#1901100-29: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits, altered slightly to deal with a conflict in EditController because of changes in 1996378-6. This will be resolved as soon as this dependent patch is committed to head and we make a reroll of 1901100. Don't worry about it now. The code here is just for posterity.

jessebeach’s picture

The following scenarios should now be supported.

Save
Cancel
Change field > Cancel > Save
Change field > Cancel > Cancel

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.

Built on
6c1c3b9b96c3c31bb761e6d4dc56a15bb3eaa061
#1901100-37: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

jessebeach’s picture

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.

Built on
0979e1f5876a3d529320cceaede1770bda425ed8
#1901100-37: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

jessebeach’s picture

Rerolled because of HEAD conflict.

Built on
affc74ccddbd92b88d9fb23756a0c20df4b19a2c
#1901100-37: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

jessebeach’s picture

I fixed the disappearing save button issue mentioned in #130

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.

Built on
ace3459c4bd3ad50db8d49b700dde202d01e419d
#1901100-37: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

jessebeach’s picture

I've gone through the code and cleaned up the CSS and JSHinted the JavaScripted. All functions that previously had no or insufficient docblocks should be marked up sufficiently now.

I've also gone through the patch line by line and cleaned out any crufty leftovers from development up to this point. From here out, we're working towards commit.

I've written tests in the FAT module, http://drupalcode.org/project/fat.git/blob/HEAD:/tests/edit.tests.js

These are some of the issues I'm still seeing.

  1. The _pad and _unpad functions in EditorDecorationView are not working perfectly. Opening and closing a field to edit it often results in the field slowly scooting across the screen.
  2. The edit-changed class is lost on mouseover when a field has been edited and then another field is activated.
  3. When a field is active, clicking an inactive field doesn't always activate it immediately. Sometimes you need to click it twice.
  4. After the entity is saved, it should be deactivated rather than letting the user continue to edit.
  5. The entity toolbar needs to be pushed from the edge of the screen with Drupal.displace, otherwise it can get lost behind the Toolbar. This needs to be calculated in EntityToolbarView.position.
jessebeach’s picture

Issue summary: View changes

Updated issue summary. Incorporated #1741692: Discard via ESC button.

klonos’s picture

JSHinted the JavaScripted

:D

tkoleary’s picture

Couple of things I noticed:

Bug: Quick edit gets repeated multiple times in the dropdown
Bug: Textarea with only one line of text. Wysiwyg toolbar covers the text on focus.
Bug: Overlapping field borders

See attached images

tkoleary’s picture

Issue summary: View changes

Updating the issue description

jessebeach’s picture

Issue summary: View changes

testing.

jessebeach’s picture

Issue summary: View changes

edit

jessebeach’s picture

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.

Built on
56f4362ad49ae274dfe6296678f78fb6bf6d7ed3
#1901100-45: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

Bojhan’s picture

@jesse Could you please mark it "needs review" when the usability team can look at it? Please do it before you make critical UX decisions, so we can review it.

Keep up the good work, its great to see this moving :)

jessebeach’s picture

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.

Wim Leers’s picture

Assigned: jessebeach » Wim Leers

Assigning to myself to prevent further patch rerolls while I work on a comprehensive review, along with a reroll to address some things that I already can address.

Bojhan’s picture

@Jesse Awesome, thanks! I totally understand this is a big effort, looking forward to seeing the result.

Wim Leers’s picture

Overall: awesome job :)

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:
  1. 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
  2. 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:
-        // Ensure only one editor (field) at a time may be higlighted or active.
-        if (from === 'candidate' && _.indexOf(this.singleEditorStates, to) !== -1) {
-          if (this.model.get('highlightedEditor') || this.model.get('activeEditor')) {
-            accept = false;
-          }
-        }
-    else if (this.model.get('highlightedEditor') === fieldModel && to === 'candidate') {
+    else if (this.model.get('highlightedEditor') === fieldModel && to === 'candidate' || to === 'inactive') {

Much like the same assumption in formEditor.js. Also checked with Jesse: also leftover, also undone.

EditorDecorationView.js
FIXED IN PATCH: EditorDecorationView was calling into AppView, which is unacceptable:
-    this.model.set('state', 'activating');
+    Drupal.edit.app.enableEditor(this.model);

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:

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

Built on

56f4362ad49ae274dfe6296678f78fb6bf6d7ed3
#1901100-47: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

jessebeach’s picture

+++ b/core/misc/ajax.jsundefined
@@ -136,6 +136,9 @@ Drupal.AjaxError.prototype.constructor = Drupal.AjaxError;
  * @code
  *    Drupal.behaviors.myCustomAJAXStuff = {
  *      attach: function (context, settings) {
+ *        // We don't want to override the original commands!
+ *        Drupal.ajax['edit-submit'].commands = {};
+ *        // Now make just the 'insert' command available.
  *        Drupal.ajax['edit-submit'].commands.insert = function (ajax, response, status) {
  *          new_content = $(response.data);

I don't understand this comment. Wouldn't the original commands object be overridden by the object literal? You would want something like this, no?

*        // We don't want to override the original commands!
*        Drupal.ajax['edit-submit'].commands = Drupal.ajax['edit-submit'].commands || {};
*        // Now make just the 'insert' command available.
*        Drupal.ajax['edit-submit'].commands.insert = function () {};
tkoleary’s picture

@Wim Leers

It's really coming together! Two things I noticed.

  • The container around the whole node before the user focuses on a field is not there.
  • Scrolling on a large text area when the wysiwyg sticks to the header it should include the entity bar (not hide it under the toolbar, see screenshot)

image

jessebeach’s picture

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:

  1. 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.
  2. 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.
jessebeach’s picture

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

The error above might need to be addressed in #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits.

I'm still processing the behemoth review/refactor from Wim in #142

Built on
56f4362ad49ae274dfe6296678f78fb6bf6d7ed3
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

Wim Leers’s picture

#143: the docs were lying before, what they claimed was not actually possible. That's why I tweeted "Once again, I'm fighting Drupal.ajax. This time because it can't deal with concurrency.
Dozens of hours lost. Sigh." — https://twitter.com/wimleers/status/344959451956142080.
I talked to nod_ about this and instead of the work-around that I implemented, he's actually fixing it right now: #2019481: JavaScript AJAX commands object is borked: it is shared among all Drupal.ajax instances, preventing scoped overrides. So, rerolling #146 on top of the patch there that makes the docs no longer a set of frustrating, time-sucking lies :)

#145.2: indeed, it's a too constraining assumption! It might be very useful for UX/clarity, but it's an assumption we cannot make.

#146: good catch :)


Attached reroll removes our Drupal.ajax mess work-arounds and instead relies on #2019481: JavaScript AJAX commands object is borked: it is shared among all Drupal.ajax instances, preventing scoped overrides for fixing it.

Gábor Hojtsy’s picture

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

jessebeach’s picture

Re: #148, I reinstalled and refreshed everything. The patch from #147 does not have the issue I raised in #146.

Reroll
cd8a251f5fe51f888c8df9c48efb6e771c6b2d4a
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

jessebeach’s picture

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.

I made a short video of the behavior: https://www.facebook.com/photo.php?v=988762275316&set=vb.501857&type=2

I altered the colors of the buttons to match the Seven button coloring styles.

I also had to add an exception case to the entityModel state change validation in order to allow the transition from changed to canceled to confirm-saved.

// Allow: deactivating -> committing
// Necessary to be able to commit from a confirmation dialog.
else if (from === 'deactivating' && to === 'committing' && context.confirmed) {
  accept = true;
}

Otherwise the entity won't save from a confirmation dialog.

jessebeach’s picture

Issue summary: View changes

edit

jessebeach’s picture

Issue summary: View changes

Added manual testing results to the scenarios in the issue description.

jessebeach’s picture

Issue summary: View changes

notes

jessebeach’s picture

Issue summary: View changes

added invalid test cases

jessebeach’s picture

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.

tim.plunkett’s picture

Priority: Critical » Major

This was tagged critical for being a "feature regression", which can't be true since this was a feature addition to D8.

Wim Leers’s picture

#1363112: Simplify names of "element-x" helper classes broke #151 in a very minor way.


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.

I'm getting closer, but I don't see the solution just yet.

Wim Leers’s picture

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

! :)

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.

Wim Leers’s picture

Issue summary: View changes

removed a strong tag

jessebeach’s picture

Adding images to put in the description block.

jessebeach’s picture

Issue summary: View changes

added another test case

jessebeach’s picture

Issue summary: View changes

added some images

jessebeach’s picture

Issue summary: View changes

updating manual tests.

jessebeach’s picture

Issue summary: View changes

updated manual tests.

jessebeach’s picture

Issue summary: View changes

Updated manual tests.

jessebeach’s picture

Issue summary: View changes

added more tests

jessebeach’s picture

Issue summary: View changes

updating tests

jessebeach’s picture

Issue summary: View changes

updating tests

jessebeach’s picture

Issue summary: View changes

updating.

jessebeach’s picture

Issue summary: View changes

update

jessebeach’s picture

Issue summary: View changes

test updates

jessebeach’s picture

Issue summary: View changes

updated tests

jessebeach’s picture

Issue summary: View changes

updated tests

jessebeach’s picture

Status: Needs work » Needs review
FileSize
20.1 KB
135.77 KB

Between #154 and #156, I added manual test cases 16 through 20. All of them failed initially. I have them all passing manual tests now. With test #3 (image save) resolved by the commit of #2004350: Unserialised database connection objects don't use Drupal's PDO Statement class, we now have full manual test passage on this feature. I'm happy to open it up to code and ux reviews.

I made a video that illustrates how the in-place edit features looks and responds: http://www.youtube.com/watch?v=FA050kmyPZY&feature=youtube_gdata_player

You can evaluate these updates on simplytest.me: http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files...

You'll need to create an article node through the content form before you can quick edit the content in the node's view mode.

This patch was built on:
712fe2a18dce3581d3301b43777ac6749ca6c502
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

Status: Needs review » Needs work

The last submitted patch, entity-toolbar-1678002-75.patch, failed testing.

jessebeach’s picture

I discovered some issues with formEditor form changes when those changes come back as invalid. The issues are resolved with this patch.

jessebeach’s picture

Status: Needs work » Needs review

setting to needs review.

jessebeach’s picture

Oh forgot to add rebase info and testing info to #158

You can evaluate these updates on simplytest.me: http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files/edit-tempstore-1901100-48.patch&patch[]=https://drupal.org/files/entity-toolbar-1678002-158.patch

You'll need to create an article node through the content form before you can quick edit the content in the node's view mode.

This patch was built on:
5f67fd1d0858c6f52b4f2685a45b673d6b33e449
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

Status: Needs review » Needs work

The last submitted patch, entity-toolbar-1678002-158.patch, failed testing.

jessebeach’s picture

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.

Screenshot of the entity toolbar. The entity&rquot;s background color is darker. The small, triangular point is indicated. The point is pointing down.

Screenshot of the entity toolbar. The small, triangular point is indicated. The point is pointing up.

This patch was built on:
81b2685cb6c79cacdd49313f5928d79da84d7f8c
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

Bojhan’s picture

@jesse your last comment is about the UI, is it ready for such review?

jessebeach’s picture

@Bojhan, yes, ready for UX review!

And maybe I should include the patches this time :/ Derp on me.

You can evaluate these updates on simplytest.me: http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files/edit-tempstore-1901100-48.patch&patch[]=https://drupal.org/files/entity-toolbar-1678002-162.patch

You'll need to create an article node through the content form before you can quick edit the content in the node's view mode.

This patch was built on:
5f67fd1d0858c6f52b4f2685a45b673d6b33e449
#1901100-48: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits

Status: Needs review » Needs work

The last submitted patch, entity-toolbar-1678002-162.patch, failed testing.

tkoleary’s picture

@Bojhan

I would hold of on UX reviews on this for a bit there's just too much cleanup that still needs to be done before it even makes sense.

nod_’s picture

Status: Needs work » Needs review

Dumping a JS review:

in ContextualLinkView.js

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

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

EntityToolbarView.js

(that.$fence && that.$fence.css(offsets));
Regular if is better.

if ($('body').children('#edit-entity-toolbar').length === 0) {
        $('body').append(this.$el);

make a$body variable.

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.

As far as JS goes feel free to RTBC when tests are green.

Bojhan’s picture

@tkoleary Thanks for pointing that out, I will probably be able to review next week when I am back.

jessebeach’s picture

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

jessebeach’s picture

in ContextualLinkView.js

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

#1800022: Update Backbone and Underscore
#2029999: Refactor Drupal.AJAX calls in Quick Edit module JavaScript once #1533366 is committed.

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.

Status: Needs review » Needs work
Issue tags: -Usability, -sprint, -Spark

The last submitted patch, entity-toolbar-1678002-170.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

#170: entity-toolbar-1678002-170.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +sprint, +Spark

The last submitted patch, entity-toolbar-1678002-170.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
139.17 KB

This patch should fix the failing test.

I've also gone through a visual design review with tkoleary and incorporated his feedback. I'm posting updated images to the issue summary as soon as I post this patch.

jessebeach’s picture

Issue summary: View changes

updated tests

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

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:

  1. She wrote automated JS tests (in the https://drupal.org/project/fat module) that guarantee the body and tags fields of an article node can be correctly in-place edited: http://drupalcode.org/project/fat.git/blob/HEAD:/tests/edit.tests.js
  2. 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.

Note that this must be committed in tandem with #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits — first commit #1901100, then this issue. #1901100 has been ready for many, many weeks, but it only makes sense to commit it together with this patch.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Wim Leers’s picture

Status: Fixed » Active

Yay!

But: could you please also commit #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits? As I said in my last comment: that patch and this one must be committed in tandem.

jessebeach’s picture

Issue tags: -sprint, -Spark

Removing tags.

Wim Leers’s picture

Status: Active » Fixed
Issue tags: +Spark
jessebeach’s picture

Status: Fixed » Active

adding tags.

Wim Leers’s picture

Status: Active » Fixed

I think we cross-posted… :P Geez, what a mess did we create!

webchick’s picture

Not sure if it's strictly related to this issue, but I came across #2037687: Can no longer in-place edit body fields when testing today.

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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

Bill900’s picture

Bill900’s picture

Issue summary: View changes

updated screenshot images