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

Delete empty paragraphs when backspacing #3732

Merged
merged 3 commits into from Dec 14, 2017

Conversation

Projects
None yet
8 participants
@noisysocks
Member

noisysocks commented Nov 30, 2017

This is a rough attempt to fix #3624.

When backspacing from an empty paragraph into a block that has no merge function (e.g. an image), the empty paragraph should be deleted.

Out with the old:

old-behaviour

In with the new:

new-behaviour

How to test

  1. Have multiple paragraphs.
  2. Backspace to empty out a paragraph and backspace into previous one, verify the empty paragraph disappears.
  3. Now insert an image, and backspace into image from the empty paragraph block, verify that the empty paragraph disappears.
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 30, 2017

Codecov Report

Merging #3732 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3732      +/-   ##
==========================================
- Coverage    37.7%   37.58%   -0.12%     
==========================================
  Files         279      278       -1     
  Lines        6737     6726      -11     
  Branches     1226     1227       +1     
==========================================
- Hits         2540     2528      -12     
- Misses       3536     3537       +1     
  Partials      661      661
Impacted Files Coverage Δ
editor/effects.js 58.2% <100%> (+0.63%) ⬆️
components/panel/row.js 0% <0%> (-100%) ⬇️
editor/utils/mobile/index.js 57.14% <0%> (-17.86%) ⬇️
editor/components/block-list/block.js 0.74% <0%> (-1.5%) ⬇️
editor/reducer.js 90.27% <0%> (-0.11%) ⬇️
editor/selectors.js 93.43% <0%> (-0.04%) ⬇️
element/index.js 100% <0%> (ø) ⬆️
blocks/hooks/index.js 100% <0%> (ø) ⬆️
editor/edit-post/layout/index.js 0% <0%> (ø) ⬆️
editor/store-defaults.js 100% <0%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 499b320...e5383ce. Read the comment docs.

codecov bot commented Nov 30, 2017

Codecov Report

Merging #3732 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3732      +/-   ##
==========================================
- Coverage    37.7%   37.58%   -0.12%     
==========================================
  Files         279      278       -1     
  Lines        6737     6726      -11     
  Branches     1226     1227       +1     
==========================================
- Hits         2540     2528      -12     
- Misses       3536     3537       +1     
  Partials      661      661
Impacted Files Coverage Δ
editor/effects.js 58.2% <100%> (+0.63%) ⬆️
components/panel/row.js 0% <0%> (-100%) ⬇️
editor/utils/mobile/index.js 57.14% <0%> (-17.86%) ⬇️
editor/components/block-list/block.js 0.74% <0%> (-1.5%) ⬇️
editor/reducer.js 90.27% <0%> (-0.11%) ⬇️
editor/selectors.js 93.43% <0%> (-0.04%) ⬇️
element/index.js 100% <0%> (ø) ⬆️
blocks/hooks/index.js 100% <0%> (ø) ⬆️
editor/edit-post/layout/index.js 0% <0%> (ø) ⬆️
editor/store-defaults.js 100% <0%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 499b320...e5383ce. Read the comment docs.

Show outdated Hide outdated editor/effects.js Outdated
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 30, 2017

Member

This merge property is also a bit strange, try the following:

Add an empty list and a quote and try to delete quote with backspace. It merges them together producing the list of 3 empty items:

empty-block

Member

gziolo commented Nov 30, 2017

This merge property is also a bit strange, try the following:

Add an empty list and a quote and try to delete quote with backspace. It merges them together producing the list of 3 empty items:

empty-block

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Dec 4, 2017

Member

@gziolo it looks like that strangeness is caused by an unrelated bug in the core/quotecore/list transform. e5383ce ought to fix it!

Member

noisysocks commented Dec 4, 2017

@gziolo it looks like that strangeness is caused by an unrelated bug in the core/quotecore/list transform. e5383ce ought to fix it!

@gziolo gziolo requested review from karmatosed and jasmussen Dec 4, 2017

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 4, 2017

Contributor

Seriously cool work. In my testing, behavior wise, this works exactly like it needs to! 👍 👍 from an experience point of view.

Contributor

jasmussen commented Dec 4, 2017

Seriously cool work. In my testing, behavior wise, this works exactly like it needs to! 👍 👍 from an experience point of view.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 5, 2017

Member

A small thing that I really appreciate from this change: Clicking into the "Write your story" and hitting Backspace resets to remove the inserted paragraph block.

Member

aduth commented Dec 5, 2017

A small thing that I really appreciate from this change: Clicking into the "Write your story" and hitting Backspace resets to remove the inserted paragraph block.

Show outdated Hide outdated blocks/editable/index.js Outdated
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 5, 2017

Member

I should note to my review, functionally it works great and I'm glad to see it handled in Editable entirely.

Member

aduth commented Dec 5, 2017

I should note to my review, functionally it works great and I'm glad to see it handled in Editable entirely.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 5, 2017

Member

Hmm, a potential issue is that some blocks may have multiple Editables, and one of them being empty is not a sufficient metric to determine that the block should be replaced. Example: Pressing backspace from a quote citation, when the quote itself has text, shouldn't delete the block.

Member

aduth commented Dec 5, 2017

Hmm, a potential issue is that some blocks may have multiple Editables, and one of them being empty is not a sufficient metric to determine that the block should be replaced. Example: Pressing backspace from a quote citation, when the quote itself has text, shouldn't delete the block.

