plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
254 stars 191 forks source link

Review captioned image markup #2020

Closed rodfersou closed 1 year ago

rodfersou commented 7 years ago

BUG/PROBLEM REPORT (OR OTHER COMMON ISSUE)

It looks like we need to define a new markup for captioned images. We suggest this one. And the fix would be as simple as change this template.

Is it possible to have this fixed on both Plone 4.3.x and Plone 5.x ?

What I did:

What I expect to happen:

Have something like:

<div>
  <p>
    <dl>
      ... image markup ...
    </dl>
    Other text
  </p>
</div>

What actually happened:

The <p> content is kicked out because <dl> tag can't be inserted into the paragraph:

<div>
  <p></p>
  <dl>
    ... image markup ...
  </dl>
  Other text
</div>

This looks that lxml library has some code that try to fix our markup:

ipdb> lxml.html.tostring(lxml.html.fromstring('<p></p>'))
'<p></p>'
ipdb> lxml.html.tostring(lxml.html.fromstring('<p><dl></dl></p>'))
'<div><p></p><dl></dl></div>'

What version of Plone/ Addons I am using:

This bug happen on all Plone versions

hvelarde commented 7 years ago

captioned images should be inserted using the <figure> and <figcaption> tags; the problem is that, different from <img>, <figure> can't be placed inside a <p> tag.

the TinyMCE plugin that inserts the caption should move out of a paragraph any <img> tag before inserting it as a captioned image.

so, for instance:

<p>foo <img src="bar">.</p>

becomes:

<p>foo.</p>
<figure>
  <img src="bar" alt="bar">
  <figcaption>bar</figcaption>
</figure> 

or:

<figure>
  <img src="bar" alt="bar">
  <figcaption>bar</figcaption>
</figure> 
<p>foo.</p>
rodfersou commented 7 years ago

@hvelarde it looks like we need a special css to put the image in the middle of the paragraph

hvelarde commented 7 years ago

keep it simple: captioned images can not be used inside a paragraph; problem solved.

rodfersou commented 7 years ago

at Plone 4.3 we have: a configlet option linked to an utility to flag outputfilters to transform img tag

Also tinymce image plugin adds captioned class into the image (when the flag is turned on)

rodfersou commented 7 years ago

at Plone 5.1 we miss the configlet option, miss the utility, then outputfilters don't find the utility and assume False and never transform the img tag

Also new image plugin don't have the option to add captioned class into image

rodfersou commented 7 years ago

Still about Plone 5.1 from buildout.coredev, even if I add by hand the missing class at <img> tag and force the outputfilters flag to always return True the image is never show to News Item content type.

The image is never show with or without the outputfilters transform. It looks like it is took out of the generated HTML at other step of the save process.

rodfersou commented 7 years ago

If html filtering is disabled the image tag start showing at Plone 5.1.

This is a new bug since <img> tag is not listed to be filtered out.

hvelarde commented 7 years ago

@plone/framework-team can someone help here?

rodfersou commented 7 years ago

@hvelarde I think the level of this issue is not easy anymore

MrTango commented 7 years ago

Just run ito this on Midsummer sprint while fixing tests. But the good news is, that the transforms (safe_html) will work way better soon and also be in sync with TinyMCE. But the captions story seems to be broken too. It's probably a good idea to create the new markup and find a way to either build the right markup in TinyMCE or refacture the caption transform in outputfilter to do it right. But we disable the style attribute by default now, so we should use it here. I think it should be doable without these specific styles.

thet commented 5 years ago

See: https://github.com/plone/mockup/pull/911

skleinfeldt commented 5 years ago

Keep in mind a11y ticket #2836 - hopefully not making it difficult to eventually implement that. W3C's Techniques for providing useful text alternatives page has lots of markup examples.