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

Parser (Fix): Output freeform content before void blocks #9984

Merged
merged 6 commits into from Sep 18, 2018

Conversation

Projects
None yet
4 participants
@dmsnell
Contributor

dmsnell commented Sep 18, 2018

Resolves #9968

It was noted that a classic block preceding a void block would
disappear in the editor while if that same classic block preceded
the long-form non-void representation of an empty block then things
would load as expected.

This behavior was determined to originate in the new default parser
in #8083 and the bug was that with void blocks we weren't sending
any preceding HTML soup/freeform content into the output list.

In this patch I've duplicated some code from the block-closing
function of the parser to spit out this content when a void block
is at the top-level of the document.

This bug did not appear when void blocks are nested because it's
the parent block that eats HTML soup. In the case of the top-level
void however we were immediately pushing that void block to the
output list and neglecting the freeform HTML.

I've added a few tests to verify and demonstrate this behavior.
Actually, since I wasn't sure what was wrong I wrote the tests first
to try and understand the behaviors and bugs. There are a few tests
that are thus not entirely essential but worthwhile to have in here.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards. (N/A)
  • My code has proper inline documentation.
Parser (Fix): Output freeform content before void blocks
Resolves #9968

It was noted that a classic block preceding a void block would
disappear in the editor while if that same classic block preceded
the long-form non-void representation of an empty block then things
would load as expected.

This behavior was determined to originate in the new default parser
in #8083 and the bug was that with void blocks we weren't sending
any preceding HTML soup/freeform content into the output list.

In this patch I've duplicated some code from the block-closing
function of the parser to spit out this content when a void block
is at the top-level of the document.

This bug did not appear when void blocks are nested because it's
the parent block that eats HTML soup. In the case of the top-level
void however we were immediately pushing that void block to the
output list and neglecting the freeform HTML.

I've added a few tests to verify and demonstrate this behavior.
Actually, since I wasn't sure what was wrong I wrote the tests first
to try and understand the behaviors and bugs. There are a few tests
that are thus not entirely essential but worthwhile to have in here.
@mcsf

mcsf approved these changes Sep 18, 2018

Thanks for the quick fix, looks good!

@mcsf mcsf added this to the 3.9 milestone Sep 18, 2018

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Sep 18, 2018

Contributor

Confirmed the test from #9968 and was able to reproduce the missing classic block in master while seeing it appear as expected here both in the editor and in the page view.

Contributor

dmsnell commented Sep 18, 2018

Confirmed the test from #9968 and was able to reproduce the missing classic block in master while seeing it appear as expected here both in the editor and in the page view.

dmsnell added some commits Sep 18, 2018

@dmsnell dmsnell merged commit 38e1ae0 into master Sep 18, 2018

2 checks passed

codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +17.85% compared to c28d2db
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dmsnell dmsnell deleted the fix/9968-no-html-soup-before-void-blocks branch Sep 18, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 19, 2018

Member

Are there PHP tests [at all / for the fix intended here] ?

Member

aduth commented Sep 19, 2018

Are there PHP tests [at all / for the fix intended here] ?

@mcsf mcsf referenced this pull request Sep 19, 2018

Merged

Parser: Propose new hand-coded parser #8083

4 of 4 tasks complete
@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Sep 19, 2018

Contributor

Are there PHP tests [at all / for the fix intended here] ?

No. Good point.

Contributor

dmsnell commented Sep 19, 2018

Are there PHP tests [at all / for the fix intended here] ?

No. Good point.

mcsf pushed a commit that referenced this pull request Sep 21, 2018

Parser (Fix): Output freeform content before void blocks (#9984)
* Parser (Fix): Output freeform content before void blocks

Resolves #9968

It was noted that a classic block preceding a void block would
disappear in the editor while if that same classic block preceded
the long-form non-void representation of an empty block then things
would load as expected.

This behavior was determined to originate in the new default parser
in #8083 and the bug was that with void blocks we weren't sending
any preceding HTML soup/freeform content into the output list.

In this patch I've duplicated some code from the block-closing
function of the parser to spit out this content when a void block
is at the top-level of the document.

This bug did not appear when void blocks are nested because it's
the parent block that eats HTML soup. In the case of the top-level
void however we were immediately pushing that void block to the
output list and neglecting the freeform HTML.

I've added a few tests to verify and demonstrate this behavior.
Actually, since I wasn't sure what was wrong I wrote the tests first
to try and understand the behaviors and bugs. There are a few tests
that are thus not entirely essential but worthwhile to have in here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment