Allow special tags to be self-closing #1101

ang-zeyu · 2020-03-08T11:14:28Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Documentation update
• [x] Enhancement to an existing feature

Resolves #1099

What is the rationale for this request?
Allow injected special tags to be self-closing as well, while preserving the contrary for script/style tags ( according to html5 spec ).

What changes did you make? (Give an overview)

Simple 2 line patch to self-closing tag state handler to allow said behaviour

// self-closing tag state handler
...
this._special = SPECIAL_NONE;
...

Update tests to test for self-closing injected special tags as well
Small documentation update to reflect this information

Is there anything you'd like reviewers to focus on?
na

Testing instructions:

npm run test

Proposed commit message: (wrap lines at 72 characters)
Allow special tags to be self-closing

Unlike the html script and style special tags, plugin authors may want
injected special tags to be able to be self-closing as well.
This can help unify the syntax used in the plugin, which makes for
better author usability of the plugin.

Let’s allow injected special tags to be self-closing.
Let’s also do so for the script and style tags, which would be
expanded to their normal form after rendering.

crphang

Only a minor comment that would help document the change better. Looks generally okay to me with tests passing as well. Blast radius (if any) would be small since this only affects a new feature (special tags). I assumed that Tokenizer.prototype._stateInSelfClosingTag is a patched function.

crphang · 2020-03-08T11:24:24Z

src/lib/markbind/src/patches/htmlparser2.js

