Skip to content
This repository has been archived by the owner on Jul 24, 2019. It is now read-only.

Toolbar Fixes #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Toolbar Fixes #68

wants to merge 2 commits into from

Conversation

jordandh
Copy link
Contributor

Fix for FF throwing NS_ERROR_UNEXPECTED in updateToolbar (issue #23)
Adds support for commands with arguments in the toolbar and also is a fix for issue #56 (comment)

My other pull request #66 fixed the FF error. This pull request includes that fix and a fix for the IE10 issue.

Added a check to see if a command is enabled before querying the command's state. This is a fix for the exception FF throws when querying a command state that is disabled.

I read through some of the spec for rich text editing and it states that a command can be enabled or disabled at any given time. When no text is selected on the page some of the commands become disabled. For example the insertunorderedlist command is disabled. document.queryCommandEnabled can be used to check for this condition. The spec does not specify what should happen if document.queryCommandState is called on a disabled command and that the browsers do different things from throwing exceptions to returning false or an empty string. With that in mind there are two ways to fix this. You can add a try/catch for FF (which throws an exception) but you might catch other types of errors so that isn't a good solution. I went with checking the enabled state of the command manually because it reduces the overhead of handling an exception and doesn't hide errors. It also matches the looseness of the spec in this area.

The other change that adds support for commands with arguments simply checks to see if the command has arguments and if so uses document.queryCommandValue to determine if the command is active.

@carlopires
Copy link

Is there something against this commits? I'm needing these fixes too.

tjdett added a commit to uq-eresearch/aorra that referenced this pull request Nov 6, 2013
Using bootstrap-wysiwyg.js from this pull request:
mindmup/bootstrap-wysiwyg#68
@lmancini
Copy link

@jordandh will this PR be merged at some point? I have encountered the very same error that your PR fixes, and I'm not sure why these commits have not been merged yet.

@jordandh
Copy link
Contributor Author

Its been a long time since this project was last updated and I'm not sure if it is still being maintained. I'm actually not using it anymore. I had to move over to tinymce. I would just add the changes yourself to your own files or your own fork of this repo.

@steveathon
Copy link

Just confirming - this fixes the FF error in my 28.0 version too. Sent this into our CDN pre-update, so hopefully it gets adopted.

@astagi
Copy link

astagi commented Apr 27, 2014

I think you better fork the project and apply the patch by yourself

@steveathon
Copy link

I'm doing that as we speak - I'm looking through those 13 outstanding ones and seeing what I can just merge in.

steveathon referenced this pull request in steveathon/bootstrap-wysiwyg Apr 27, 2014
Conflicts:
	bootstrap-wysiwyg.js

Fixed for #68 with update to @jordanh version.
steveathon referenced this pull request in steveathon/bootstrap-wysiwyg Apr 27, 2014
…ap-wysiwyg

This combines the #68 change where FF throws an error and #103 where it supports multiple instances thanks to @brutuscat

# By Mauro Asprea
# Via Mauro Asprea
* 'fix-multiple-instances' of github.com:brutuscat/bootstrap-wysiwyg:
  Fix to support multiple instances adding a wrapper context for toolbar's bindings

Conflicts:
	bootstrap-wysiwyg.js
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants