MM-55972/MM-59809 Simplify parsing of table delimiters and improve how whitespace in tables is handled #19
Conversation
…w whitespace in tables is handled
| @@ -787,20 +782,32 @@ var blockStarts = [ | |||
| return 0; | |||
| } | |||
|
|
|||
| const nextColumn = measureNonspaceColumn(parser.nextLine); | |||
There was a problem hiding this comment.
Ensuring that the second line of the table is near the same indentation level as the first line was previously part of reTableDelimiter, but it needs to be done manually now
| // check for a delimiter first since it's stricter than the header row | ||
| const nextLine = trimSpacesAfterPipe(parser.nextLine); |
There was a problem hiding this comment.
trimSpacesAfterPipe was added as a fix for https://mattermost.atlassian.net/browse/MM-13516. Before the change I commented on above, we could only trim the trailing whitespace before passing the line into reTableDelimiter. With the whitespace handled separately, we can just use a regular old .trim to get rid of that whitespace
lib/blocks.js
Outdated
| @@ -860,6 +867,23 @@ var blockStarts = [ | |||
| } | |||
| ]; | |||
|
|
|||
| const reValidTableDelimiter = /^:?-+:?$/; | |||
There was a problem hiding this comment.
For the delimiter row to be valid, all of the cells in it must be made up of a string of dashes with a colon before or after to change the alignment of that row. They can start and end with whitespace, but that's removed before checking with the regex
There was a problem hiding this comment.
All the other regexes are between line 14095-14137. Should we put this one there too?
There was a problem hiding this comment.
Good call. I started defining the regexes with their corresponding functions in lib/inlines.js, but I guess I never did that for lib/blocks.js
| @@ -949,6 +973,27 @@ var findNextNonspace = function() { | |||
| this.indented = this.indent >= CODE_INDENT; | |||
| }; | |||
|
|
|||
| function measureNonspaceColumn(line) { | |||
There was a problem hiding this comment.
I could've extracted this from the above method, but I'm hesitant about modifying the existing functions as much as possible to avoid future conflicts, so I just copied this out
| @@ -488,20 +502,40 @@ aaa|bbb | |||
| <th>aaa</th> | |||
| </tr> | |||
| </thead></table> | |||
| ```````````````````````````````` | |||
There was a problem hiding this comment.
A few of the previous tests were wrong when comparing against GitHub, and that got broken with the new whitespace handling, so I ended up replacing them
lib/blocks.js
Outdated
| @@ -860,6 +867,23 @@ var blockStarts = [ | |||
| } | |||
| ]; | |||
|
|
|||
| const reValidTableDelimiter = /^:?-+:?$/; | |||
There was a problem hiding this comment.
All the other regexes are between line 14095-14137. Should we put this one there too?
hmhealey
added
4: Reviews Complete
Summary
The issues in the linked tickets are caused by
reTableDelimiterbeing really complicated in a way that broke RN's implementation of regular expressions despite working just fine on the ones implemented by browsers which made it hard to track down. Instead of having one really complicated regex to specifically match the delimiter row (aka the second row of the table that's all dashes), we'll now parse it as if it was a regular table row and then go back to double check if it's valid as a delimiter row (basically that it's all dashes).When doing this, I also found out that we still don't handle whitespace around tables quite correctly, particularly because we parse the first two lines of the table at once which bypasses the built-in whitespace handling for the delimiter row, so I ended up having to check the indentation level of that delimiter row manually.
Ticket Link
https://mattermost.atlassian.net/browse/MM-59809
https://mattermost.atlassian.net/browse/MM-55972