Big images break side-by-side revision viewer + inconsistency when displaying images

[a.hoereth]

Images with big width break the two column layout atm. I think width: 100% would be nicer for images in the revision viewer.

Also: Images no longer available are displayed as broken. Should maybe be enhanced to a warning like "attachment no longer available" instead.

revisions-big-broken-img.png shows a revision on the right with a broken image. The two columns lost their 48% width each ratio. revisions-big-img-likeitshouldbe.png shows a image which is displayed as html/shortcode combination instead. The latter is nicer, but not so helpful for end-users.

Which way to go?

[ocean90]

Good point. HTML should be escaped. Noticed this a while ago with HTML lists too.

(a.hoereth applies for GSoC, so please leave the ticket for him first. :)

[a.hoereth]

Exchanges wp_kses_post with esc_html

[a.hoereth]

wp_kses_post was used before. This leaves images etc intact. Exchanged it with esc_html which now escapes all image html etc. Thinks this one is better for revisions. The layout won't break any more and we do not need to think about no longer available attachments here.

[ocean90]

For the record: The behavior was changed in [23506] - htmlspecialchars was replaced by wp_kses_post.

[adamsilverstein]

Replying to ocean90:

For the record: The behavior was changed in [23506] - htmlspecialchars was replaced by wp_kses_post.

i don't think we want to change this back. the reason we switched to wp_kses_post is to allow the Diff view to show more of a WYSIWYG view - not an HTML/Code view. with this patch everything goes back to source code, before it i see the formatting in a visual view with images and styles intact. if changes are made within an image code i see the source code.

two options i think are better: add a 100% width as suggested, and/or switch to the single column view (see ​testing patch) which is much less likely to have this issue.

screenshots - before patch:

after patch:

[a.hoereth]

I get your point and also think that the wp_kses_post is more useful from an user point of view.
Still to notice: a new revision is saved when editing the image source code. You won't see any difference between the revisions using wp_kses_post.

What about shortcodes in revision view? [caption] for example.

[ocean90]

Replying to adamsilverstein:

before it i see the formatting in a visual view with images and styles intact. if changes are made within an image code i see the source code.

Okay, then I have missed this discussion. But for me it looks inconsistent, if have this mixed behavior - for example when I change a image - ​http://cl.ly/OkOc - I would expect to see the image not the source code.

[adamsilverstein]

Replying to ocean90:

Replying to adamsilverstein:

before it i see the formatting in a visual view with images and styles intact. if changes are made within an image code i see the source code.

Okay, then I have missed this discussion. But for me it looks inconsistent, if have this mixed behavior - for example when I change a image - ​http://cl.ly/OkOc - I would expect to see the image not the source code.

in this case the diff engine thinks you have just changed part of the line and is highlighting the differences between the two image source codes. if you add a new line and an image you will see a complete image, or remove an image and put text on that line.

[adamsilverstein]

Replying to a.hoereth:

I get your point and also think that the wp_kses_post is more useful from an user point of view.
Still to notice: a new revision is saved when editing the image source code. You won't see any difference between the revisions using wp_kses_post.

i don't think these are issues as they revert to a source view:

you will see the image in source code view in the comparison window -

What about shortcodes in revision view? [caption] for example.

captions will show in source code, changes are highlighted

[a.hoereth]

Replying to adamsilverstein:

What about shortcodes in revision view? [caption] for example.

captions will show in source code, changes are highlighted

I was talking about considering executing shortcodes in the revision view - at least core shortcodes like captions. Instead of showing [caption]PICTURE Textcaption display the combination minimalistically styled.

[adamsilverstein]

Replying to a.hoereth:

Replying to adamsilverstein:

What about shortcodes in revision view? [caption] for example.

captions will show in source code, changes are highlighted

I was talking about considering executing shortcodes in the revision view - at least core shortcodes like captions. Instead of showing [caption]PICTURE Textcaption display the combination minimalistically styled.

good point, i hadn't considered supporting that. i think the underlying goal is to bring the display as close as possible to the visual editor view - so i think supporting core shortcodes like [caption ...] makes sense. feel free to open a new ticket for this specific issue.

[ocean90]

Then we have to support oEmbeds too.

[adamsilverstein]

Replying to a.hoereth:

I get your point and also think that the wp_kses_post is more useful from an user point of view.
Still to notice: a new revision is saved when editing the image source code. You won't see any difference between the revisions using wp_kses_post.

What about shortcodes in revision view? [caption] for example.

i tried 24254-2.patch​ but it just let the image get as wide as the html said and i got a big horizontal scroll bar on large images i inserted in my testing; ( see ​http://cl.ly/OjVO )

actually, i don't think we can really safely resize the images might have unintended side effects, plus what if a user inserts several small images in a row?

what about setting max-width on the containing td's and setting their overflow to hidden or scroll? i think that should enforce a neat layout regardless of content. we should really get away from tables entirely, but thats for another day.

[a.hoereth]

max-width for images to prevent them from breaking the revision table layout

[a.hoereth]

Add horizontal scroll bars for revision td's when they would exceed the 48% width otherwise

[a.hoereth]

Replying to adamsilverstein:

i tried 24254-2.patch​ but it just let the image get as wide as the html said and i got a big horizontal scroll bar on large images i inserted in my testing; ( see ​http://cl.ly/OjVO )

Just updated 24254-2.patch​. Which browser are you using? Firefox requires the newly added table-layout: fixed property for it to work.

actually, i don't think we can really safely resize the images might have unintended side effects, plus what if a user inserts several small images in a row?

Multiple images just extend to a new line. We can't provide a perfect wysiwyg layout here: we have less than the post editors width. I think no one expects that.

what about setting max-width on the containing td's and setting their overflow to hidden or scroll? i think that should enforce a neat layout regardless of content.

24254-3.patch​ adds scroll bars to tds if required. The scrollbar breaks the "this is one text" feeling. I do not like it. I also tried to add a scrollbar to each column on its own.. But this seems to require div instead of a table.

[a.hoereth]

Replying to ocean90:

Then we have to support oEmbeds too.

The visual editor does not display oembeds either if you paste a youtube url in there. [caption] actually is displayed there visually. I don't know why WordPress/tinyMCE uses a "semi-shortcode" instead of html (like it normally does) here.

[adamsilverstein]

Replying to a.hoereth:

Replying to ocean90:

Then we have to support oEmbeds too.

The visual editor does not display oembeds either if you paste a youtube url in there. [caption] actually is displayed there visually. I don't know why WordPress/tinyMCE uses a "semi-shortcode" instead of html (like it normally does) here.

[caption] and [galllery] are the only two shortcodes i can think of that display differently in the visual editor, am i missing any? if the html is unrevised it would be nice to give these the same visual treatment as the editor.

[adamsilverstein]

Replying to a.hoereth:

Replying to adamsilverstein:

i tried 24254-2.patch​ but it just let the image get as wide as the html said and i got a big horizontal scroll bar on large images i inserted in my testing; ( see ​http://cl.ly/OjVO )

Just updated 24254-2.patch​. Which browser are you using? Firefox requires the newly added table-layout: fixed property for it to work.

actually, i don't think we can really safely resize the images might have unintended side effects, plus what if a user inserts several small images in a row?

Multiple images just extend to a new line. We can't provide a perfect wysiwyg layout here: we have less than the post editors width. I think no one expects that.

what about setting max-width on the containing td's and setting their overflow to hidden or scroll? i think that should enforce a neat layout regardless of content.

24254-3.patch​ adds scroll bars to tds if required. The scrollbar breaks the "this is one text" feeling. I do not like it. I also tried to add a scrollbar to each column on its own.. But this seems to require div instead of a table.

mostly testing in firefox, latest, osx; will test your latest patches, thanks!

[adamsilverstein]

rollup patches

[adamsilverstein]

Replying to adamsilverstein:

Replying to a.hoereth:

Replying to adamsilverstein:

i tried 24254-2.patch​ but it just let the image get as wide as the html said and i got a big horizontal scroll bar on large images i inserted in my testing; ( see ​http://cl.ly/OjVO )

Just updated 24254-2.patch​. Which browser are you using? Firefox requires the newly added table-layout: fixed property for it to work.

actually, i don't think we can really safely resize the images might have unintended side effects, plus what if a user inserts several small images in a row?

Multiple images just extend to a new line. We can't provide a perfect wysiwyg layout here: we have less than the post editors width. I think no one expects that.

what about setting max-width on the containing td's and setting their overflow to hidden or scroll? i think that should enforce a neat layout regardless of content.

24254-3.patch​ adds scroll bars to tds if required. The scrollbar breaks the "this is one text" feeling. I do not like it. I also tried to add a scrollbar to each column on its own.. But this seems to require div instead of a table.

mostly testing in firefox, latest, osx; will test your latest patches, thanks!

this works perfect for me now, great!

24254.diff​ is a rollup combining your two patches, (sorry about the patch name i thought we were using trac auto-naming, but had dashes instead)

[a.hoereth]

If images stick to max-width: 100%, wherefore do we need overflow: auto? Just in case?

Just created #24263 for [caption] and [gallery] and will look into a possible fix tomorrow. Added you as CC, needs some discussion about the gallery-shortcode.

[adamsilverstein]

Replying to a.hoereth:

If images stick to max-width: 100%, wherefore do we need overflow: auto? Just in case?

Just created a #24263 for [caption] and [gallery] and will look into a possible fix tomorrow. Added you as CC, needs some discussion about the gallery-shortcode.

i'm not sure we need the overflow:auto either, i will have to test; i do know that when i tested your 1st patch i was still getting the horizontal scroll bars; i will do some more testing and post back here.

[markjaquith]

I think this partial rendering business is a huge can of worms, and that we should go back to a simple diff for now, until such time as we can fully investigate the implications of doing full/partial rendering.

[markjaquith]

In 24192:

Go back to plain text diffs between revisions instead of attempting partial rendering.

fixes #24254

[adamsilverstein]

Replying to markjaquith:

In 24192:

Go back to plain text diffs between revisions instead of attempting partial rendering.

fixes #24254

:(