+		this._sectionStart = this._index + 1;
+		if (this._special > 2) {
+			/*
+			 We intentionally fail if the special tag in question is


Minor: Would really help to document/comment what "fail" imply when you set this._special = SPECIAL_NONE.

Updated

Will update back if there are changes besides rebasing on the latest!

openorclose

Since there's no such thing as self closing tags in the HTML specs, should we just allow script and style tags to be self closing too?

In the generated HTML they would all be expanded out to the valid <script></script> and <special></special> forms anyway.

ang-zeyu · 2020-03-10T09:52:36Z

Since there's no such thing as self closing tags in the HTML specs, should we just allow script and style tags to be self closing too?

In the generated HTML they would all be expanded out to the valid <script></script> and <special></special> forms anyway.

I'm open to either; What do you think? @crphang

crphang · 2020-03-10T16:07:26Z

Since there's no such thing as self closing tags in the HTML specs, should we just allow script and style tags to be self closing too?
In the generated HTML they would all be expanded out to the valid <script></script> and <special></special> forms anyway.

I'm open to either; What do you think? @crphang

Looks fine to me, not 100% sure if there will be no issues, but seems like it will make the code simpler here

ang-zeyu · 2020-03-11T07:04:10Z

Updated to allow script / style tags as well;
It should be side effect free as htmlparser2 dosen't do special processing for script/style tags outside of its tokenizer

crphang · 2020-03-21T09:19:54Z

@marvinchin could you help with the review of this? 🙇

marvinchin

Mostly LGTM :) Just some comments might help other developers understand what's happening better.

Also, please explicitly request a review from me when your code is ready so I know when to look at it (even for re-reviews!)

marvinchin · 2020-03-21T09:32:24Z

src/lib/markbind/src/patches/htmlparser2.js

@@ -261,6 +261,22 @@ Tokenizer.prototype._stateBeforeSpecial = function(c) {
 	this._special = result + 1;
 };

+/**
+ * Removes the special state if the special tag was self-closed.


Can we explicitly state that this is an altered version of the _stateInSelfClosingTag defined in htmlparser? I think it might help other developers new to this part of the codebase understand what's going on better.

marvinchin · 2020-03-21T09:33:32Z

src/lib/markbind/src/patches/htmlparser2.js

+		this._cbs.onselfclosingtag();
+		this._state = TEXT;
+		this._sectionStart = this._index + 1;
+		// Allow all special tags (including script & style tags) to be self-closed


Let's explain why we allow this despite the spec not allowing it. I think just stating the fact that the original htmlparser allows it is sufficient.

To clarify, did you mean the original htmlparser disallows it ( that is the case, we're breaking away from the parser's default behaviour here )?

Oops, I actually thought that the original htmlparser allowed it, since the changes we made didn't seem to affect any of the logic specific to script/style tags. After taking a closer look at the rest of the changes in our patches, it seems like you are right 🙂

ang-zeyu · 2020-03-21T09:54:05Z

(even for re-reviews!)

Will do 👍 Thanks for looking through this!

ang-zeyu · 2020-03-21T10:52:45Z

~~Resolved minor conflict with #1049~~

broken currently

marvinchin

LGTM :)

ang-zeyu · 2020-03-21T12:03:54Z

broken currently

Fixed, updated #1049's cheerio test site call with xmlMode: false.
Needed because in xmlMode <abc></abc> is recognised as an empty tag, hence it renders back to <abc />, which breaks once passed to the next plugin where xmlMode = false

Unlike the html script and style special tags, plugin authors may want injected special tags to be able to be self-closing as well. This can help unify the syntax used in the plugin, which makes for better author usability of the plugin. Let’s allow injected special tags to be self-closing. Let’s also do so for the script and style tags, which would be expanded to their normal form after rendering.

* 'master' of https://github.com/MarkBind/markbind: Update test files (MarkBind#1138) Remove OK button from modals (MarkBind#1134) Add start from line number functionality to code blocks (MarkBind#1115) Allow special tags to be self-closing (MarkBind#1101) Simplify baseUrl resolving process (MarkBind#1087) Remove redundant file writing (MarkBind#1090) Bump acorn from 5.7.3 to 5.7.4 (MarkBind#1121) Bump acorn from 7.1.0 to 7.1.1 in /src/lib/markbind (MarkBind#1120) Unify markdown-it parser variants (MarkBind#1056) Remove dynamic include feature (MarkBind#1037) Fix flex shrink not applying in content wrapper (MarkBind#1135) Escape Nunjucks for special tags (MarkBind#1049) Update documentation on icon slot for boxes (MarkBind#1123)

…x-pageNav * 'master' of https://github.com/MarkBind/markbind: Update test files (MarkBind#1138) Remove OK button from modals (MarkBind#1134) Add start from line number functionality to code blocks (MarkBind#1115) Allow special tags to be self-closing (MarkBind#1101) Simplify baseUrl resolving process (MarkBind#1087) Remove redundant file writing (MarkBind#1090) Bump acorn from 5.7.3 to 5.7.4 (MarkBind#1121) Bump acorn from 7.1.0 to 7.1.1 in /src/lib/markbind (MarkBind#1120) Unify markdown-it parser variants (MarkBind#1056) Remove dynamic include feature (MarkBind#1037) Fix flex shrink not applying in content wrapper (MarkBind#1135) Escape Nunjucks for special tags (MarkBind#1049) Update documentation on icon slot for boxes (MarkBind#1123) Convert code in boxes to code block (MarkBind#1086)

Unlike the html script and style special tags, plugin authors may want injected special tags to be able to be self-closing as well. This can help unify the syntax used in the plugin, which makes for better author usability of the plugin. Let’s allow injected special tags to be self-closing. Let’s also do so for the script and style tags, which would be expanded to their normal form after rendering.

ang-zeyu mentioned this pull request Mar 8, 2020

Special Tags have parsing issues when closed with <tag /> instead of <tag></tag> #1099

Closed

ang-zeyu force-pushed the allow-self-closing-injected-tags branch from 441d658 to c97b9e2 Compare March 8, 2020 11:23

crphang reviewed Mar 8, 2020

View reviewed changes

ang-zeyu force-pushed the allow-self-closing-injected-tags branch from c97b9e2 to 1d0925e Compare March 8, 2020 11:32

crphang approved these changes Mar 8, 2020

View reviewed changes

ang-zeyu force-pushed the allow-self-closing-injected-tags branch from 1d0925e to 4d44772 Compare March 9, 2020 10:30

openorclose requested changes Mar 10, 2020

View reviewed changes

ang-zeyu changed the title ~~Allow injected special tags to be self-closing~~ Allow special tags to be self-closing Mar 11, 2020

ang-zeyu force-pushed the allow-self-closing-injected-tags branch from 4d44772 to 2ff592b Compare March 11, 2020 07:02

openorclose approved these changes Mar 13, 2020

View reviewed changes

marvinchin reviewed Mar 21, 2020

View reviewed changes

ang-zeyu force-pushed the allow-self-closing-injected-tags branch from 2ff592b to 9e1fea6 Compare March 21, 2020 10:31

ang-zeyu requested a review from marvinchin March 21, 2020 10:31

ang-zeyu force-pushed the allow-self-closing-injected-tags branch from 9e1fea6 to 7c1aef1 Compare March 21, 2020 10:52

marvinchin approved these changes Mar 21, 2020

View reviewed changes

marvinchin self-requested a review March 21, 2020 11:02

ang-zeyu force-pushed the allow-self-closing-injected-tags branch 2 times, most recently from acad179 to 9359c19 Compare March 21, 2020 12:01

ang-zeyu force-pushed the allow-self-closing-injected-tags branch from 9359c19 to bb6284a Compare March 22, 2020 05:25

marvinchin added this to the v2.12.1 milestone Mar 23, 2020

marvinchin merged commit 7850366 into MarkBind:master Mar 23, 2020

ang-zeyu mentioned this pull request Dec 6, 2020

Ignore self-closing special tags for markdown-it #1408

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow special tags to be self-closing #1101

Allow special tags to be self-closing #1101

ang-zeyu commented Mar 8, 2020 •

edited

Loading

crphang left a comment

crphang Mar 8, 2020

ang-zeyu Mar 8, 2020 •

edited

Loading

openorclose left a comment

ang-zeyu commented Mar 10, 2020

crphang commented Mar 10, 2020

ang-zeyu commented Mar 11, 2020

crphang commented Mar 21, 2020

marvinchin left a comment

marvinchin Mar 21, 2020

marvinchin Mar 21, 2020

ang-zeyu Mar 21, 2020

marvinchin Mar 21, 2020

ang-zeyu commented Mar 21, 2020

ang-zeyu commented Mar 21, 2020 •

edited

Loading

marvinchin left a comment

ang-zeyu commented Mar 21, 2020

Allow special tags to be self-closing #1101

Allow special tags to be self-closing #1101

Conversation

ang-zeyu commented Mar 8, 2020 • edited Loading

crphang left a comment

Choose a reason for hiding this comment

crphang Mar 8, 2020

Choose a reason for hiding this comment

ang-zeyu Mar 8, 2020 • edited Loading

Choose a reason for hiding this comment

openorclose left a comment

Choose a reason for hiding this comment

ang-zeyu commented Mar 10, 2020

crphang commented Mar 10, 2020

ang-zeyu commented Mar 11, 2020

crphang commented Mar 21, 2020

marvinchin left a comment

Choose a reason for hiding this comment

marvinchin Mar 21, 2020

Choose a reason for hiding this comment

marvinchin Mar 21, 2020

Choose a reason for hiding this comment

ang-zeyu Mar 21, 2020

Choose a reason for hiding this comment

marvinchin Mar 21, 2020

Choose a reason for hiding this comment

ang-zeyu commented Mar 21, 2020

ang-zeyu commented Mar 21, 2020 • edited Loading

marvinchin left a comment

Choose a reason for hiding this comment

ang-zeyu commented Mar 21, 2020

ang-zeyu commented Mar 8, 2020 •

edited

Loading

ang-zeyu Mar 8, 2020 •

edited

Loading

ang-zeyu commented Mar 21, 2020 •

edited

Loading