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.

Sign up for GitHub

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

Jump to bottom

Using p for htmlTag outputs incorrectly #3254

Closed
chicgeek opened this issue Feb 2, 2016 · 9 comments
Closed

Using p for htmlTag outputs incorrectly #3254

chicgeek opened this issue Feb 2, 2016 · 9 comments

Comments

@chicgeek
Copy link
Contributor

chicgeek commented Feb 2, 2016

Description

I have an XML file that creates a new container. It is using a <p> element, one of the valid element types for htmlTag. Using the move directive, I relocate top.search into this new element.

XML source

The XML:

<?xml version="1.0"?>
<page>
    <body>
        <referenceContainer name="main.content">
            <container name="new.container" htmlTag="p" htmlClass="new-container" />
        </referenceContainer>
        <move element="top.search" destination="new.container" />
    </body>
</page>

HTML output

Expected markup output:

<p class="new-container">
    <!-- top.search -->
</p>

Actual markup output:

<p class="new-container"></p>
<!-- top.search -->
<p></p>

Notes

  • This is on merchant_beta 8 on both our custom theme and Luma.
  • This bug does not occur with some of the other allowed elements I've tested, including div and ul. The block is nested in the new container as expected.
  • The docs specify Any valid HTML 5 tag can be used for htmlTag, though this is not the case currently. This mismatch is partly covered in issue Cant set HtmlTag to "nav" #2549. M2 provides the error: Html tag "aside" is forbidden for usage in containers. Consider to use one of the allowed: dd, div, dl, fieldset, main, header, footer, ol, p, section, table, tfoot, ul.
@alankent
Copy link

alankent commented Feb 2, 2016

Just to be clear, if you only change htmlTag="p" with htmlTag="div" what is the output?

@chicgeek
Copy link
Contributor Author

chicgeek commented Feb 3, 2016

The output is:

<div class="new-container">
    <!-- top.search -->
</div>

@alankent
Copy link

alankent commented Feb 4, 2016

I am going to leave that one for the engineers closer to the code base sorry. Did you try to repeat in M2 GA? Is the problem still there? I agree that looks strange.

@chicgeek
Copy link
Contributor Author

chicgeek commented Feb 5, 2016

We're in the process of upgrading our project to GA. I'll check again when the upgrade is done and report back.

@vrann vrann added CS labels Mar 8, 2016
@vzaporozhets vzaporozhets added PS and removed CS labels Mar 28, 2016
@mazhalai
Copy link
Contributor

Thank you we have created MAGETWO-51319 to investigate and fix.

@mazhalai mazhalai added CS and removed PS labels Mar 29, 2016
@shiftedreality shiftedreality added PS and removed CS labels Mar 30, 2016
@undefined-olha
Copy link
Contributor

Hello, @chicgeek !
Thank you for your interest in Magento.
According to W3C it is forbidden to using block elements in <p> tag. Search block in your case is a <div> and browser "tries" to render it in the right way. Please use htmlTag="div" or any other valid tag for your purpose.

@MomotenkoNatalia
Copy link
Contributor

@chicgeek Internal ticket was closed as "Not a bug". Feel free to reopen this issue If you have any additional comment.

@chicgeek
Copy link
Contributor Author

chicgeek commented Apr 8, 2016

@olgalytvynenko @MomotenkoNatalia : Is there now going to be internal ticket raised for this to update both the docs and the M2 error which implies p is supported?

@undefined-olha
Copy link
Contributor

@chicgeek, sorry, probably I was not clear enough. There is no problem in Magento and information on devdocs is correct. If you look in source of your page (in Chrome you can do it by pressing ctr+U) you will see something like this:

magetwo-51319

So the block was rendered correctly. But placing <div> (or any other block element) in paragraph it's wrong because it was designed for text formatting. If you will create some block using xml which will contain something like <span>Some</span> and place that inside htmlTag="p" container the same way as you did, everything will be fine. I hope it helped.
Please fell free to ask if you have any questions.

magento-engcom-team pushed a commit that referenced this issue Oct 4, 2018
Merge pull request #3254 from magento-tsg/2.2-develop-pr48
98ee213 
[TSG] Backporting for 2.2 (pr48) (2.2.8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@chicgeek @vrann @shiftedreality @mazhalai @alankent @MomotenkoNatalia @vzaporozhets @undefined-olha