WYSIWYG-enabled text formats break WYSIWYG for any elements underneath a tablefield.

The patch doesnt seem to be working.Still unable to style table using wysiwyg after applying patch.Has anyone else tested it?.I suggest a dev release be made available.

@doncheks, I don't believe this issue is about adding WYSIWYG functionality to tablefield. Instead it is about disabling the client-side wysiwyg logic that kicks-in for tablefields using wysiwyg-enabled text formats, which are not compatible with tablefield. The problem is that any tablefields that want to make user of any filter rules within a WYSIWYG-enabled text format may trigger issues for other fields on the same form.

Anyway, this patch appears to work fine in terms of addressing the issue outlined. From what I can see it follows the generally accepted convention for disabling WYSIWYG on a field-by-field basis (e.g. set the #wysiwyg flag on the form element to FALSE). I'll test some more but consider this a confirming review.

After some additional testing it seems clear that this is a good fix. It's implemented in an innocuous way (setting a flag that only the wysiwyg module reacts to) so I don't see much risk in causing problems for other parts of the module. On this ground I think it's safe to mark RTBC.

I am however a bit worried that tablefield isn't really being maintained anymore. Hopefully this is not the case though.

Hey all, so sorry for the tardy response. I'm trying to get caught up with the issue queue.

I'm not sure that I feel comfortable making this module aware of other modules. Is there any other way that you can solve this without coding against the wysiwyg module?

Thx!

Thanks for checking this out Kevin.

Is there any other way that you can solve this without coding against the wysiwyg module?

I'm not personally aware of one. It seems that as long as you offer a text format option, and that format is wysiwyg-enabled, the wysiwyg logic could be triggered even if the input is not compatible with it. I've encountered other cases where I've needed to disable wysisyg on a field-by-field basis, and everything I read indicated that the #wysiwyg property was the tool for this.

The comments in the wysiwyg module itself seem to indicate this as well. In wysiwyg_pre_render_text_format():

  // Allow modules to programmatically enforce no client-side editor by setting
  // the #wysiwyg property to FALSE.
  if (isset($element['#wysiwyg']) && !$element['#wysiwyg']) {
    return $element;
  }

It looks like this programatic control was added in #524126: Ability to hide WYSIWYG on certain fields.

This is also the logic used in this interesting sandbox project by Dave Reid: https://drupal.org/sandbox/davereid/1973302

The info was the basis of my review in #3 and #4. Anyway, perhaps the patch author also has some comments or other ideas.

@Kevin Hankens, it's been a couple of weeks so I wanted to check-back and see if you had any thoughts based on my comments in #6.

Would you feel better with a fix like this if we looked at it through the lens of a "feature request" (to play nice with wysiwyg) as opposed to a bug report? Perhaps that's the preferred course of action given that we are talking bout compatibility with another contrib module.

Please do advise so we can hopefully move this forward.

Hmm. I guess the patch in #1 would degrade gracefully. I'm loathe to make tablefield aware of non-core modules, but perhaps this is the only option. I'll see if I can get it in this week and maybe roll a new release asap!

Thanks Kevin. I understand your point of view, and it's too bad that wysiwyg is not more configurable against these kings of conflicts. Anyway, it will be great to see these two modules gracefully exist side-by-side, especially given that wysiwyg is fairly ubiquitous and tablefield is quite popular.

I just wanted to check-back on this as it's be RTBC for several months now. Any chance this will make it in?

I think this can be committed, it does not really tie in the wysiwyg module, I think it does not hurt inserting the #wysiwyg property even without the module_exists check. I'll give Kevin a few more days chance to comment and then likely commit this.