Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Markdown Table doesn't support CJK characters #1534

Closed
avasconcelos114 opened this issue Mar 22, 2018 · 7 comments
Closed

Markdown Table doesn't support CJK characters #1534

avasconcelos114 opened this issue Mar 22, 2018 · 7 comments

Comments

@avasconcelos114
Copy link

Summary

When parsing a markdown table that includes a Chinese, Japanese, or Korean character, the table doesn't render.

Environment Information

  • Device Name: Samsung Galaxy S7 (Simulator using Genymotion)
  • OS Version: Android 7.1.1
  • Mattermost App Version: master branch, latest commit being ebbbd96
  • Mattermost Server Version: 4.8.0

Steps to reproduce

  1. Create a table with any amount of columns and rows
  2. Include a Korean, Chinese, or Japanese character in any cell

Expected behavior

Should render the markdown table with CJK characters included

Observed behavior

Only the header renders, but the original text is seen below
md_table_cjk
Note: above issue happened when the Korean characters "버스" were added to the second row of the table, the same happened when testing when adding random Chinese or Japanese characters. It otherwise renders fine when the whole table only uses the alphabet

Possible fixes

I'm unfamiliar with how markdown parsing works, but running console.log on this function revealed that the rows containing CJK characters weren't even logged.

@hmhealey
Copy link
Member

Hi @avasconcelos114,

Thanks for the report. Are you able to post the original message as it appears when typed in the textbox or when you edit the post? I tried recreating what you typed, and it's working fine for me. I'm on iOS, but that shouldn't matter for this.

|No|Category|Line/Stop|
|--|--|--|
|1|Stop|School Bus Stop (LGVillage)|
|2|버스|Premiere Bus stop(Mangpo 3rd exit)|

screen shot 2018-03-22 at 10 29 45 am

screen shot 2018-03-22 at 10 29 54 am

@avasconcelos114
Copy link
Author

Hmm, interesting that it works fine in your version

I'll share another table that has the same issue happening in it, and also a table that is currently being used in production by one of the chatbots (and where we first spotted this issue). I'll also more details on the steps I took to make sure it wasn't a mistake on my part when setting things up:

Tables

| header 1 | header 2 |
| :---: | :---: |
| huh | 한글 |
|번호|구분|노선명/정류장명|
|:-:|:-:|-|
|1|정류장|망포중학교 버스정류장 (LG빌리지)|
|2|정류장|임광그대가프리미어 버스정류장(망포역 3번출구)|

Step taken to encounter error (in more detail)

  1. Cloned the repository from github
  2. Followed instructions on basic setup of a dev environment
  3. Downloaded Genymotion
  4. Set up a virtual machine (Samsung Galaxy S7 - 7.1.0 - API 25 - 1440x2560)
    Since it had been about a month since steps 1~4, before the next step I pulled again from the master branch and ran rm -rf node_modules && npm install before proceeding
  5. Build project with make run-android command
  6. Write table in English on webapp & confirm it rendered well on mobile app
  7. Edit table cell to Korean, table now looked like image below
    screenshot from 2018-03-23 10-17-44

I hope this helps on finding out where things went wrong, let me know if there is any other piece of information you need

@hmhealey
Copy link
Member

My apologies. I tested all these including the one I sent originally on a Nexus 5, and it does appear to work differently on Android than iOS. I really did not expect that.

I've filed a ticket to track this on our end (link)

@avasconcelos114
Copy link
Author

avasconcelos114 commented Mar 26, 2018

All good :)

Also, I've been looking through the markdown parser and I think I got an approximate idea about what may be going wrong (although I still don't understand why, and especially why only on Android... )

The first thing I noticed, is that the container types on every row after the first one becomes a paragraph when Korean is added to the table.
Table in English:
type_eng
Table with Korean added:
type_kor

When checking why that was the case, I noticed that what happened was that this.offset's value was always smaller than ln.length's by 1. and that in this bit of code, it seemed to be adding the children of table as paragraph instead of table_row

} else if (this.offset < ln.length && !this.blank) {
    // create paragraph container for line
    container = this.addChild('paragraph', this.offset);
    this.advanceNextNonspace();
    this.addLine();
}

and in the end it seemed that when the header row is being parsed, the following condition somehow isn't being fulfilled:

if (remaining.startsWith('|')) {
    parser.advanceOffset(1);
}

And removing removing the if condition caused the table to render normally

//if (remaining.startsWith('|')) {
    parser.advanceOffset(1);
//}

after_fixing

The remaining question is why is the condition not being fulfilled and causing the remaining nodes to be added as paragraph instead, and naturally, why is this only (seemingly) happening on Android.

Either way, I'm sure the workaround I found is bound to be problematic, but I'm hoping that this information will help you find a more adequate solution :)

Edit: Creating an array from remaining , and evaluating the first item seem to be a valid alternative to using startsWith('|'). Leaving an example of the possible changes to the original part in here:

var remaining = parser.currentLine.slice(parser.offset);
var remainingArr = remaining.split('')

var rowMatch = reTableRow.exec(remaining);
if (!rowMatch) {
    return 0;
}
        
parser.closeUnmatchedBlocks();
parser.addChild('table_row', remainingArr[0] === '|' ? 1 : 0);
        
if (remainingArr[0] === '|') {
    parser.advanceOffset(1);
}

Bonus: Found an issue that seems to be related to what's happening here

@hmhealey
Copy link
Member

Wow, nice work. The behaviour they describe in that ticket is bizarre, but I think that makes sense. I'll take a look at implementing it since I think we might do a 1.7.1 to fix that.

That thread also mention it should be fixed by upgrading React Native which we're currently doing for 1.8, but we probably wouldn't want to do that big of an upgrade for a dot release

@avasconcelos114
Copy link
Author

Ah great!

Just tested it again with your newest commit and it's working nicely :)

Thank you for taking care of it

@hmhealey
Copy link
Member

hmhealey commented Apr 2, 2018

Awesome, glad to hear it works. If you're using the version from the App Store or Play Store, it'll be fixed shortly in 1.7.1

@hmhealey hmhealey closed this as completed Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants