Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.59k stars 799 forks source link

Tiled Gallery set to 0px width/height when child of inline element #898

Open mrwweb opened 10 years ago

mrwweb commented 10 years ago

After hours of banging my head against my keyboard, I was able to diagnose what I believe is a bug with the Tiled Gallery sizing JS. I think it may have possibly been related to other issues reported in the past that weren't solved. I think I've seen this before too.

When a gallery is the immediate descendant of an inline element,* I believe the JS that finds the parent item's dimensions gets a width and height of 0 from that inline element and then sizes the gallery to match—adding inline styles of width: 0px; height: 0px; on a bunch of elements in the gallery and effectively making it invisible.

Setting the <span> to display as a block element immediately made the gallery appear.

* In my case, I was using a <span> for inserting schema markup which strikes me as a valid usecase.

georgestephanis commented 10 years ago

Cc: @rase- (who is our tiled gallery wizard)

rase- commented 10 years ago

Putting block elements inside inline elements in general is not a valid use case, as inline elements should generally only contain other inline elements or data, but I think we should deal with the possibility of this happening in some way.

Should we detect the case and make the container display: block?

Thoughts @georgestephanis?

mrwweb commented 10 years ago

That's less true in HTML5.

From https://developer.mozilla.org/en-US/docs/Web/HTML/Block-level_elements: "The distinction of block-level vs. inline elements is used in HTML specifications up to 4.01. In HTML5, this binary distinction is replaced with a more complex set of content categories https://developer.mozilla.org/en-US/docs/HTML/Content_categories. The "block-level" category roughly corresponds to the category of flow content https://developer.mozilla.org/en-US/docs/HTML/Content_categories#Flow_content in HTML5, while "inline" corresponds to phrasing content https://developer.mozilla.org/en-US/docs/HTML/Content_categories#Phrasing_content, but there are additional categories."

I do think dealing with it would be good. If possible, it would probably be safer to traverse the DOM tree up to the nearest block-level parent. On Aug 7, 2014 6:43 AM, "Tony Kovanen" notifications@github.com wrote:

Putting block elements inside inline elements in general is not a valid use case, as inline elements should generally only contain other inline elements or data, but I think we should deal with the possibility of this happening in some way.

Should we detect the case and make the container display: block?

Thoughts @georgestephanis https://github.com/georgestephanis?

— Reply to this email directly or view it on GitHub https://github.com/Automattic/jetpack/issues/898#issuecomment-51456299.

rase- commented 10 years ago

Even with the category distinction I still think it generally holds that it's not a good idea to put block elements inside inline flow control elements for more predictable rendering.

But yes, I agree with you that we should take this into account. Also, I think your suggestion is better than my initial suggestion, because it's not a very good idea to start messing with styles in elements that are not generated by the Tiled Galleries module, if we can also make them work by not having to change the styles at all. :)

bhubbard commented 8 years ago

I recently updated all the galleries and carousel to support schema by default, and this was released in the last update, so maybe his code is not even required? @mrwweb ?

Pull Request https://github.com/Automattic/jetpack/pull/3363

stale[bot] commented 6 years ago

This issue has been marked as stale. This happened because:

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

github-actions[bot] commented 3 years ago

This issue has been marked as stale. This happened because:

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.