Editor.js attach/detach doesn't pass context all the way down

Patch.

@nod_

the comment of findFieldForFormatSelector suggest it's not the right place to make the change

I don't think I get what you mean. How does the comment suggest that ?

findFieldForFormatSelector() is merely a glorified element selector ("find the DOM textarea pointed by the $formatSelector's data-editor-for attribute"), used as a helper for attach/detach.
Thus, as any code in attach/detach, it has to be scoped within a given context (which is the context that was used to find the $formatSelector that is being worked on in the first place) ?

Taking a step back - the specific logic here is the following :
- Some behavior needs to be attached to a pair of elements (a "format select" + the associated textarea). The association is representend by the format select having a data attribute pointing to its textarea's ID.
- The current attach logic is:
Find the format selects present the "attach context"
Then look for the associated textareas not specifically in that same context, but (implicitly, by the absence of a context for the jQ selector used) in the toplevel document.
This breaks when the format select is not in the toplevel document to begin with.

So either:

1) (current patch) The textarea needs to be looked for in the same "context" than the select.
This is consistent with the general rule that "all DOM manipulations within attach/detach have to be scoped to the context of the attach/detach"
But it implies that the select and its textarea are always shipped alongside (you never handle an "attach context" that contains one and not the other). That sounds like a reasonable assumption, but I'm not 100% sure...

2) Or, the textarea needs to be looked for in the same "document" than the select:

var field_id = $formatSelector.attr('data-editor-for');
return $formatSelector.parents('body').find('#' + field_id).get(0);

That looks a bit weird without an explanation though.
Maybe, going up a little less, up to a known common parent ? '.text-format-wrapper' ?

3) Or, the code needs to be bullet-proofed for the fact that maybe the associated textarea will not be found where it is currently looked for (in the toplevel document)

Ideally, we'd do 2), I guess. Question is: can we trust the select and its textarea will have a known common parent (that doesn't depend on theme verbosity & classes...) ?

No, the idea is indeed to attach behaviors within CK (iframe or divarea)

The case here is a combination of several things :

- Entity_embed lets you embed "a rendered entity in a given view mode" inside a body field, and have a faithful preview in CK. This is intended as "the unified D8 solution for embedding media in textareas", whatever the various underlying media data model (Media, Scald...).
- The API for rendering an entity (entity_view($entity, $view_mode)) doesn't let you specify "BTW, this one is going to be rendered in a wysiwig, so you might want to output things a little differently". This also wouldn't play nice with the render cache. So the HTML you get in the wysiwyg is strictly the same as the one used in front end.
- Thinking of "media" use cases, for example, there are totally cases where that HTML might need some JS massaging - which are typically implemented with behaviors.

Taking actual examples from a recent D7 client case (the one I showed you in Amsterdam :-) :
- picturefill needs to scan the new content for picture tags, or images do not show on browsers without native picture support
- an image lazy-load script needs to scan the new content for imgs, or images do not show
- we also had some some custom "contextual-links"-like code to allow editing the media in a popup, from the embedded representation in the wysiwig; that's implemented with behaviors too - because behaviors are our tool for this.

So, running behaviors in there makes a lot of sense IMO. That's what the patch in https://github.com/drupal-media/entity_embed/pull/103 does.

Then, if you embed a node in a view mode that displays the comment field - yeah, you get a comment textare displayed inside your CK, and editor.js behavior trying to run on it ...
Weird case, and of course we don't expect to have that textarea to get its own CK... but at least we'd like the JS not to break :-)

But wouldn't it make sense to then exclude Drupal.behaviors.editor.attach()

I've been toying with that idea as well - but how would we do that ? Drupal.attachBehaviors() is currently all or nothing.

I guess maybe entity_embed could use some JS foo to manually redefine a couple selected Drupal.[some_component].attach() to do
"if (context is within a CK) {return;}; call the original implementation" ? You can do such things in JS right ?
(not really clean though, open to other suggestions...)

Also - we intend to support <picture> in our core image CK plugin at some point, right ? Then, the question of running behaviors in CK content will arise in core too ?

Well, or I guess that specific case can manually trigger picturefill() calls on the new content instead of going through the generic behaviors attach. Entity_embed is much more generic, and so cannot know specifically which behaviors to run.

Regarding the "bug" in editor.js - what about this ?

- keeps the current lookup logic (look for the textarea in the top document, don't write code to support the weird case of textareas in iframes / CK inception),
- just safeproof the attach / detach code to do nothing in case no textarea was found
- side-effect : patch finds the textarea once, and then passes the [format select, textarea] pair to the various helpers, instead of having to find the textarea again each time

@Wim : thanks, did that (https://github.com/yched/entity_embed/commit/1f0a4bc36b16a5a8cb912475c0b...).

I still think safeproofing the way the [select, textarea] pair is passed around, as proposed in #11, would make sense, but no biggie.

So #11 doesn't declare or use the context parameter of findFieldForFormatSelector is that wanted

Indeed, those were leftovers. Removed.

+ Adds the missing doc for the new "field" param added to changeTextEditor()

@Wim Leers:

In current HEAD, editor.js behaviors attach/detach:
- finds all DOM elements with the '.editor' class,
- assumes they have a 'data-editor-for' attribute, containing the ID of an element present in the top-level document (does not intend to support textareas in iframes)
- and then does stuff on that pair of elements, confident that all the conditions above were met and that the pair was found.

The script breaks, messing with the rest of the JS execution on the page, when that pair was not found for whatever reason.
That could be :
1) because the '.editor' element is completely unrelated (that class is not really namespaced and is easy to clash with)
2) because some code tried to attach/detach behaviors on an element in an iframe - which is not a completely crazy thing to do (and there could be other cases than CK iframes ? For example, our popups / modals currently happen to be divs, but there are other, iframe-based approaches to popups ?)

For the above, and independently of which actual cases it choses to support (CK in a CK [obviously not], CK in a non-CK iframe),
I think editor.js should gracefully degrade to "not for me, just do nothing" on the cases it choses *not* to support, rather than telling you you did a forbidden thing by breaking JS execution.

The latest patches try to do that : only "do the stuff" if the two elements were actually found, for the cases it intends to support.
Then, the question of "what exactly do we want to support" (CKeditor in a non-CK iframe ?) can be discussed separately.

Writing the above, I notice that the patch does not fix case 1) above (an '.editor' element without a 'data-editor-for' attribute).
Could be that $(context).find('.editor') would need to be refined to $(context).find('.editor[data-editor-for]') ? I thought the trend for "behavior markers" in the DOM was to move away from marker classes and rely on data- attributes instead, for that matter ?

I thought the trend for "behavior markers" in the DOM was to move away from marker classes and rely on data- attributes instead, for that matter ?

Grepping for "$(context).find('[" in core leads to:
states.js : behavior based on [data-drupal-states]
vertical-tabs.js : ... [data-vertical-tabs-panes]
node.preview.js : ... [data-drupal-autosubmit]
quickedit.js : ... [data-quickedit-field-id] / [data-quickedit-entity-id]
navbar.js : ... [data-drupal-nav-tabs]

So it does seem like a common, if not fully unified, approach ? Even though we are a bit inconsistent on how exactly to namespace those marker attributes (data-drupal-[component], data-[component]...)

@nod_ : if the 'drupal-' namespace is the most recent convention, then data-drupal-editor-for ?

re #25:

1. We can drop the $formatSelector argument entirely [in changeTextEditor()] if we're passing in field anyway.

The last line of the function sets an attribute on the $formatSelector, so it seems we still need it ?

2. if (!field) {return} can go away once the selector is converted to using the data- attribute only.

I don't think so :
- "Using the data- attribute only" takes care of case #19.1 (a DOM element with the 'editor' class that is just a false-positive).
- That if (!field) return is what takes care of case #19.2 : do nothing if findFieldForFormatSelector() could not find a textarea that we intend to support (in current code, that's a textarea in the top-level document).

Attached patch ties the behavior to the data-editor-for attribute instead of the .editor class.

re @Wim #27 - Putting data-editor-active-text-format on the textarea rather than the select :
Oh, right. Looks like a slightly larger refactor, but could be done I guess. Will look into it.

Updated patch does as suggested in #27.1 : simplifies changeTextEditor() by storing the data-editor-active-text-format on the field (textarea) rather than on the select.

+ Since the field knows its currently active format, no need to pass that current format as a separate param ?

--> patch does:

-  function changeTextEditor($formatSelector, field, activeFormatID, newFormatID) {
+  function changeTextEditor(field, newFormatID) { 

- Slightly clearer var name within changeTextEditor() (s/activeFormatID/previousFormatID)

- While I'm in there, simplify if (cond) {...} else if (!cond) {...}
to if (cond) {...} else {...} :-)

Reroll

Fixed :-)