Fields which are "dnd-enabled" are assigned a default context, which is currently unused in "normal" use case.
The patch below implements :

  • Default contexts are saved in a variable dnd_context_defaults which is an array[bundle][fileld].
  • DnD-enabled textareas are tagged with the default context of their bundle/field-name.
  • On client side, atom rendering is deferred from drag event to drop event, where correct context can be selected, and eventually fetched.
CommentFileSizeAuthor
#14 ie8_daggable-1995238-18.patch3.92 KBdDoak
#11 scald_default_context_per_field-2013823-11.patch4.39 KBjcisio
#10 scald_default_context_per_field-2013823-10.patch4.89 KBjcisio
#8 scald_default_context_per_field-2013823-8.patch3.17 KBjulien_g
#7 scald_default_context_per_field-2013823-7.patch2.64 KBgifad
#6 scald_default_context_per_field-2013823-6.patch5.93 KBgifad
#5 scald_default_context_per_field-2013823-5.patch5.63 KBgifad
#1 scald_default_context_per_field-2013823-1.patch4.8 KBgifad
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gifad’s picture

gifad CreditAttribution: gifad commented
FileSize
scald_default_context_per_field-2013823-1.patch4.8 KB

The patch, attached.

Additionally :

  • dnd-library.js have been fixed of erasing atom's legend when merging new context.
  • editor.css includes settings for field-label, which is usually provided by field.css, which is not included at edit time

As a performance improvement, all the "normally" usable contexts could be embedded in the library items, but I could not find a way to do it without disturbing dnd architecture on client side...

Please consider,
Regards

jcisio’s picture

jcisio CreditAttribution: jcisio commented
Status: Active » Needs work

It is in general a good idea. However the try catch block is there for IE compatibility, it should not be removed. Also pay attention to coding standards.

gifad’s picture

gifad CreditAttribution: gifad commented

In the current implementation, the try block uses dataTransfer 'text/html', which is no more used in the patch;
The patch uses 'Text', which was in the catch block;
So what could I try/catch ?

BTW, it doesn't work in IE8, but the same way as the demo site...
I'm sorry I have no IE9 machine to test with..

jcisio’s picture

jcisio CreditAttribution: jcisio commented

Yes, it works with IE9+ and we should keep it. Also I think text/html is necessary because it helps to drag and drop to a place where we don't have control on the drop event.

Here is a more thorough review:
- Unnecessary or unrelated CSS change.
- Instead of using dnd_context_defaults, I think it'd be more DX friendly to add an option into field instance settings (like "context_default")
- I don't understand on the legend trick in JS.
- Coding standards: two spaces instead of tab.

gifad’s picture

gifad CreditAttribution: gifad commented
FileSize
scald_default_context_per_field-2013823-5.patch5.63 KB

- unrelated CSS change : ok, it's another issue : missing field.css for proper rendering of Authors and Tags in wysiwyg;
- add an option into field instance settings : this option exists, it's just named "Scald Editor Context " and described as "Choose a Scald Context to use for displaying Scald Atoms included in the textarea during editing.";
I added a variable as I am not able to retrieve this vale in the dnd_process_textarea() function, where I need it;
Also, this setting defaults to "Debuging display", so I preferred not to use it until users explicitly specify it...
(Yet another issue : mee.module should filter "hidden" contexts out of this list...)
- legend trick in JS : when an atom is fetched via ajax, the data received contains an empty (not NULL) legend, so the $.extend(true, Drupal.dnd.Atoms[atom_id], data[atom_id]); empties the atom legend (just try "Edit atom properties > Context > Full page")
- Coding standards : done.

Additionally, I managed to embed all "normally available" contexts in scald_dnd_library_add_item() (the "contexts" property of Atoms was already ready to receive it), so I could remove fetchAtom() call in drop callback;

gifad’s picture

gifad CreditAttribution: gifad commented
FileSize
scald_default_context_per_field-2013823-6.patch5.93 KB

Fixed the missing hidden context filtering in field settings :

gifad’s picture

gifad CreditAttribution: gifad commented
FileSize
scald_default_context_per_field-2013823-7.patch2.64 KB

To preserve IE compatibility, the try/catch blocks in drag and drop callbacks have been restored to original state.
The context information is now picked in plugin instanciation, and overrides Drupal.settings.dnd.contextDefault.
The major drawback is that the default context is the same for all textareas of the content-type...

dnd_context_defaults variable has been thown away, leaving mee.module out of the patch;
default context information is now retrieve in dnd_process_textarea() via $form_state['field']['body']['und']['instance']['settings']['context'];
This was obvious, but I'm getting old, and couldn't see it immediately ;)

Other unrelated "minor" fixes removed;

julien_g’s picture

julien_g CreditAttribution: julien_g commented
Issue summary: View changes
FileSize
scald_default_context_per_field-2013823-8.patch3.17 KB

I applied this patch to be able to configure my delfault contexts per field.
2 issues about this patch :
- the was and IE bug that makes IE only use the general default context (sdl_editor_representation). I fixed this bug.
- I was unable to apply the patch on scald 7.x-1.x-dev because of the improvments in the targeted modules. I have adaptated the patch to the last dev version.

But this idea is a good way to use the field instance setting context for default editor context.

jcisio’s picture

jcisio CreditAttribution: jcisio commented

Looks ok, except:

+++ b/modules/library/dnd/dnd.module
@@ -75,6 +75,8 @@ function dnd_process_textarea($element, $form_state) {
+    $default = $form_state['field']['body']['und']['instance']['settings']['context'];

Should not hardcode the field name and language.

jcisio’s picture

jcisio CreditAttribution: jcisio commented
FileSize
scald_default_context_per_field-2013823-10.patch4.89 KB

New patch introduces another "default context" in the textarea (#4), and correct the other setting on fallback context. It also fixes the hardcoded field name/field language.

However, default context per field should not be stored as a global state (mention in #7). It needs to be fixed.

I also don't think it is a good reason to preload all atoms with all contexts. It unnecessarily fills the cache because usually not all contexts are used for every atom.

jcisio’s picture

jcisio CreditAttribution: jcisio commented
Status: Needs work » Needs review
FileSize
scald_default_context_per_field-2013823-11.patch4.39 KB

I think it is a quite nice clean up. Didn't test with IE though. So please test & review ;)

jcisio’s picture

jcisio CreditAttribution: jcisio commented
Status: Needs review » Fixed

Tested again with IE9 and it works. So committed 9f0849d and pushed. Thanks all!

Status: Fixed » Closed (fixed)

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

dDoak’s picture

dDoak CreditAttribution: dDoak commented
FileSize
ie8_daggable-1995238-18.patch3.92 KB

Here is a new patch due to a regression on atom reference Drag&Drop + A cleanly way to detect native drag behavior

JonesUI’s picture

JonesUI CreditAttribution: JonesUI commented

Is the patch in #11 included with the latest dev release (7.x-1.1+67-dev)?

jcisio’s picture

jcisio CreditAttribution: jcisio commented

Ignore #14 because it is already posted at https://drupal.org/node/1995238#comment-8483081