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

Can't insert a table within a table like the release notes advertise. #9543

Closed
futuremotiondev opened this issue Dec 17, 2023 · 1 comment
Closed
Labels
bug It's a bug desktop All desktop platforms high High priority issues

Comments

@futuremotiondev
Copy link

futuremotiondev commented Dec 17, 2023

Operating system

Windows

Joplin version

2.13.9

Desktop version info

Joplin 2.13.9 (prod, win32)

Sync Version: 3
Profile Version: 44
Keychain Supported: Yes

Revision: 1bbec44

Code Section: 1.0.6
CodeMirror Options: 1.1.0
Csv Import: 1.0.1
Menu items, Shortcuts, Toolbar icons: 1.1.0
Quick HTML tags: 0.2.0
Simple Backup: 1.3.3
Simple Highlighter: 1.0.0
Suitcase: 0.3.2
To Google Search: 1.0.0

Current behaviour

The new Joplin release notes advertise the following:

20231203-rte-table

In this update, significant enhancements have been implemented in the Rich Text Editor to enhance user experience. One notable improvement is the ability to seamlessly embed various content within tables, including lists and other tables, which was a frequently requested feature.

Unfortunately, when I insert a table in the rich text editor and then switch to the markdown editor and then back to the RTE, the table is removed.

Screenshots:

Before

before

After

after

Can someone guide me in the right direction on how to get this working?

Thanks.

Expected behavior

Keep the table within the table.

Logs

No response

@futuremotiondev futuremotiondev added the bug It's a bug label Dec 17, 2023
@laurent22 laurent22 added desktop All desktop platforms high High priority issues labels Dec 17, 2023
@personalizedrefrigerator
Copy link
Collaborator

personalizedrefrigerator commented Dec 21, 2023

It seems that adding a header, list, or similar is currently necessary to preserve nested tables.

For example,
screenshot: Nested tables, outer table has a list
works, but
screenshot: Nested tables, outer table has no lists/headers
does not.

Relevant tests:

Edit 2: Possible fix:

The following diff fixes the issue and illustrates the source of the problem, but I don't like the solution because it changes Turndown's behavior:

Diff
diff --git a/packages/app-cli/tests/html_to_md/preserve_nested_tables.html b/packages/app-cli/tests/html_to_md/preserve_nested_tables.html
index 3e0509f91..335c6c13a 100644
--- a/packages/app-cli/tests/html_to_md/preserve_nested_tables.html
+++ b/packages/app-cli/tests/html_to_md/preserve_nested_tables.html
@@ -1,10 +1,10 @@
 <body>
-<table border="5px" bordercolor="#8707B0">
+<table>
 <tr>
 <td>Left side of the main table</td>
 <td>
-<table border="5px" bordercolor="#F35557">
-<h4 align="center">Nested Table</h4>
+<table>
+<b>Nested Table</b>
 <tr>
 <td>nested table C1</td>
 <td>nested table C2</td>
diff --git a/packages/app-cli/tests/html_to_md/preserve_nested_tables.md b/packages/app-cli/tests/html_to_md/preserve_nested_tables.md
index f9c47f3bb..4c3d10dde 100644
--- a/packages/app-cli/tests/html_to_md/preserve_nested_tables.md
+++ b/packages/app-cli/tests/html_to_md/preserve_nested_tables.md
@@ -1 +1 @@
-<table border="5px" bordercolor="#8707B0"><tbody><tr><td>Left side of the main table</td><td><h4 align="center">Nested Table</h4><table border="5px" bordercolor="#F35557"><tbody><tr><td>nested table C1</td><td>nested table C2</td></tr><tr><td>nested table</td><td>nested table</td></tr></tbody></table></td></tr></tbody></table>
\ No newline at end of file
+<table><tbody><tr><td>Left side of the main table</td><td><b>Nested Table</b><table><tbody><tr><td>nested table C1</td><td>nested table C2</td></tr><tr><td>nested table</td><td>nested table</td></tr></tbody></table></td></tr></tbody></table>
\ No newline at end of file
diff --git a/packages/turndown/src/rules.js b/packages/turndown/src/rules.js
index 5414fcd6f..a281ccb35 100644
--- a/packages/turndown/src/rules.js
+++ b/packages/turndown/src/rules.js
@@ -46,8 +46,8 @@ Rules.prototype = {
     if (node.isBlank) return this.blankRule
     var rule
 
-    if ((rule = findRule(this.array, node, this.options))) return rule
     if ((rule = findRule(this._keep, node, this.options))) return rule
+    if ((rule = findRule(this.array, node, this.options))) return rule
     if ((rule = findRule(this._remove, node, this.options))) return rule
 
     return this.defaultRule

Currently, we keep nested tables using turndown.keep. This function adds a keep rule that is documented to keep anything matching its arguments.

Turndown, however, checks keep rules after checking normal rules (rules added with .add). Because we already have a mapping rule defined for TABLE nodes, the keep rule is ignored (and the mapping rule is used instead).

The above diff changes Turndown to check keep rules first. I don't like this solution because it makes it more difficult for us to maintain our fork of Turndown. A better, more complete solution, would be to update our table rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug desktop All desktop platforms high High priority issues
Projects
None yet
Development

No branches or pull requests

3 participants