New lines inserted after header containers are broken in Chrome/Safari #360

Merged
merged 31 commits into from Jul 30, 2013

Conversation

Projects
None yet
5 participants
Owner

winhamwr commented Feb 14, 2013

Steps to reproduce:

  1. Visit the demo page in Chrome 19 or Safari 5.1.7 - http://files.wymeditor.org/wymeditor/examples/01-basic.html
  2. Change the 'Hello World' paragraph container to any header container
  3. Place the cursor at the end of the 'Hello World' text and hit enter

Expected result: a new paragraph container is created below the header container. This occurs in Firefox.

Actual result: a plain div is created below the header container. This div container cannot be changed to any other type of container. The only way to fix this is to switch to HTML view and change the div to another standard container (ex. p or h1).

Owner

winhamwr commented Jun 26, 2012

Hi Dana,

Well shoot. I expect this is related to the fix for #352. This is when Selenium-style tests sure would be nice since it's pretty rough to try and simulate keystroke behavior in javascript.

It looks like we're going to need to go to a configuration option to allow folks to use div tags as their default block level elements.

Thanks so much for the bug report. We'll have to tackle this one for a beta 4.

-Wes

winhamwr added a commit that referenced this pull request Jul 12, 2012

winhamwr added a commit that referenced this pull request Jul 12, 2012

Refs #360. Checkpoint towards a nicely package-ified world.
Used the common.js package specification and the jquery plugins
manifest spec to create starting points for those files. They're
useful with grunt.js

Is there a good workaround for this until a formal fix is provided? This is the ONLY "show-stopper" that's keeping me from deploying the editor, considering who my target audience is. ::cringes at mental image of emails from scared writers::

Owner

winhamwr commented Sep 20, 2012

Using the 1.0.0b1 release will fix this bug. It was a regression in the 1.0.0b2 release. You'll lose the bug fixes listed in the b2 changelog, but my guess is those aren't particularly useful fixes for your use cases. You probably don't have a lot of table/image insertion.

In my case you're correct. There will be image insertion, but really only via hyperlink.

I'll keep pushing forward and testing.

Thanks for the quick response!

SIDE NOTE: This is also the EASIEST editor to theme that I've come across. Whatever you do, PLEASE keep it this simple. :)

Owner

winhamwr commented Sep 20, 2012

SIDE NOTE: This is also the EASIEST editor to theme that I've come across. Whatever you do, PLEASE keep it this simple.

That's great to hear. The documentation around creating new themes could definitely be improved, and there are definitely ways we could improve performance, but I also love that just copying a folder and going from there keeps the barrier to creating new skins very low.

Um... could you please tell me how to download Version 1.0.0b1? I'm such a noob I can't find it. Thanks!

Owner

winhamwr commented Sep 20, 2012

All of the packaged versions of WYMeditor are available from the Downloads Page. You can also build packaged versions of WYMeditor yourself using git by following these instructions.

Owner

winhamwr commented Sep 21, 2012

If anyone needs a fix for this issue and can't jump back to WYM 1.0.0b1, you should try @vjt's fix in #369.

This may sound crazy... but even though I am using 1.0.0b1, this patch magically fixed the issue I was having with posting after a heading paragraph block that I posted about elsewhere. So long as Paste from Word is used properly, problems appear to be gone.

QUESTION What is the best way to get the average user to actually use the Word Paste feature? Most folks just want to copy and paste right in the window, and we know how tragic that would be. I just want something that says, "HEY, YOU... YEAH YOU! DO NOT PASTE YOUR WORD CRUFT HERE!!! USE THE BRIGHT SHINY WORD PASTE!" (of course, without yelling, but damn if cross-browser crap isn't frustrating enough, now you gotta add this turd to the punch bowl!!!!). I know getting all browsers to behave is a toss-up at best sometimes, but is there a reliable, cross-browser method to prevent direct pasting into the editor window? If so, I will gladly use this as a temporary fix until a more permanent solution comes along.

PS: I did extensive testing with the old catchpaste and hotkey plugins, but unfortunately they did not seem to play well with Firefox. :(

Owner

winhamwr commented Sep 24, 2012

this patch magically fixed the issue I was having with posting after a heading paragraph block

Awesome! That's very encouraging.

What is the best way to get the average user to actually use the Word Paste feature?

I wish I had a good solution for this (because I would be using it myself). The only solution I'm aware of that works outside IE is the way that tiny mce does it by using a magic, invisible div to catch the contents of the paste and then processing them. Implementing that isn't super difficult, but it does require some real javascript to make happen.

If nobody beats me to it (I hope they do), I'll definitely build that at some point. If you know any good javascript developers who are looking for consulting work, let me know. I've got some budget to pay someone to work on this and related issues.

-Wes

I wish I knew some good developers! I'd be using them myself actually.

This paste issue is critical to my project, as the pagination plugin uses paragraphs to determine when to page-break. The only OTHER alternative is to beef-up the editor's native capabilities so that if someone pastes directly in the window (and "if" in this case means "oh hell yes they're gonna do it"), at the very least everything is wrapped in p-tags and cruft is sifted out.

That's the ONLY thing stopping me from deploying the editor in my component - I have worked with writers before, and I KNOW of what they are capable. :(

Okay, slight change in strategy:

I'm still wrapping my brain around how to patch in to the onpaste event. Firefox is a bit touchy about iframes and pasting it seems, which is why I had to disable the catchpaste plugin.

If I could just write a simple event like the following pseudo-code, I can just add in my own paste clean routine:

$(editor).onpaste(function()
     // add word crap cleaning code here
 )
Owner

winhamwr commented Sep 26, 2012

I may be remembering this completely wrong (and I'd love correction if that's true), but IE is the only browser that lets you access the contents of a paste directly with a handler. For Chrome and FF, they don't because it's a security concern. Because of that, it's not as easy as just writing an onpaste handler (except for IE).

Owner

winhamwr commented Sep 26, 2012

Also, this stackoverflow thread has a good, detailed discussion of the techniques required to catch the pasted content: http://stackoverflow.com/questions/2176861/javascript-get-clipboard-data-on-paste-event-cross-browser

Edit: Specifically, check out the second answer and then Tim Down's response. Dude is a cross-browser selection genius. He has a jsfiddle with his solution here: http://jsfiddle.net/weswinham/2QufM/

I left a comment on the spot where you'll want to do the work to clean up the HTML before inserting the document fragment.

This is EXACTLY what I've been trying to find! I apologize for not being clearer in my earlier statements. I'm well aware of the safety limitations of paste access implemented by the various browsers (and for damn good reason). What I should have said was that, after pasting (which means it is now content within the editor), I wanted to then clean up the mess. I keep forgetting that especially with client-side scripting you HAVE to be very clear about what you're saying. :)

I see your notes - now that I see HOW to capture the pasted information, I'll start implementing my Word crap cleaning code. I already developed it for jwysiwyg so it'll be interesting to see how that works here - and yes, I also use timeout as part of the solution. :)

How do you recommend this be implemented? Should it be incorporated into the core? Would this function better as a plugin? Admittedly, I'm still learning editor syntax, but hey, I'll give it a shot. I think this REALLY would be helpful to so many, since most people aren't going to be good boys and girls and use the bright shiny paste button.

Thank you again for all the help!

Owner

winhamwr commented Sep 27, 2012

This is EXACTLY what I've been trying to find!

Awesome! Glad I could help. This is the strategy I've been trying to explain via text and the references to tinymce/fsckeditor, but this code is definitely a much less complex/ambiguous method of communication.

How do you recommend this be implemented? Should it be incorporated into the core?

I think it should definitely be in core and I'd love a pull request on it. It would be especially great if the method that did the actual HTML cleanup was separate from the code that did the capture, which would make it easy for folks to write plugins that changed that behavior (since my guess is that it would vary a little bit here and there). I am super-duper interested in this functionality, so if you were able to at least get started with some code and some tests in a pull request, I'll definitely be able to justify spending hours collaborating on it.

since most people aren't going to be good boys and girls and use the bright shiny paste button.

Definitely. It's basically perverse from a user experience perspective for us to ask them to. Removing that button will make me very happy :)

Hey, I'm no client-side scripting guru, but since I need it I'll try. :)

Owner

winhamwr commented Sep 27, 2012

no client-side scripting guru

I think there are like 5 of those in the world. Kind of like unicorns :)

Owner

winhamwr commented Sep 27, 2012

@Bratzilla I just got a comment notification email with a note from you about editing the jsfiddle, but it doesn't seem to be here. Github error, or did you delete it?

My bad... I submitted the comment too soon.

I'm still learning my way around Rangy, and I'm trying to figure out how to parse the pastedHTML. Ideally, all I want to grab is the content of the editable container (the big "glop" of text so to speak) and run that through my sanitizer.

Owner

winhamwr commented Sep 27, 2012

No problem. One thing that you'll probably find useful from WYMeditor is the wym.restoreSelectionAfterManipulation helper function. Example usage here.

If you write your paste catching + cleanup code as a callable, you'll be able to use that to reliably grab the user's current selection and then restore it after you do your paste stuff.

As far as detecting whether your inserted HTML should be wrapped in P tags itself, check out the current paste function. It does the same kind of logic you'll need to do for determining if it's a multi-line past, which calls _handleMultilineBlockContainerPaste. Basically, you'll look at your DocumentFragment, and if there's more than one block-level element, you'll split the current container in two and insert them all after the first split container. If it's only one block-level element, it probably means you want to strip the surrounding P tags from your DocumentFragment and just insert the stuff in the middle inline without splitting the current container.

Have you been able to get rolling with this stuff inside WYMeditor? Since there are so many different cases to handle, this is something that would really benefit (for your sanity) on some unit tests. If you ever want to drop by freenode IRC in the #wymeditor channel, I can probably help you get started with that.

Sorry about the delay... real life has sidelined me for a bit.

I've done a LOT more digging into this... even found some nice charts and articles explaining about cross-browser paste compatibility. Opera and Safari really throw a monkey wrench in, as some versions may have paste detection and "faking" capabilities on Windows, but not the Mac...

It seems like there are a LOT of variables to making direct paste clean-up work cross-browser, because not every browser can even properly DETECT the paste event.

I found this amazing fiddle: someone else seems to be going through a LOT of pain to make cross-browser paste work predictably:

http://jsfiddle.net/JgU37/44/

I am THIS close to just pushing the whole bloody thing off to the server and let PHP sort it all out. At least that seems to be what some suggest. :P

ANOTHER TACT....

Just thought of something...

WYMEditor has a "write-back" function to update the textarea with what's currently in the iframe.

Is there a way to simply take THAT text and clean it up? Theoretically, you could completely bypass the complexities of cross-browser paste events.

frob commented Nov 2, 2012

Am I wrong or would this problem would go away it we could use the container selector to go from a 'div' to a 'p'.

I have been using this editor for a while and only recently came acros this error. This is my favorite editor. I might work on a solution later, but I am swamped at the moment. I haven't done any development work on wym before so this would be the first. In fact this is the first time I have ventured up to the 1.x branch. I'm a Drupal dev and have been using .5 with the WYSIWYG Drupal Module.

I decided to try and get this editor working with Drupal again. I've got it working with its current code base (and will be submitting a branch to the Drupal people soon).

All that to say "Hello!"

Owner

winhamwr commented Nov 2, 2012

Hi Frob,

Am I wrong or would this problem would go away it we could use the container selector to go from a 'div' to a 'p'.

That would certainly mitigate the problem, but it would be a real pain to use. I hope to get a real solution Soon (tm). It will you to configure whether you'd prefer to use div or p tags as your primary container.

There are also several other bug fixes laying around in pull requests and copy/paste jobs on issues here that should make for a solid 1.0.0b4

I've got it working with its current code base (and will be submitting a branch to the Drupal people soon).

That's wonderful! Thanks so much for putting in the work to get it updated.

-Wes

winhamwr added some commits Jul 11, 2012

Refs #360. Checkpoint towards a nicely package-ified world.
Used the common.js package specification and the jquery plugins
manifest spec to create starting points for those files. They're
useful with grunt.js
Refs #360. Stubbed out setTopLevelContainer and added a failing test.
Folks will be able to either set the topLevelContainer option on
editor init or they'll be able to call `setTopLevelContainer` and
pass in either a 'p' or 'div'.
Refs #360. Specced out a more-sane way of handling the rules for elem…
…ent nesting.

This is necessary to be able to change what the default root element
is on the fly, since p's and div's will need to be traded all over the
place.
Owner

winhamwr commented Feb 14, 2013

Remaining TODO:

  • Convert remaining usage of MAIN_CONTAINERS and POTENTIAL_LIST_ELEMENTS to use the DocumentStructureManager
  • Find a way of distinguishing between block elements like li/th/td that can't just have new block elements inserted after them (tables). These elements are special.
  • When switching the defaultRootNode, change the DocumentStructureManager.structureRules to properly add/remove div and p to effectively make the other one not allowed
  • Change the root node conversion code to actually use the DocumentStructureManager rules consistently. Some of the update handlers use hard-coded WYMeditor.P and such

