Drupal plugin buttons disabled in WebKit based browsers

I've dug into this and found the problem.

It's the same one faced here: http://dev.fckeditor.net/ticket/1180.
FCKeditor determines which state to put a button in by checking document.queryCommandEnabled() which will return a number to indicate if a [builtin browser-]command can be run at the current state of the document. FCKeditor calls this for all commands, even those created by plugins. Since a plugin's command isn't native to the browser queryCommandEnabled() throws an exception. FCKeditor catches this exception and uses it to assume that the given command was from a plugin and should be set to "off" rather than "disabled". But some browsers seem to return -1 instead of throwing an exception, fooling FCKeditor to disable the button.

The FCKeditor devs have inserted an exception for the print button in Safari, but this seem to happen for all plugin buttons in Chrome.

We could simply check to see if the state variable we get back from FCKeditor's wrapper method returns -1 and change the state to FCK_TRISTATE_OFF (0) instead of FCK_TRISTATE_DISABLED (-1), but then we'd rule out the possibility of Drupal plugins disabling their buttons if say the selection isn't valid for what it does. (Note that we currently don not have this disabled-check in Drupal plugins.)

But, we could also regard this as an FCKeditor problem and hope they change their logic to work around that. I doubt they'll do that though since they're probably busy with CKeditor 3.0.

EDIT: Now that I think of it, this probably applies to native plugins which provide their own buttons as well, but I haven't confirmed that.

This is still happening... are we just waiting for FCKeditor to fix the issue? Cheers

Hello

I just opened an issue on the same problem before realizing it may be a duplicate.

Any workaround ?

Should we try to upgrade to CKEditor who is still in early beta ? Because reading the bug report TwoD mentionned it also happens in some versions of CKEditor.....

thanks a lot.

CKeditor is no longer in beta, it's up to 3.0.1 at this point. (Probably won't help much if the problem is still there though.)
The best workaround for this would probably be the method I mentioned above. We won't try to fix problems with the editors own buttons (or native plugins), but maybe we can do something about the Drupal plugins like Teaser Break. As Drupal plugins currently can't use the 'disabled' button state, we could simply swap out that and force the 'off' state instead. In the future, if Drupal plugins can use a 'disabled' state, we might be able to manually double check with the plugin itself. It requires some extra code to get the proper selection from the editor, send the selected element to the plugin and compare the results to determine if the button should really be disabled and not just turned off. It's all a theory at this point though, but if anyone feels they can contribute I'd be happy to review patches. I'm a bit swamped myself to take on this too now. :(

Thanks a lot for your quick reply.

Changing the code is probably way over my head for now but I'll check sometime soon with ckeditor 3.0.1 and I'll report here if they fixed it.

Good luck with your workload, I know the feeling :)

subscribe

I put together a simple test to see if queryCommandEnabled throws an exception as expected. If not, the button state returned from FCKeditor will be changed as per above. I have not been able to test it in Safari as I don't have it, so I need help with that.

This patch is mostly to see if a test like this is enough and a practical way to fix this. We might need the same thing for CKEditor, or other editors, in which case the test itself should be moved out of the FCKeditor implementation.

the patch #7 works for me with Safari and FCKeditor 2.6.5.

EDIT: And yes, this applies not only to Teaser button.

Thanks for testing! Do you know if this issue still exists in CKEditor?

We can't do anything about buttons from native plugins behaving this way since it would require changes in their code, but since Drupal plugins may provide functionality vital to integrating the editor nicely, I don't mind this quirk.

If this is needed by more editors, which seems likely, I think we should move the test to the Wysiwyg init function.

TwoD I can't guarantee it but from memory CKEditor didn't suffer from the same issue.

The patch also fixes Opera, not only WebKit.

Discussed the new comment:

document.queryCommandEnabled should throw an exception when invoked with a command that doesn't exist. In Safari, it just returns. What it returns is not important, just that it doesn't throw an exception. Safari and Chrome (Webkit based) reach this point even though they shouldn't.

We discussed this patch and figured that it's basically like $.supports, and ideally, we'd be using a similar design - perhaps even literally the same, keeping the code forward-compatible ($.supports only exists from jQuery 1.3 onwards, but that doesn't prevent us from stacking something onto $)

And here is the hopefully forward-compatible jQuery.support approach. Untested. But the logic "should" be the same.

+++ wysiwyg.init.js	14 Feb 2010 01:43:05 -0000
@@ -1,8 +1,22 @@
+  catch () {
+    $.support.queryCommandEnabled = true;

catch() must have a named argument or it counts as a syntax error.

Otherwise the patch works fine in IE (8), FF and Safari!

I think we might want to rename the variable a bit, as there are other quirks with queryCommandEnabled than it throwing or not throwing an exception.

Powered by Dreditor.

subscribing

subscribing

Didn't realize this was still set to "needs work".
Uploading a new patch with the small catch()-change just to get it out of the way.
It applies to the current 6.x -devs and we need help to verify this makes sense in as many of the major browsers as possible.

+++ wysiwyg.init.js	14 Feb 2010 01:43:05 -0000
@@ -1,8 +1,22 @@
+  // An exception should be thrown for nonexistent commands.

"non-existing"

+++ wysiwyg.init.js	14 Feb 2010 01:43:05 -0000
@@ -1,8 +1,22 @@
+    document.queryCommandEnabled('__wysiwygTestCommand');
+    $.support.queryCommandEnabled = false;

Sorry, I'm actually not familiar with JS exceptions -- is that second line actually parsed + executed in case of an exception?

+++ wysiwyg.init.js	14 Feb 2010 01:43:05 -0000
@@ -1,8 +1,22 @@
+  catch (er) {

s/er/e/

Powered by Dreditor.

No, that second line will not be executed when an exception is thrown by the line above it. Add a comment about that?

So the button isn't greyed out anymore! Yay!....but when I go to click it, it does nothing....

It works in Safari, but not in Chrome

any boby tell me how to apply this patch with windows os. i am using unxutills.but could,nt get success to implement the patch file.its showing me "cant find file to patch at line 8. perhaps you use the wrong -p or --strip option?".

plz plzz plzz giv a solution for this.

thanks in advance..

http://drupal.org/node/60179
The patch was created from the wysiwyg directory, you should not be needing a -p value if you apply it from there. I often just start with -p0 and try -p1 if a file can't be found tho, then look inside it to see what the actual paths are. I think that if it had looked like "wysiwyg/wysiwyg.init.js" and "wysiwyg/editors/js/fckeditor-2.6.js.", and "wysiwyg" was your current directory, you'd have to use -p1 to strip the first folder from the paths.

thanks for your reply ...!!
i applied a patch which is mention on #18. but not over come from this problem yet.button is not working in chrome or opera yet.
can any body provide a new patch which includes all these implementation with patch #18.

thanks in advance..waiting for a kind response from you.

Made a few test just now (Wysiwyg 6.x-2.x-dev + patch in #18):
Chrome 4 [virtual] WinXP. - OK
Chrome 6.0.472.53 on [virtual] WinXP. - OK
Chromium 5 on Ubuntu 10.04. - OK
Chromium 6.0.472.53 (57914) on Ubuntu 10.04. - OK
Opera 10.61 (6430) on Ubuntu 10.04. - OK
Safari via Wine on Ubuntu 10.04 - OK.

In no tests were the Teaser Break and Image Assist buttons grayed out and it was always possible to insert the <!--break--> comment using the TB button. (I never tested clicking the Image Assist button as only TinyMCE supports "native" dialog handling for cross-editor plugins - which IA relies on atm.) Some browsers (Chrome on WinXP was one) did have problems when clicking the break placeholder image, a text selection was created instead of the expected object selection if the break was not already there when the editor was loaded.

Thanks for reporting, reviewing, and testing! Committed to all branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

This patch does fix the greying-out of the Image Browser module's image-add button in FCKeditor, but while it makes the button appear live in Chrome, it doesn't fix its functionality - nothing happens when clicked. (It's possible this is an Image Browser or FCKeditor issue instead, but so far this is my best lead.)

Mind linking to the exact image browsing module you're using? If it uses Wysiwyg's functions for opening a dialog, like Image Assist does, it'll only work in TinyMCE. We were unable to implement this part of the API in a practical way for the other editors, and it'll face a major rewrite for Wysiwyg 3.x (if we keep it at all).

I still have this issue with the latest dev build and with the release version.

I'm using the Image Browser module which adds a button to the editor. The button works for IE8 and Chrome however it's grayed out for Firefox. I tested the firefox user with admin and normal user and get the same results. Thanks for any ideas on this, I'm at a loss on how to fix it as my javascript debugging is rusty, thanks again.