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

Optional <p> closing not implemented #1503

Closed
MacKLess opened this issue Aug 23, 2018 · 5 comments · Fixed by #1778
Closed

Optional <p> closing not implemented #1503

MacKLess opened this issue Aug 23, 2018 · 5 comments · Fixed by #1778

Comments

@MacKLess
Copy link
Collaborator

MacKLess commented Aug 23, 2018

Description

Resolved all but the optional closing <p> tags for release of v1.8.0. Someone should tackle this issue when convenient with plenty of tests as it is a guaranteed bug farm.

Input

The code looked like this before beautification:

<p> Some content
<h1> Other content
</h1>

Expected Output

The code should have looked like this after beautification:

<p> Some content
<h1> Other content
</h1>

Actual Output

The code actually looked like this after beautification:

<p> Some content
    <h1> Other content
    </h1>

Settings

Default

@bitwiseman
Copy link
Member

Seriously. Here's the text from https://www.w3.org/TR/html5/syntax.html#optional-tags:

A <p> element’s end tag may be omitted if the p element is immediately followed 
by an <address>, <article>, <aside>, <blockquote>, <details>, <div>, <dl>, 
<fieldset>, <figcaption>, <figure>, <footer>, <form>, <h1>, <h2>, <h3>, <h4>, 
<h5>, <h6>, <header>, <hr>, <main>, <nav>, <ol>, <p>, <pre, <section>, <table>, or <ul> 
element, or if there is no more content in the parent element and the parent element
is an HTML element that is not an <a>, <audio>, <del>, <ins>, <map>, 
<noscript>, or <video> element,  or an autonomous custom element.

Handling the simple case of closing a P tag when it is the the parent of one of the listed would be relatively straightforward, but beyond that this will be quite a hairball. Anyone willing to provide examples, they maybe we can work them one at time.

@nikeee
Copy link

nikeee commented Apr 3, 2020

Any news on this issue?

@bitwiseman
Copy link
Member

bitwiseman commented Apr 3, 2020

@nikeee
No yet, but reading it again maybe this isn't as bad as I first thought.

The change needs to be made here:
https://github.com/beautify-web/js-beautify/blob/e00d3b2b78600ffe590dc4a87a085fb1b586c9dd/js/src/html/beautifier.js#L766-L769

Unlike other cases, this doesn't need to walk back up the DOM to figure out if the element is auto-closes - it only applies when p is immediately before.

var p_closers = ['address', 'article', 'aside', 'blockquote', 'details', 'div', 'dl', 'fieldset', 'figcaption', 'figure', 'footer', 'form', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'header', 'hr', 'main', 'nav', 'ol', 'p', 'pre', 'section', 'table', 'ul'];
var p_parent_excludes = ['a', 'audio', 'del', 'ins', 'map', 'noscript', 'video'];

// ...

else if (parser_token.parent.tag_name === 'p' && p_closers.indexOf(parser_token.tag_name) !== -1) {
    // TODO: check for the parent element is an HTML element that is not an <a>, <audio>, <del>, <ins>, <map>, <noscript>, or <video> element,  or an autonomous custom element.
    // To do this right, this needs to be coded as an inclusion of the inverse of the exclusion above.
    // But to start with (if we ignore "autonomous custom elements") the exclusion would be fine.
    var p_parent = parser_token.parent.parent;
    if (p_parent === null || p_parent_excludes.indexOf(p_parent.tag_name) === -1) {
      result = result || this._tag_stack.try_pop('p');
    }
  }
}

bitwiseman added a commit to bitwiseman/js-beautify that referenced this issue Apr 4, 2020
bitwiseman added a commit to bitwiseman/js-beautify that referenced this issue Apr 4, 2020
@bitwiseman
Copy link
Member

@nikeee
Thanks for bringing this up. Could you do me a favor and try some of your inputs on https://beautifier.io/ to make sure my fix is working?

@nikeee
Copy link

nikeee commented Jan 5, 2021

It works as I expected. Thanks! :)

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

Successfully merging a pull request may close this issue.

3 participants