Implement class flag that can be used to have an element removed by the parser #414

NickMcL · 2013-07-01T21:46:52Z

In order to allow plugins and other external functions to tell the parser to remove certain elements from the editor's XHTML output, a class flag should be implemented that can be simply added as a class of an element to tell the parser to remove it.

…o be removed by the parser.

…lements.

NickMcL · 2013-07-01T21:59:19Z

I added a class flag WYMeditor.EDITOR_ONLY_CLASS that can be added as a class to any element to have that element removed when the editor's HTML is parsed by the XHTML parser. This allows for the creation of "editor-only" elements if they have this class in the sense that the elements will be visible within the editor, but they will then be removed from the XHTML output of the editor.

In addition, I added a large number of tests for this "editor-only" functionality to ensure it works on container elements, inline elements, self-closing elements, tables, lists, elements with multiple classes, and nested "editor-only" elements.

I updated the CHANGELOG with this new feature, but I wasn't sure where I should document the feature in the docs. Let me know if you have a suggestion on where I should include documentation on the feature.

NickMcL · 2013-07-02T13:45:20Z

src/test/unit/test.js

+var TEXT_CONTAINER_ELEMENTS = ['p', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6',
+                               'pre', 'blockquote', 'div'];
+var TEXT_INLINE_ELEMENTS = ['a', 'span', 'strong', 'em'];
+var SELF_CLOSING_ELEMENTS = ['br', 'hr', 'img'];


There weren't any existing constants in WYMeditor for these groupings of elements, so I added them here for now. Let me know if you think it would be worthwhile to include them as constants in WYMeditor.

We sort of have this defined in parser/xhtml-sax-listener.js as inline_tags, but it's not a global. I'll have to think on this a bit. These kind of things are perfect candidates for he StructureManager that #360 defines

…a comment.

NickMcL · 2013-07-02T14:03:11Z

src/test/unit/utils.js

 */
-function htmlEquals(wymeditor, expected) {
+function htmlEquals(wymeditor, expected, assertionString) {


I found it useful to be able to include a string message to specify what each assertion in a test was checking, so I added a parameter for including this string message to htmlEquals.

Good call. That's definitely helpful for providing good test output.

winhamwr · 2013-07-02T18:37:48Z

src/wymeditor/parser/xhtml-parser.js

@@ -15,6 +15,12 @@ WYMeditor.XhtmlParser = function(Listener, mode) {
    this._last_match = '';
    this._current_match = '';

+    // These are used for removing blocks flagged for removal by the parser


All of the work that you're doing here actually belongs in the xhtml-sax-listener. The lexer is responsible for taking in strings of characters and grouping them in to logical HTML tokens, the parser is responsible for classifying these tokens according to the XHTML grammar and the xhtml-sax-listener is responsible for actually doing interesting things based on events managed by the parser. Deleting things with a certain class, ignoring white space, stripping certain tags, fixing nesting etc. are all things for the sax listener.

All of the state that's not specifically necessary for interpreting grammar should also be in the listener.

Currently, we have some things in the parser that should be there, especially where they're directly managing the listener state (for example the fixNestingBeforeOpeningBlockTag call), but we want to move away from that as much as possible. The xhtml-parser should actually be much simpler than it is.

I would do this by:

Adding a WYMeditor.XhtmlSaxListener._shouldRemoveTag method that looks at the class attributes to determine if it has the EDITOR_ONLY_CLASS

In WYMeditor.XhtmlSaxListener.prototype.inlineTag, check if _shouldRemoveTag and if so, don't add to this.output

In WYMeditor.XhtmlSaxListener.prototype.openBlockTag, if you _shouldRemoveTag, then set a flag on the listener. this._insideTagToRemove to true and use another variable to keep track of where you currently are on the tag stack. Make sure and only do this check if you're not already _insideTagToRemove.

Then, in all of the other listeners that add to this.output (eg. addContent, openBlockTag, inlineTag, closeBlockTag, closeUnopenedTag, etc), check the _insideTagToRemove flag and if it's true, don't actually add to the output.

In WYMeditor.XhtmlSaxListener.prototype.closeBlockTag, if the flag is set, but we're back down to the same place on the tag stack that triggered the flag, flip the flag back

Also, fixNestingBeforeOpeningBlockTag is not well-implemented and might cause you some problems. You'll probably want to throw in some tests that mix that functionality with the tag removal here.

…listener rather than the parser.

…ists.

… Chrome and Firefox.

… all browsers.

NickMcL · 2013-07-03T17:02:36Z

I reimplemented the removal of editor-only elements so that all of the work takes place in xhtml-sax-listener rather than xhtml-parser. This change worked well since the removal of editor-only elements is now more streamlined and efficient.

I also modified the fixNestingBeforeOpeningBlockTag function to account for editor-only elements in all of its cases, and now, editor-only elements are removed as expected in all the invalid list cases fixed by the function. In addition, I added several tests for a variety of combinations of invalid lists and editor-only elements, and I confirmed that these tests are all passing in Firefox 21, Chrome 27, and IE7-10.

As for possibly reimplementing fixNestingBeforeOpeningBlockTag, I chose not to reimplement it in the parser rather than the listener because after looking further into it, it's much easier to have the invalid list fixes applied by the listener itself rather than having the parser try to modify the output that has already gone through the listener to make the fixes.

winhamwr · 2013-07-03T17:33:45Z

src/wymeditor/editor/webkit.js

@@ -240,7 +240,15 @@ WYMeditor.WymClassSafari.prototype.openBlockTag = function (tag, attributes) {
        }
    }

-    this.output += this.helper.tag(tag, attributes, true);


I didn't even realize this was a thing. I bet finding this was a PITA. I created #416 as a ticket to stop doing any lexer/parser/listener overrides. All this method does is add confusion.

winhamwr · 2013-07-03T17:50:33Z

I love the tests here. Also, good call on not moving the list nesting fix to the parser for now. This solution is actually pretty straight-forward.

…s for the added Listener flags.

NickMcL · 2013-07-03T18:55:03Z

I reformatted the conditionals you pointed out so that they don't use all negatives. Originally, I avoid using return; because I thought I read somewhere that using it is bad practice. I think the code does look more readable using it now though.

I also added comments for all of the variables I added to the WYMeditor.XhtmlSaxListener object, and I merged in the changes from #364 and switched over my uses of html() in this ticket to _html() accordingly.

winhamwr · 2013-07-03T19:15:37Z

As far as return, my sensibilities come from the zen of python:

Flat is better than nested.

I'm not actually sure if that's what most folks writing javascript prefer, but to me, it's much easier to understand. It means you have to keep fewer things in your head when reading through the code. If I'm indented a level, that's extra context that I need to keep in my brain when evaluating what's happening. If it's flat, then I only need to have verified that the code path I'm following didn't return, which is easy to do when reading top to bottom inside a well-structured method that's not huge/complex.

winhamwr · 2013-07-03T19:16:48Z

src/wymeditor/parser/xhtml-sax-listener.js

+    // to true.
+    this._insideTagToRemove = false;
+
+    // If the last tag was not added to the output, this flag is set to true.


Why do we need this flag? That's more what I would be looking for in this comment. "Why" comments are much better than "what" comments.

I added more description in the comment for this flag.

…ction so it still works in all use cases.

winhamwr · 2013-07-03T19:28:58Z

src/wymeditor/parser/xhtml-sax-listener.js

+    // result in an extra closing LI tag coming up later in the parser. When
+    // one of these situations occurs, this flag is set to true so that the
+    // parser knows to ignore the extra closing LI tag.
+    this._removeExtraLI = false;


Looking at the way this is actually used, it seems like any time this flag is true, the next time you encounter a closing block tag that hasn't already been opened, you ignore it. I'd say this variable should either be called _ignoreNextExtraBlockClose or some such or the place that uses it should ensure its dealing with an li tag.

Also, is there any chance you could get in a situation where you would hit two li tags that you would need to remove? If that's possible, then this flag method would be a problem, since the first one would clear the flag. An alternative way to do that would be a _unopenedBlockClosesToIgnore list to which you can add tag types.

The place that uses this flag does ensure its dealing with an li tag now, so your first point shouldn't be a problem.

You are correct that it is possible that there would be two or more closing li tags that need removal (although that would require a pretty horribly broken list to achieve), so I switched this flag to a counter that can be incremented and decremented to keep track of how many extra closing li tags should be expected.

…le are used.

…nts that wasn't working and added a test for it.

jlward · 2013-07-08T15:57:00Z

src/wymeditor/parser/xhtml-sax-listener.js

-    if (this.remove_comments) {
-        this.output += text;
+    if (this.remove_comments || this._insideTagToRemove) {
+        return;


Is this correct? Before if this.remove_comments was True then the text would be appended to this.output. Now if this.remove_comments is True you return. I am not certain which it should be, just want to make sure it is correct now.

I assumed that the original this.remove_comments was a mistake and should have actually been !this.remove_comments. It doesn't make much sense that this.remove_comments being true would cause comments to be included in the output whereas this.remove_comments being false would cause comments to be removed from the output.

jlward · 2013-07-08T16:16:58Z

Looks good other than a few code construction issues. You may want to wait for @winhamwr to put his check mark though.

…tTagRemoved flag.

winhamwr · 2013-07-11T16:45:34Z

Looks great!

Closes #414. Implement class flag that can be used to have an element removed by the parser

NickMcL added 2 commits July 1, 2013 12:19

Refs wymeditor#414. Implemented a class flag for marking an element t…

cf016b2

…o be removed by the parser.

Refs wymeditor#414. Started adding tests.

209342b

NickMcL added a commit to NickMcL/wymeditor that referenced this pull request Jul 1, 2013

Refs wymeditor#414. The parser now removes editor-only self-closing e…

a2f80ea

…lements.

NickMcL added a commit to NickMcL/wymeditor that referenced this pull request Jul 1, 2013

Refs wymeditor#414. Updated the CHANGELOG.

727bb0e

NickMcL added 4 commits July 1, 2013 17:54

Refs wymeditor#414. Added more tests.

9fe294e

Refs wymeditor#414. The parser now removes editor-only self-closing e…

83c5540

…lements.

Refs wymeditor#414. Updated the CHANGELOG.

fd1290a

Merge branch 'master' into issue_414

636dedb

NickMcL reviewed Jul 2, 2013
View reviewed changes

NickMcL added 2 commits July 2, 2013 09:55

Refs wymeditor#414. Improved naming consistency and clarification of …

31433e4

…a comment.

Refs wymeditor#414. Added to comment for htmlEquals.

749a6be

NickMcL reviewed Jul 2, 2013
View reviewed changes

NickMcL mentioned this pull request Jul 2, 2013

Convert previously implemented editor-only elements to using the newly added editor-only class #415

Merged

winhamwr reviewed Jul 2, 2013
View reviewed changes

NickMcL added 4 commits July 2, 2013 16:01

Refs wymeditor#414. Reimplemented editor-only element removal in the …

b2fcc65

…listener rather than the parser.

Refs wymeditor#414. Added tests for editor-only elements in invalid l…

da3f2aa

…ists.

Refs wymeditor#414. Editor-only elements now work in invalid lists in…

682b13d

… Chrome and Firefox.

Refs wymeditor#414. Editor-only elements now work in invalid lists in…

0e9223a

… all browsers.

winhamwr reviewed Jul 3, 2013
View reviewed changes

NickMcL added 2 commits July 3, 2013 14:21

Merge branch 'master' into issue_414

b958f67

Refs wymeditor#414. Improved conditional formatting and added comment…

923f5b1

…s for the added Listener flags.

winhamwr reviewed Jul 3, 2013
View reviewed changes

NickMcL added a commit to NickMcL/wymeditor that referenced this pull request Jul 3, 2013

Refs wymeditor#415. Megre wymeditor#414 and fix deprecated html() fun…

f3609ba

…ction so it still works in all use cases.

winhamwr reviewed Jul 3, 2013
View reviewed changes

NickMcL added 3 commits July 3, 2013 16:21

Refs wymeditor#414. Improved comments and changed how a couple variab…

eadb4eb

…le are used.

Refs wymeditor#414. Fixed usage of a couple variables.

fb917e5

Refs wymeditor#414. Fixed an invalid list case with editor-only eleme…

9d9a0b7

…nts that wasn't working and added a test for it.

jlward reviewed Jul 8, 2013
View reviewed changes

NickMcL added 2 commits July 8, 2013 14:27

Refs wymeditor#414. Fixed some code construction issues.

b9fdf6d

Refs wymeditor#414. Fixed a minor placement issue of setting the _las…

0df6ec6

…tTagRemoved flag.

NickMcL mentioned this pull request Jul 8, 2013

Don't override parser functions for browser-specific behavior #416

Merged

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

Merge pull request #414 from FriedRice/issue_414

3a361a8

Closes #414. Implement class flag that can be used to have an element removed by the parser

winhamwr merged commit 3a361a8 into wymeditor:master Jul 11, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement class flag that can be used to have an element removed by the parser #414

Implement class flag that can be used to have an element removed by the parser #414

NickMcL commented Jul 1, 2013

NickMcL commented Jul 1, 2013

NickMcL Jul 2, 2013

winhamwr Jul 2, 2013

NickMcL Jul 2, 2013

winhamwr Jul 2, 2013

winhamwr Jul 2, 2013

winhamwr Jul 2, 2013

NickMcL commented Jul 3, 2013

winhamwr Jul 3, 2013

winhamwr commented Jul 3, 2013

NickMcL commented Jul 3, 2013

winhamwr commented Jul 3, 2013

winhamwr Jul 3, 2013

NickMcL Jul 3, 2013

winhamwr Jul 3, 2013

NickMcL Jul 3, 2013

jlward Jul 8, 2013

NickMcL Jul 8, 2013

jlward commented Jul 8, 2013

winhamwr commented Jul 11, 2013

Implement class flag that can be used to have an element removed by the parser #414

Implement class flag that can be used to have an element removed by the parser #414

Conversation

NickMcL commented Jul 1, 2013

NickMcL commented Jul 1, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickMcL commented Jul 3, 2013

Choose a reason for hiding this comment

winhamwr commented Jul 3, 2013

NickMcL commented Jul 3, 2013

winhamwr commented Jul 3, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlward commented Jul 8, 2013

winhamwr commented Jul 11, 2013