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

Feature request: add an option to allow inline elements inside a span #461

Closed
jznf opened this issue Oct 22, 2016 · 9 comments
Closed

Feature request: add an option to allow inline elements inside a span #461

jznf opened this issue Oct 22, 2016 · 9 comments
Milestone

Comments

@jznf
Copy link

jznf commented Oct 22, 2016

current behaviour is that:

<span><button>OK</button></span>

throws:

Warning: inserting implicit <span>
Warning: replacing unexpected button with </button>
Warning: missing </button>

and the final result is:

<span><button><span>OK</span></button></span>

It might be handy to have an option to prevent this behaviour and let the original markup unchanged as it is valid and it's a part of quite common pattern.

@geoffmcl
Copy link
Contributor

@jznf, thank you for bringing this here...

As discussed, off list, this has been the behavior of tidy for a long time, thus any change should be subject to a new option, to suspend this action... probably not too difficult...

Very much welcome any thoughts on this... thanks...

@geoffmcl geoffmcl added this to the 5.3 milestone Oct 31, 2016
@geoffmcl
Copy link
Contributor

geoffmcl commented Mar 1, 2017

As we are about to release 5.4, and there has been no progress on this Feature Request, moving it to next, 5.5 milestone...

I have constructed a sample in_461.html which passes W3C validation, so certainly think this feature should be added, even as the default...

As always further comments, patches or a PR very welcome... thanks...

@geoffmcl geoffmcl modified the milestones: 5.5, 5.3 Mar 1, 2017
@lhchavez
Copy link
Contributor

lhchavez commented Apr 15, 2017

Ah this is just a difference in the HTML5 spec. Previously <button>..</button> was allowed to contain flow as children (which includes blocks and inlines). In HTML 5, that is restricted to phrasing content (only inline elements)

I'll send a PR for this.

lhchavez added a commit to lhchavez/tidy-html5 that referenced this issue Apr 15, 2017
HTML 5 restricts the children of <button> elements to only phrasing
content. HTML 4 also allowed block elements.

In particular, this change allows code such as

    <span><button>OK</button></span>

to be left as-is, since the <span> does not need duplication for
crossing a block boundary. Fixes htacg#461.
lhchavez added a commit to lhchavez/tidy-html5 that referenced this issue Apr 15, 2017
This change adds the InsertInlineTags flag (default=off) that propagates
inline elements across block boundaries. This causes

    <span><div>OK</div></span>

to become

    <span><div><span>OK</span></div></span>

This fixes issue htacg#461.
lhchavez added a commit to lhchavez/tidy-html5 that referenced this issue Apr 15, 2017
HTML 5 restricts the children of <button> elements to only phrasing
content. HTML 4 also allowed block elements.

In particular, this change allows code such as

    <span><button>OK</button></span>

to be left as-is, since the <span> does not need duplication for
crossing a block boundary. Mitigates htacg#461.
@geoffmcl
Copy link
Contributor

geoffmcl commented May 5, 2017

@lhchavez as stated. thank you for your PR #531... While this works fine, I have now proposed an alternative fix, PR #540, that not only addresses this <button> issue, but tidies up some other code... including an important fix for the TY_(IsHTML5Mode)(doc ) service...

I have been delaying merging #540 hoping you would get a chance to pull and test the issue-461 branch, and confirm. @balthisar has already tested, and confirmed...

And now there is also a branch issue-461 in the tidy-tests repo, which should match this change, and should pass 100% - stage 1 - exit codes, and 2 - diff results vs expects...

I understand RL must come first, but you say closing your #531, and/or adding a +1 here, or something, would indicate for me to move forward...

I am also holding off on a few other PRs to get #540 in place first... which will bump us to 5.5.15... thanks...

@lhchavez
Copy link
Contributor

lhchavez commented May 5, 2017

oh, sorry i've been in the critical path. please go ahead and merge that other change since it's clearly better for everyone!

if i end up seeing other issues, i'll send further PRs. i'll close this one.

@lhchavez
Copy link
Contributor

lhchavez commented May 5, 2017

(and by "this one" i mean the one i sent)

@geoffmcl
Copy link
Contributor

geoffmcl commented May 5, 2017

@lhchavez thanks for the quick reply... sorry to push when there may be other critical RL items...no apology needed...

It's a bit late in my working day today, to get into merges, etc, but will get to it tomorrow, or soonest... thanks...

@balthisar
Copy link
Member

Should I merge everything we've both okayed? I'm still GMT-5, so might be able to get to it tonight.

geoffmcl added a commit that referenced this issue May 6, 2017
Issue #461 - alternative patch for this issue
@jznf
Copy link
Author

jznf commented May 6, 2017

THANK YOU!!!

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

No branches or pull requests

4 participants