winhamwr added a commit that referenced this pull request Feb 15, 2013

Refs #360. Manually reverts #352. DIV-fixing-and-kind-of-breaking.
Issue #360 will have a fully-baked fix for this.
Contributor

FriedRice commented Jul 26, 2013

As part of this ticket, I fully implement the defaultRootContainer option in the DocumentStructureManager so that either p or div elements can be specified as the default root container. When one of these options is specified as the default root container, the other option will not be allowed to be used as a root container in the document. For example, if div is specified as the default root container, the editor will not allow the user to insert new p containers in the root of the document, and if there are existing p containers in the root of the document, they will be converted to div containers if the user types or moves within that container.

This is also enforced by changing the containers panel options so that only one of the possible default root container options is available (e.g. if div is the default root container, the "Paragraph" option will be removed from the containers panel, and a "Division" option will be added to the containers panel instead).

By strictly enforcing the default root container, the issue specified in the original issue description has also been fixed because the div created after the heading is now automatically converted to a p container immediately after it is created. Because this has been fixed, the selenium test for it now passes, and the unit tests for converting containers also pass.

In addition to fixing that issue, I also fixed a couple small issues in IE I noticed while implementing the default root container feature. I fixed an issue where text directly inserted into the body of the editor would not be wrapped in a container in IE like it normally is in other browsers, and I fixed an issue where elements that are not allowed to be directly in the body of the editor (such as strong, em, a, etc.) where not being wrapped in a container in IE like they normally are in other browsers.

As another part of this ticket, I had to make div elements visible in the editor by applying the CSS used to display the other visible containers in the editor to div as well. I also had to create a label image for div elements so that the div containers are labelled like the other containers in the editor.

Finally, I removed most of the build process things added in this ticket because some of the had already been added in #336, and the other ones should probably be added in a separate tickets specifically for improving the build process. I also updated the README to merge together the changes made in this ticket with the various changes that have been made to it since this ticket was last worked on.

Contributor

FriedRice commented Jul 26, 2013

One thing I forgot to mention: There's a lot of code repetition between the browser extension classes of the editor for this ticket, but I decided to save trying to consolidate all of the different browser extensions into the editor base for another ticket since this ticket is already quite long.

+// This test doesn't pass in older versions of IE because they add an extra
+// space onto the end of an `li` element's text content. The functionality
+// tested still works in those older IE versions.
+if (!jQuery.browser.msie || !SKIP_KNOWN_FAILING_TESTS) {
@FriedRice

FriedRice Jul 29, 2013

Contributor

This test was failing in IE 7 and 8, and I suspect it might also fail in newer versions of IE. The only reason the test fails in those browsers is because they add a space between text and a table within an li element despite the actual outdenting that is tested functioning as expected. I looked into fixing the test in those browsers for a little bit, but I couldn't find an easy solution, so I don't think it's worth spending any more time on fixing at the moment.

@@ -233,7 +231,8 @@ jQuery.extend(WYMeditor, {
// Containers that we allow at the root of the document (as a direct child
// of the body tag)
- MAIN_CONTAINERS : ["p", "h1", "h2", "h3", "h4", "h5", "h6", "pre", "blockquote"],
+ MAIN_CONTAINERS : ["p", "div", "h1", "h2", "h3", "h4", "h5", "h6", "pre",
@FriedRice

FriedRice Jul 29, 2013

Contributor

Although constants like MAIN_CONTAINERS and several of the following structure constants should eventually be moved into the DocumentStructureManager, I decided to leave these constants in core.js for now instead of spending the time to switch all of places these constants are used in the project over to using the DocumentStructureManager. I think converting all of these constants over to the DocumentStructureManager should probably be its own ticket.

@winhamwr

winhamwr Jul 29, 2013

Owner

Seems reasonable.

@@ -289,21 +289,21 @@ function simulateKey(keyCode, targetElement, options) {
keydown = jQuery.Event('keydown');
- keydown.keyCode = keyCode;
+ keydown.which = keyCode;
@FriedRice

FriedRice Jul 29, 2013

Contributor

I noticed that we don't consistently use one of either keyCode or which to get the key input of key events across the project, and it caused me some issues with writing tests that trigger key events, so I decided to go through the project and switch all the uses of keyCode to which to keep things consistent. I chose which over keyCode because which is the recommended option by jQuery to watch keyboard key input because the property normalizes the keyCode and charCode event properties.

@winhamwr

winhamwr Jul 29, 2013

Owner

Makes sense. Can you think of a good place to document that in the docs? Seems like something we want to write down so that we don't forget it. Maybe the coding standard?

@FriedRice

FriedRice Jul 30, 2013

Contributor

I added a section in the "Coding Standard" page in the docs about it.

Contributor

FriedRice commented Jul 29, 2013

Issue #369 was fixed by the changes I made in this ticket. I tried my hand at writing a selenium test to demonstrate the behavior in #369 to show that it is now fixed (I've never written selenium tests before, so I just modelled the test after the other selenium tests). It looks like the test does properly replicate the behavior in #369, and the test passes, thus showing that the issue is fixed.

Contributor

FriedRice commented Jul 29, 2013

I can't confirm it, but I think my changes in this ticket also probably fix the third of the three problems described in #367 because the div containers should be converted to p containers now.

Makefile
+selenium: selenium-chrome
+
+unittest:
+ @@build/phantomjs_test.sh localhost:8000/test/unit
@winhamwr

winhamwr Jul 29, 2013

Owner

We should probably remove this, since we can do this via grunt, now. No reason to have two ways of doing the same thing.

@FriedRice

FriedRice Jul 30, 2013

Contributor

Fixed

src/wymeditor/editor/firefox.js
+ // If we potentially created a new block level element or moved to a new
+ // one, then we should ensure the container is valid and the formatting is
+ // proper.
+ if (evt.which === WYMeditor.KEY.UP ||
@winhamwr

winhamwr Jul 29, 2013

Owner

It seems like we should probably abstract determining if an event could have created a new block level element out to a method in the editor base, to reduce code duplication here. It would also be easier to read.

keyCanCreateBlockElement maybe?

@winhamwr

winhamwr Jul 29, 2013

Owner

I agree with you about not reducing all of the duplication across browsers with this ticket, but we should probably look for areas like this where we can incrementally deduplicate things, especially where it also improves readability.

- evt.keyCode !== WYMeditor.KEY.ENTER &&
+ // If the inputted key cannont create a block element and is not a command,
+ // check to make sure the selection is properly wrapped in a container
+ if (!wym.keyCanCreateBlockElement(evt.which) &&
@FriedRice

FriedRice Jul 30, 2013

Contributor

I eliminated some of the code duplication in the keyup handlers in the browser extension classes by adding the methods keyCanCreateBlockElement and isForbiddenMainContainer to the editor base to determine if a key code can create a block element and if a tag is a forbidden main container respectively. I think using these methods also makes the keyup handlers a lot more readable.

I also added constants to the core for both of those methods to utilize (POTENTIAL_BLOCK_ELEMENT_CREATION_KEYS and FORBIDDEN_MAIN_CONTAINERS respectively) to make it easy to modify in future if needed which keys are considered able to create block elements and which tag names are forbidden main containers.

@winhamwr

winhamwr Jul 30, 2013

Owner

This is much more readable. Great change!

@@ -7,6 +7,7 @@ module.exports = function(grunt) {
qunit: {
all: {
options: {
+ timeout: 15000,
@FriedRice

FriedRice Jul 30, 2013

Contributor

The Travis build was failing on a time out intermittently every once in a while despite all of the same test runs passing on my local machine, so I figured the time out was happening because Travis was running the tests too slowly sometimes, so I changed the time out from the default of 5 seconds to 15 seconds when the tests are run with Grunt to try to avoid the tests timing out on Travis.

winhamwr added a commit that referenced this pull request Jul 30, 2013

Merge pull request #360 from wymeditor/issue_360
Fixes #360. New lines inserted after header containers are broken in Chrome/Safari

@winhamwr winhamwr merged commit 56ba260 into master Jul 30, 2013

1 check passed

default The Travis CI build passed
Details

@winhamwr winhamwr deleted the issue_360 branch Jul 30, 2013

lehni added a commit to wemakeit/wymeditor that referenced this pull request May 15, 2014

Refs #360. Manually reverts #352. DIV-fixing-and-kind-of-breaking.
Issue #360 will have a fully-baked fix for this.

@mightyiam mightyiam modified the milestones: 1.0.0, 1.0.x Sep 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment