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

Lists with newlines between the items are malformed #65

Closed
tomkelsey opened this issue Aug 20, 2019 · 6 comments · Fixed by #127
Closed

Lists with newlines between the items are malformed #65

tomkelsey opened this issue Aug 20, 2019 · 6 comments · Fixed by #127
Labels
enhancement New feature or request released
Milestone

Comments

@tomkelsey
Copy link

tomkelsey commented Aug 20, 2019

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

Lists with newlines between the items are malformed

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. GIFs and screenshots are very helpful too.

On entering text such as (NB: copy/pasting from below didn't work - I had to type it)

1. Item one

2. Item two

3. Item three

With an explicit newline between each point and then running the filter the output is:

1. Item one

1. Item two

1. Item three

What is the expected behavior?

I think the output should be:

1. Item one

2. Item two

3. Item three

Either reformatted as a list or as plain text but still preserving the correct numbers.

Which versions of the filters or of Draft.js, and which browser / OS are affected by this issue? Did this work in previous versions of the filters / Draft.js?

I cloned the repo and edited FilterableEditor.js to have a new on-demand filter button:

<button onMouseDown={this.onFilter}>Filter</button>

With the filter as follows:

  onFilter = () => {
    const { filtered, extended } = this.props
    const filters = {
      blocks: Object.keys(extended ? BLOCKS_EXTENDED : BLOCKS),
      styles: Object.keys(extended ? STYLES_EXTENDED : STYLES),
      entities: ENTITIES,
      maxNesting: extended ? MAX_NESTING_EXTENDED : MAX_NESTING,
      whitespacedCharacters: ["\n", "\t", "📷"],
    }

    const filteredState = filterEditorState(filters, this.state.editorState)
    this.setState({ editorState: filteredState })
  }

PS. Great stuff with this package - its super helpful :)

@thibaudcolas
Copy link
Owner

Hey @tomkelsey, thanks for reporting this! Glad you find this package helpful 🙂.

I think what’s happening here is that the filtering does not recognise this as a single list because of the newlines, but it might be easier to understand if you can share the contentState of the editor here, so we can see what blocks are there before and after filtering.

Could you also clarify what you mean by "newline"? Whether it’s creating a new block in the editor, or whether it’s just inserting a line break (\n).

@thibaudcolas thibaudcolas added this to the v2.3.0 milestone Aug 21, 2019
@thibaudcolas thibaudcolas added the bug Something isn't working label Aug 21, 2019
@tomkelsey
Copy link
Author

Hey thanks for getting back to me :)

I guess the easiest example I've just thought of is if you input:

2. Hello

{
  "blocks": [
    {
      "key": "4km2d",
      "text": "2. Hello",
      "type": "unstyled",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    }
  ],
  "entityMap": {}
}

Into an editor and run the filter the output will be:

1. Hello

{
  "blocks": [
    {
      "key": "4km2d",
      "text": "Hello",
      "type": "ordered-list-item",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    }
  ],
  "entityMap": {}
}

When I guess I'd expect it to just be 2. Hello (and not converted into a list?)


For my original example:

1. Hello

2. Hello

3. Hello
{
  "blocks": [
    {
      "key": "4km2d",
      "text": "1. Hello",
      "type": "unstyled",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    },
    {
      "key": "c4os9",
      "text": "",
      "type": "unstyled",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    },
    {
      "key": "ecrei",
      "text": "2. Hello",
      "type": "unstyled",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    },
    {
      "key": "b4olh",
      "text": "",
      "type": "unstyled",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    },
    {
      "key": "nv84",
      "text": "3. Hello",
      "type": "unstyled",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    }
  ],
  "entityMap": {}
}

Is converted to:

1. Hello

1. Hello

1. Hello
  "blocks": [
    {
      "key": "4km2d",
      "text": "Hello",
      "type": "ordered-list-item",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    },
    {
      "key": "c4os9",
      "text": "",
      "type": "unstyled",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    },
    {
      "key": "ecrei",
      "text": "Hello",
      "type": "ordered-list-item",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    },
    {
      "key": "b4olh",
      "text": "",
      "type": "unstyled",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    },
    {
      "key": "nv84",
      "text": "Hello",
      "type": "ordered-list-item",
      "depth": 0,
      "inlineStyleRanges": [],
      "entityRanges": [],
      "data": {}
    }
  ],
  "entityMap": {}
}

Perhaps a solution (if possible) would be to check that the number in the first bullet point is 1 before converting to an ordered list? If greater than 1 leave as plain text?

I guess that would cause my second example to still be wrong - the first item would be converted to a list but the subsequent items would be plain text.

Perhaps you could also check that there is more than one item before converting to a list? That way my second example would all stay as plain text

@thibaudcolas thibaudcolas added enhancement New feature or request and removed bug Something isn't working labels Oct 26, 2019
@thibaudcolas
Copy link
Owner

With your extra description I think it’s quite clear to me that what you are running against is intentional. Those blocks are converted to list items because they are prefixed with something the filters recognise as a list prefix, and that conversion is needed to retain ordered lists from Word, Google Docs and Dropbox Paper.

It also makes sense that it’s creating list items all numbered "1.", since all the list items in the input are separated by unstyled blocks (not newlines).


To solve your problem, I think the easiest you could do would be to redefine your own filterEditorState based on the other filters. You can then either disable all transformations of blocks by text prefixes, or just the ones that are currently problematic:

/**
* Applies whitelist and blacklist operations to the editor content,
* to enforce it's shaped according to the options.
* Will not alter the editor state if there are no changes to make.
*/
export const filterEditorState = (
options: FilterOptions,
editorState: EditorStateType,
) => {
const {
blocks,
styles,
entities,
maxNesting,
whitespacedCharacters,
} = options
const shouldKeepEntityRange = (content, entityKey, block) => {
const entity = content.getEntity(entityKey)
const entityData = entity.getData()
const entityType = entity.getType()
const blockType = block.getType()
return (
shouldKeepEntityType(entities, entityType) &&
shouldKeepEntityByAttribute(entities, entityType, entityData) &&
!shouldRemoveImageEntity(entityType, blockType)
)
}
// Order matters. Some filters may need the information filtered out by others.
const filters = [
// 1. clean up blocks.
removeInvalidDepthBlocks,
preserveBlockByText.bind(null, PREFIX_RULES),
limitBlockDepth.bind(null, maxNesting),
// 2. reset styles and blocks.
filterInlineStyles.bind(null, styles),
// Add block types that are always enabled in Draft.js.
filterBlockTypes.bind(null, blocks.concat([UNSTYLED, ATOMIC])),
// 4. Process atomic blocks before processing entities.
preserveAtomicBlocks,
resetAtomicBlocks,
// 5. Remove entity ranges (and linked entities)
filterEntityRanges.bind(null, shouldKeepEntityRange),
// 6. Remove/filter entity-related matters.
removeInvalidAtomicBlocks.bind(null, entities),
filterEntityData.bind(null, entities),
// 7. Clone entities for which it is necessary.
cloneEntities,
// 8. Finally, do text operations.
replaceTextBySpaces.bind(null, whitespacedCharacters),
]
const content = editorState.getCurrentContent()
const nextContent = filters.reduce(
(c, filter: (ContentState) => ContentState) => filter(c),
content,
)
return applyContentWithSelection(editorState, content, nextContent)
}

Everything in there is meant to be reusable and composable, except for PREFIX_RULES.


Anyway, I’ll see if I can think of a good way to make this behavior configurable without resorting to defining your own filterEditorState, but I’m not sure what this will look like just yet.

thibaudcolas added a commit that referenced this issue Jan 3, 2020
The `filterEditorState` options now have an optional `blockTextRules` parameter, which allows users to use their own text-based block conversion rules, or entirely disable the conversions by passing an empty array. By default, `blockTextRules` is set to the filters’ built-in prefix rules for list items.
thibaudcolas added a commit that referenced this issue Jan 3, 2020
…#127)

The `filterEditorState` options now have an optional `blockTextRules` parameter, which allows users to use their own text-based block conversion rules, or entirely disable the conversions by passing an empty array. By default, `blockTextRules` is set to the filters’ built-in prefix rules for list items.
thibaudcolas added a commit that referenced this issue Jan 3, 2020
The `filterEditorState` options now have an optional `blockTextRules` parameter, which allows users to use their own text-based block conversion rules, or entirely disable the conversions by passing an empty array. By default, `blockTextRules` is set to the filters’ built-in prefix rules for list items.
thibaudcolas added a commit that referenced this issue Jan 3, 2020
# [2.3.0](v2.2.4...v2.3.0) (2020-01-03)

### Features

* **api:** add `blockTextRules` parameter to options. Fix [#65](#65) ([#127](#127)) ([2850d72](2850d72))
@thibaudcolas
Copy link
Owner

thibaudcolas commented Jan 3, 2020

@tomkelsey I’ve just pushed a change which should be released shortly, and allow you to disable or customise this behavior,

  onFilter = () => {
    const { filtered, extended } = this.props
    const filters = {
      blocks: Object.keys(extended ? BLOCKS_EXTENDED : BLOCKS),
      styles: Object.keys(extended ? STYLES_EXTENDED : STYLES),
      entities: ENTITIES,
      maxNesting: extended ? MAX_NESTING_EXTENDED : MAX_NESTING,
      whitespacedCharacters: ["\n", "\t", "📷"],
+      blockTextRules: [],
    }

    const filteredState = filterEditorState(filters, this.state.editorState)
    this.setState({ editorState: filteredState })
  }

Note setting blockTextRules to an empty array as I did here will completely disable conversion of blocks based on their text – if you want to keep some of the built-in conversions, you’ll need to copy-paste them as needed from:

const BLOCK_PREFIX_RULES = [
{
// https://regexper.com/#%5E(%C2%B7%20%7C%E2%80%A2%5Ct%7C%E2%80%A2%7C%F0%9F%93%B7%20%7C%5Ct%7C%20%5Ct)
test: "^(· |•\t|•|📷 |\t| \t)",
type: "unordered-list-item",
depth: 0,
},
// https://regexper.com/#%5E(%E2%97%A6%7Co%20%7Co%5Ct)
{ test: "^(◦|o |o\t)", type: "unordered-list-item", depth: 1 },
// https://regexper.com/#%5E(%C2%A7%20%7C%EF%82%A7%5Ct%7C%E2%97%BE)
{ test: "^(§ |\t|◾)", type: "unordered-list-item", depth: 2 },
{
// https://regexper.com/#%5E1%7B0%2C1%7D%5Cd%5C.%5B%20%5Ct%5D
test: "^1{0,1}\\d\\.[ \t]",
type: "ordered-list-item",
depth: 0,
},
{
// Roman numerals from I to XX.
// https://regexper.com/#%5Ex%7B0%2C1%7D(i%7Cii%7Ciii%7Civ%7Cv%7Cvi%7Cvii%7Cviii%7Cix%7Cx)%5C.%5B%20%5Ct%5D
test: "^x{0,1}(i|ii|iii|iv|v|vi|vii|viii|ix|x)\\.[ \t]",
type: "ordered-list-item",
depth: 2,
},
{
// There is a clash between this and the i., v., x. roman numerals.
// Those tests are executed in order though, so the roman numerals take priority.
// We do not want to match too many letters (say aa.), because those could be actual text.
// https://regexper.com/#%5E%5Ba-z%5D%5C.%5B%20%5Ct%5D
test: "^[a-z]\\.[ \t]",
type: "ordered-list-item",
depth: 1,
},
]

I hope this helps!

@thibaudcolas
Copy link
Owner

🎉 This issue is fixed in v2.3.0, available on npm: draftjs-filters@2.3.0.

Generated by 📦🚀 semantic-release

@tomkelsey
Copy link
Author

Many thanks @thibaudcolas :) Will have a look at this next week hopefully!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants