Problem/Motivation
While working on #1678002: Edit should provide a usable entity-level toolbar for saving fields, it became more apparent that our Create.js/VIE.js architecture in the Edit module would not support (easily) the interaction pattern we're trying to build. Namely that one interactions primarily with an entity for in-place editing -- one edits fields in the context of an entity and saves changes in the context of that entity.
jessebeach took several days to determine if it would be feasible to factor Create/VIE out of the Drupal codebase #1678002-71: Edit should provide a usable entity-level toolbar for saving fields and we determined yes, we could do it in a reasonable amount of time and effort. Wim Leers developed these reasons for the refactor in #1678002-72: Edit should provide a usable entity-level toolbar for saving fields:
- 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.
The most salient reason from the list above is this one:
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.
We will keep #1678002: Edit should provide a usable entity-level toolbar for saving fields focused on the introduction of an entity-level toolbar for in-place editing. This issue here will become the home of the Create/VIE out-factoring.
Proposed resolution
Remove Create/VIE from Drupal. Refactor the existing Edit JavaScript to leverage Backbone Models and Views. This is already the case to some degree, but because Create uses jQuery UI to handle Widget creation and eventing, these aspects of the application will need to be ported to Backbone. Most of this porting is already completed and working as before.
Remaining tasks
Separate #1678002: Edit should provide a usable entity-level toolbar for saving fields into two issues and move the refactor code here.
Finish porting functionality to the new Backbone Models and Views.
User interface changes
See #1678002: Edit should provide a usable entity-level toolbar for saving fields.
API changes
Some, but since no code is leveraging these systems yet outside of Edit/Editor, the changes will not break anything.
Related Issues
#1678002: Edit should provide a usable entity-level toolbar for saving fields
Comment | File | Size | Author |
---|---|---|---|
#36 | edit-create-refactor-1979784-36.patch | 390.4 KB | Wim Leers |
#36 | interdiff.txt | 852 bytes | Wim Leers |
#30 | edit-create-refactor-1979784-30.patch | 392.78 KB | jessebeach |
#30 | interdiff_28-to-30.txt | 18.51 KB | jessebeach |
#29 | edit-create-refactor-1979784-29.patch | 383.89 KB | Wim Leers |
Comments
Comment #1
webchickTaggity tags.
Comment #2
jessebeach CreditAttribution: jessebeach commentedHere is the port from #1678002: Edit should provide a usable entity-level toolbar for saving fields with just the refactor code and none of the entity toolbar changes.
Comment #3
Wim LeersChanges:
EditorView
is back, was removed in #2. Now provides a base class for other in-place editors, with docs and some default method implementations.edit.js
toAppView
.EntityView
, but didn't remove the instantiations.findEditableProperties
.storage.js
is gone.Next: make the 'direct' editor work again.
Comment #4
henribergius CreditAttribution: henribergius commentedAll of the issues mentioned are definitely fixable, though I would've preferred communication along the lines "here are the things we need you to fix for us to be able to keep using it" than the way it is said now.
I still believe in benefit of using a common codebase shared between CMSs instead of everybody trying to reinvent (and maintain!) all the wheels themselves. For a longer version, please refer to http://bergie.iki.fi/blog/drupal-and-collaboration/
The question is, then: how set are you with doing all that by yourselves? Certainly the changes we made in the process of integrating Create.js -- like porting your Edit module to Backbone -- make this a lot easier. But maintaining the full editing stack is a lot of work. Trust me, I should know ;-)
Now, going to the concrete issues:
1. VIE is indeed a big library, especially if you build the services in. However, it is in no way locked into RDFa (or JSON-LD), but merely uses those as defaults. As I've said to Wim, there are several places and ways in which we can make VIE smaller if there is a need
2. jQuery UI widgets are a handy way of connecting stateful JavaScript code with DOM elements. The breakage caused by 1.10 was a bummer, of course. We already had plans to migrate the core functionality of Create to regular Backbone views and models, but to keep the jQuery widgets around as API compatibility wrappers
3. Create has per-field state handling, but with the assumption that the editors would drive state changes. In Edit, the idea is to move this around, which causes some complications. Again, fixable with a bit of time
4. Initially Create was, indeed, built with its own UI (and we still ship that as a "sane default"). But as the integrations with Drupal's Edit (Backbone), OpenCms (GWT), and TYPO3 Neos (Ember) have shown, it plays also quite nicely with a custom UI. The main assumption causing problems for Edit was already covered by 3.
5. This would need a bit more discussion
7. Sure, there would be at least some wins in download size. But at the cost of having more code "proprietary" to Drupal, and so maintained only by your community
Anyway, we can discuss this further when you have the report of your reimplementation done. I would've liked to hear about this sooner so individual issues could've been addressed one by one. But hey, I can't have everything!
Comment #5
Wim LeersWe'd like to expand on the context of how we reached the undertaking of this experiment. Last week (around 4/17/2013), jessebeach was trying to introduce an entity toolbar element to the Edit module (#1678002: Edit should provide a usable entity-level toolbar for saving fields). After a couple days of halting attempts to work this entity-layer of abstraction in the Create/VIE framework and our Edit JS on top of that, she decided to see what Edit might look like without Create/VIE. This was no simple matter and it took the entirety of a weekend to get a quick-and-dirty proof of concept in place that suggested it might be possible. The prototype also seemed to support an entity-level toolbar in a way that wasn't easily possible with the previous codebase.
Spurred by the initial results, we continued this week working this proof of concept into a patch (#1979784: Factor Create.js and VIE.js out of the Edit module — this issue), which we posted yesterday. This has been a fast process. We did not know what the results would be. We wanted to see what the code would look like. Our intent was to reach out to you, Henri, once we had our heads wrapped around what exactly we're proposing. We did not intend for you to be informed of this work through Twitter; rather, once we had code to compare (i.e. facts to compare instead of statements like "the code would be so much better if X and Y"), we wanted to reach out and have a discussion with you, which we planned to do today since this patch was posted late in the evening European time.
Given the design direction of the entity toolbar and the realities of Drupal's entity/field modeling, it looks more and more like we have to take a direction that moves us away from Create and VIE. We are truly sincere when we say that we always prefer sharing code rather than rolling our own. But requirements, performance, timelines and supporting the Drupal contrib space pose us a tough decision. We think the benefits of the approach we're proposing in this patch are substantial at this moment.
We reached out via e-mail to have a call about this ASAP.
Comment #6
jessebeach CreditAttribution: jessebeach commentedThis patch rebases on HEAD (9c996a8f78dd29df0270e5af15b4e8d172ebcc45).
Changes include:
stats:
46 files changed, 2305 insertions(+), 7674 deletions(-)
Comment #7
henribergius CreditAttribution: henribergius commentedCommenting on this, and the email report of your effort that you received.
Apart from Create.js itself, I believe a major contribution we did to the Edit codebase was porting it to Backbone. This gives me confidence that you'll be able to produce a maintainable inline editing solution, with or without Create.
However, by bailing out of Create, you're making a bet against the shared CMS JavaScript ecosystem. If merely integrating Create improved your own JS codebase, what would be possible once the multiple Create and VIE based editing UIs are actually out there and use? By withdrawing, you'll be missing out on whatever the world outside of Drupal comes up with.
As for the more concrete issues you've faced. I agree that the concept of Editables as integrated now doesn't fit Drupals "edit each field separately" paradigm. In Create the assumption is that people are editing and publishing entities as a whole, not just fields.
But this would be easy to work around by treating each Drupal field as a separate entity. This way you'd have all the control and state you crave with the existing features that Create can offer. And you could even use features like collection editing for dealing with multivalued fields (if Drupal has support for those).
That said, I see you're more or less determined to push ahead. I'm sad to see you go, as Drupal would've been a valuable participant in the decoupled CMS world. But can't win them all...
Comment #8
Wim Leers@henribergius, thank you for the considered response. We are fairly confident that the approach we're taking — removing the Create.js and VIE dependencies — is what we need to do at this moment in time given our current schedule and design constraints.
As a rule we always want to work with community projects that align with Drupal's needs. Even though that alignment shifted for us now because of RDFa & JSON-LD support and our entity-based architecture, that doesn't mean that we might not come back into alignment later. We might find ourselves revisiting this issue as we spin up Drupal 9. Jesse and I and the rest of the Drupal front end folks will take any and all options on the table and consider their benefits.
Comment #9
jessebeach CreditAttribution: jessebeach commentedreroll on (9ae545f7544dd7b66d6b2f9e95fc35277d8ed44a).
Comment #10
jessebeach CreditAttribution: jessebeach commentedThis patch reintroduces the save flow for direct editables. At least, that's all I've tested it with. It "might" work with form editables.
The validation code still needs to be cleaned up.
Comment #11
Wim LeersMaking Edit leverage
Drupal.TabbingManager
(#1844220-15: Make in-place editing keyboard and aurally accessible) is blocked on this issue.Comment #12
jessebeach CreditAttribution: jessebeach commentedRebased on 8.x (50ae3296382669baebee8b9afa909361eb5aa412).
Comment #13
Wim LeersWorking on a big step forward, please don't reroll in the mean time.
Comment #14
Wim Leersedit.css
whitespace changes. (That was all that was changed there.)Drupal.edit.EntityCollection
, likeDrupal.edit.FieldCollection
— to remove one-off code.ContextualLinkView.js
removed all changes in #1981760: Edit should use the new drupalContextualLinkAdded event to cleanly instead of hackily insert its "Quick edit" contextual link. Worse, all code inedit.js
has been merged completely incorrectly, leading to duplicate, erroneous code. Fixed all that. It was only working by coincidence… :)Drupal.behaviors.edit
, because nod_ doesn't like that — he prefers us to use functions that are only available within the closure. He thinks only(de)(at)tach
methods should exist onDrupal.behaviors[modulename]
. Complying with that.FieldModel
s be created, because otherwise they're just consuming memory and not doing anything useful.So we really need three asynchronously updated queues: 1) fields without metadata, 2) fields that have been verified to be in-place editable by the current user, 3) contextual links which qualify for a "Quick edit" link
edit.js
. At the same time, that rids us of organically grown complexity that needed to be refactored anyway, and of the last remnants of "updating the DOM to allow VIE to work cleanly". On the downside, our previous refactoring steps got rid of the ability to destroy things properly when a field got removed. That's now also fixed.There are now just a handful of functions in
edit.js
' closure, that I tried to make as clear and concise as possible:initEdit
,processField
,initializeField
,fetchMissingMetadata
,initializeEntityContextualLink
anddeleteContainedModelsAndQueues
. Everything is well-documented now.I had expected to work on more directly useful things, but this really needed to be done. The changes made in this interdiff should allow faster progress now though :)
Comment #15
Wim LeersThe patch in #14 contains one stupid small mistake.
Comment #16
Wim LeersIn this reroll
So, back to what I really wanted to work on. I said in #3:
Changes
backbone.drupalform.js
is no more. It doesn't make for us to useBackbone.sync
, because we do not have a REST API that allows us to create content *and* validate that content. It certainly does not make sense for the 'form' in-place editor, which is the one used for most fields currently (i.e. all non-textual fields, all structured data fields).EditService.js
contained work-arounds to distinguish between fields that would be "directly" in-place edited (i.e. the value itself would be in-place edited, which almost certainly equates to textual editing) and those that would not be. After all, a single field may contain many items (cardinality >1) or a single item (cardinality == 1). So, in short: before this patch it was very hacky, and all above patches did not yet deal with this.The new approach to deal with this is very simple: a new, required
EditorView.getEditedElement()
method:The 'direct' and 'editor' in-place editors can now just say "hey, we don't care about editing this field wrapper thing, we care about the first and only field item within the field wrapper".
FieldToolbarView
had to go and clean up the remnants of the hidden form that's being used to save data. That's now all taken care of byEditorView.save()
.Comment #17
jessebeach CreditAttribution: jessebeach commentedAwesome! This always felt like a hack, albeit a necessary one. The save method on EditorView is much more grokkable. In fact EditorView really exposes plainly the editing interaction model now. Before, it had been half in Create, half in Edit and it was unclear which code was responsible for what.
Awesome, awesome work Wim!
We're finally boiling the code down to just what's needed and little more.
I added overridden
remove
methods to the EditorDecorationView and the FieldToolbarView. We'd only been callingundelegateEvents
on the views, which is a temporary way to remove the views. It doesn't stop the view from listening to model change events. By calling the Backbone.View.prototype.remove method on the views, we remove it correctly.We're ready to start reviewing this code.
Wim suggested we might move the deletion of Create.js and VIE to another issue so that the diff here in this patch isn't so large.
Comment #18
jessebeach CreditAttribution: jessebeach commentedOops, I wanted the bot to run!
Comment #20
Wim LeersI created a minor follow-up task where we can document which name changes we want to introduce that are not necessary (i.e. don't belong) for this refactoring: #1989974: [meta] Clean up/improve consistency of Edit.
Comment #21
Wim Leersedit
library and into the page-level attaching. That way, the same settings don't get attached on every metadata callback. Much better.Next up: to a reasonable degree, simplify things and make them more consistent. Particularly
FieldModel
. Do some renaming too. Since Jesse's in agreement with the patch in #16, we can now do that last bit of work as we are approaching RTBC :)Comment #22
Wim LeersOops, I excluded #17. Fixed that. Interdiff at #21 is still valid.
Comment #24
Wim Leers#22: edit-create-refactor-1979784-22.patch queued for re-testing.
Comment #25
jessebeach CreditAttribution: jessebeach commentedWim Leers notes that the following issues can be closed as a result of this refactor, including one Major:
#1874934: Revise Create.js-related code when upstream issues have been fixed
#1855142: Refactor the data methods of Create.js objects out of the jQuery UI presentation layer
#1879898: Clean up EditService.js (preferably get rid of it in favor of RDFa) and get rid of Backbone.syncDirect in favor of JSON-LD
#1971368: Patch Backbone.sync safely
#1977292: EditorSelector::getAllEditorAttachments() rebuilds all attachments constantly
Comment #26
Wim LeersBig interdiff, but this is just changing indentation and removing
jQuery.extend
where it is not needed.Before:
After:
Comment #27
Wim LeersThis patch should be RTBC'able, or close to RTBC'able. Please review! :)
Changes
editID
in all JS code tofieldID
. It is now alsoFieldModel
'sid
attribute, instead ofeditID
in there. So: field ID (formerly edit ID) atfieldModel.id
, entity ID atentityModel.id
. Nice & simple.FieldModel
as far as possible!FieldModel
, into a newEditorModel
. As a side-effect,EditorModel
s only exist for the duration thatEditorView
s exist — which makes a lot more sense. (Previously, aFieldModel
would continue to remember the last state of the last in-place editor, even after quick editing of an entity was stopped.)AppView.(un)decorate
toAppView.(setup|teardown)Editor
.edit_contextual_links_view_alter()
->edit_page_build()
. More logical given recent contextual links changes. Copied from #914382: Contextual links incompatible with render cache.function(
/function (
/, because nod_ would definitely complain about that otherwise :)Also note the clean-up & consistency improvements list that got updated at #1989974: [meta] Clean up/improve consistency of Edit. We don't want to do those things here because those changes are unrelated to the task at hand: factor Create.js & VIE.js out of Edit module's JS.
When testing, try doing something like
Drupal.detachBehaviors(jQuery('.block-custom-block'))
, you'll notice thatDrupal.edit.collections.(fields|entities)
are updated, and in fact that the "Quick edit" link no longer appears in the custom blocks' contextual links! This never worked before.Before
After
Comment #28
Wim LeersFixed initial feedback from nod_:
EditorModel
not in patch. Oops!Drupal.edit.Metadata
should beDrupal.edit.metadata
because it's not a constructorjQuery
when the$
alias is in the closure.Comment #29
Wim LeersIndentation fix.
Comment #30
jessebeach CreditAttribution: jessebeach commentedI went through the patch line by line, which is surprisingly easy to do after the refactor. Most of the code is comments! And speaking of comments, I edited many of them where they varied slightly from our standard. Much of this code is left over from before the refactor.
I didn't touch any code, just comments.
In my opinion, we're ready to commit this. Any further changes should be addressed in #1989974: [meta] Clean up/improve consistency of Edit after the bulk of the refactor is in place and we can considering behavior changes in isolation.
Comment #32
Wim Leers#30: edit-create-refactor-1979784-30.patch queued for re-testing.
Comment #34
nod_Yesterday Wim and Jesse walked me through the patch and after test failures are gone and a little stray line is remove that's RTBC for me.
Comment #35
Wim Leers#30: edit-create-refactor-1979784-30.patch queued for re-testing.
Comment #36
Wim LeersThis removes the stray line that nod_ mentions in #34 (and fixes the line above).
Comment #37
nod_RTBC for me :) see #34
Comment #38
Gábor HojtsyI think it would be important to get this land sooner than later. The sheer size of it makes it too big of a target for rerolls.
Comment #39
alexpottAs this is removing libraries added to core and this is Dries's call.
Comment #40
jessebeach CreditAttribution: jessebeach commentedOur motivations for this change are detailed in the issue summary.
Comment #41
Wim LeersAnd before vs. after data in terms of # of bytes to load are available at #27. Number of bytes to download have been cut in half, both minified and minified + gzipped.
Comment #42
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #43
Gábor HojtsySuperb, thanks!
Comment #44
Wim LeersYay! :)
Comment #45
Wim LeersI've closed all these issues:
Yay :)
Comment #46
droplet CreditAttribution: droplet commented#1. Click quick edit
#2. Click CLOSE icon
#3. @see browser console
=============
#1. Click quick edit
#2. Click stop quick edit
#3. Click quick edit
#4. Click body text to edit
..
.
.
ERROR:
uncaught exception: The editor instance "editor1" is already attached to the provided element.
[Break On This Error]
...new CKEDITOR.dom.element(c):null},getSelectedText:function(){var a=this._.cache;...
=============
#1. empty body text
#2. no quick edit in menu
Comment #47
nod_need a new issue for the bug
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedThis was committed, so removing the "Avoid commit conflicts" tag.