@mcsf

Nice work, but it seems the replace behavior takes precedence over the merge one, leading to unexpected outcomes. Note how focus is lost when deleting into another paragraph:

Before After
gutenberg-3732-merge-p gutenberg-3732-replace-p
@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Dec 6, 2017

Member

Hmm, a potential issue is that some blocks may have multiple Editables, and one of them being empty is not a sufficient metric to determine that the block should be replaced.

Good point... there's a lot of dragons subtleties here.

This approach kind of works right now because in practice it's only core/paragraph that has both onMerge and onReplace passed to its Editable.

Unfortunately that means that the bug that this PR attempts to fix is only fixed for paragraph blocks and not quotes (which has onMerge but not onReplace):

quote-bug

If we give core/quote an onReplace, we fix the problem:

quote-bug-2

BUT doing this means we now have the problem you describe where a quote that contains only a citation can be incorrectly deleted:

quote-bug-3


Long story short, I think that patching Editable is unworkable. To fix this bug in the general case we need the merge/deletion logic to read the block's entire state.

I have two ideas:

  1. Go back to what we had before where each block defines an isEmpty method which the MERGE_BLOCKS effect can use to determine if that block should be deleted when being merged. I like the explicitness of this approach, but it does mean extra API surface.
  2. Same as (1) but instead of each block defining an isEmpty we build a method that compares each of the block's attributes against the block's default attributes. If no attribute differs from its default, then the block is empty and can be deleted when being merged. I think this might be our best bet.
Member

noisysocks commented Dec 6, 2017

Hmm, a potential issue is that some blocks may have multiple Editables, and one of them being empty is not a sufficient metric to determine that the block should be replaced.

Good point... there's a lot of dragons subtleties here.

This approach kind of works right now because in practice it's only core/paragraph that has both onMerge and onReplace passed to its Editable.

Unfortunately that means that the bug that this PR attempts to fix is only fixed for paragraph blocks and not quotes (which has onMerge but not onReplace):

quote-bug

If we give core/quote an onReplace, we fix the problem:

quote-bug-2

BUT doing this means we now have the problem you describe where a quote that contains only a citation can be incorrectly deleted:

quote-bug-3


Long story short, I think that patching Editable is unworkable. To fix this bug in the general case we need the merge/deletion logic to read the block's entire state.

I have two ideas:

  1. Go back to what we had before where each block defines an isEmpty method which the MERGE_BLOCKS effect can use to determine if that block should be deleted when being merged. I like the explicitness of this approach, but it does mean extra API surface.
  2. Same as (1) but instead of each block defining an isEmpty we build a method that compares each of the block's attributes against the block's default attributes. If no attribute differs from its default, then the block is empty and can be deleted when being merged. I think this might be our best bet.
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 11, 2017

Member

Could we add a new callback onRemove to Editable which is called as in the current implementation where we delete an empty Editable? Then, the block could decide if onRemove should be handled as onReplace( [] ) (i.e. paragraph, or in other blocks when first checking that all other fields are empty as well).

Member

aduth commented Dec 11, 2017

Could we add a new callback onRemove to Editable which is called as in the current implementation where we delete an empty Editable? Then, the block could decide if onRemove should be handled as onReplace( [] ) (i.e. paragraph, or in other blocks when first checking that all other fields are empty as well).

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Dec 13, 2017

Member

Could we add a new callback onRemove to Editable which is called as in the current implementation where we delete an empty Editable?

Nice suggestion. I've done this and it seems to work pretty well.

I also fixed a similar bug to what @gziolo found where if one presses backspace in an empty paragraph block that follows a list block, an empty <li> is created.

Member

noisysocks commented Dec 13, 2017

Could we add a new callback onRemove to Editable which is called as in the current implementation where we delete an empty Editable?

Nice suggestion. I've done this and it seems to work pretty well.

I also fixed a similar bug to what @gziolo found where if one presses backspace in an empty paragraph block that follows a list block, an empty <li> is created.

@aduth

aduth approved these changes Dec 13, 2017

This seems to work quite nicely.

Show outdated Hide outdated blocks/editable/index.js Outdated
Show outdated Hide outdated blocks/editable/index.js Outdated

noisysocks added some commits Dec 12, 2017

Delete empty paragraphs when backspacing
When backspacing from an empty paragraph into a block that has no merge
function (e.g. an image), the empty paragraph should be deleted.

Empty lists, headings and quotes are treated similarly.
Improve core/quote -> core/list conversion
Reduce the amount of times that empty <li>s are created when converting
a core/quote to a core/list.

The conversion logic is:

* 'Quote' / 'Citation' -> `<li>Quote</li><li>Citation</li>`
* '' / 'Citation' -> `<li></li><li>Citation</li>`
* 'Quote' / '' -> `<li>Quote</li>`
* '' / '' -> ``
Improve core/paragraph -> core/list conversion
Prevent empty paragraphs from being converted to an empty <li>.

@noisysocks noisysocks merged commit e421572 into master Dec 14, 2017

3 checks passed

codecov/project 38.31% (-0.11%) compared to 9bb6609
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@noisysocks noisysocks deleted the try/deleting-empty-blocks-when-backspacing-into-an-image branch Dec 14, 2017

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 10, 2018

Contributor

Nice work here.

Contributor

mtias commented Jan 10, 2018

Nice work here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment