WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#24254 closed defect (bug) (fixed)

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

Reported by: a.hoereth Owned by: markjaquith
Milestone: 3.6 Priority: high
Severity: normal Version: 3.6
Component: Revisions Keywords: has-patch
Focuses: Cc:

Description

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?

Attachments (8)

revisions-big-broken-img.png (189.7 KB) - added by a.hoereth 12 months ago.
revisions-big-img-likeitshouldbe.png (182.9 KB) - added by a.hoereth 12 months ago.
24254.patch (1.1 KB) - added by a.hoereth 12 months ago.
Exchanges wp_kses_post with esc_html
24254-2.patch (475 bytes) - added by a.hoereth 12 months ago.
max-width for images to prevent them from breaking the revision table layout
24254-3.patch (463 bytes) - added by a.hoereth 12 months ago.
Add horizontal scroll bars for revision td's when they would exceed the 48% width otherwise
24254.diff (575 bytes) - added by adamsilverstein 12 months ago.
rollup patches
24254.2.diff (1.9 KB) - added by markjaquith 12 months ago.
24254.3.diff (2.0 KB) - added by markjaquith 12 months ago.

Download all attachments as: .zip

Change History (30)

comment:1 ocean9012 months ago

  • Keywords needs-patch added; dev-feedback removed
  • Milestone changed from Awaiting Review to 3.6
  • Priority changed from normal to high

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.hoereth12 months ago

Exchanges wp_kses_post with esc_html

comment:2 a.hoereth12 months ago

  • Keywords has-patch dev-feedback added; needs-patch removed

wp_kses_post was used before. This leaves images etc intact. Exchanged it with esc_html which now escapes all image html etc. I think 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.

Last edited 12 months ago by a.hoereth (previous) (diff)

comment:3 follow-up: ocean9012 months ago

  • Keywords dev-feedback removed

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

comment:4 in reply to: ↑ 3 adamsilverstein12 months ago

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:

http://f.cl.ly/items/0p0q0827431K3u2d0B09/Screen%20Shot%202013-05-03%20at%208.10.22%20AM.jpg

after patch:

http://f.cl.ly/items/1J3J1m1C3g133e3d0p0B/Screen%20Shot%202013-05-03%20at%208.17.22%20AM.jpg

comment:5 follow-ups: a.hoereth12 months ago

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.

comment:6 follow-up: ocean9012 months ago

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.

comment:7 in reply to: ↑ 6 adamsilverstein12 months ago

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.

comment:8 in reply to: ↑ 5 ; follow-up: adamsilverstein12 months ago

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 -

http://f.cl.ly/items/1i3A2Z1t2A1J2o0q0v2B/Screen%20Shot%202013-05-03%20at%2010.07.22%20AM.jpg

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

captions will show in source code, changes are highlighted

http://f.cl.ly/items/422V2e3a0Y3E2D2s3q1W/Screen%20Shot%202013-05-03%20at%2010.14.10%20AM.jpg

comment:9 in reply to: ↑ 8 ; follow-up: a.hoereth12 months ago

Replying to adamsilverstein:

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

captions will show in source code, changes are highlighted

http://f.cl.ly/items/422V2e3a0Y3E2D2s3q1W/Screen%20Shot%202013-05-03%20at%2010.14.10%20AM.jpg

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.

comment:10 in reply to: ↑ 9 adamsilverstein12 months ago

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

http://f.cl.ly/items/422V2e3a0Y3E2D2s3q1W/Screen%20Shot%202013-05-03%20at%2010.14.10%20AM.jpg

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.

comment:11 follow-up: ocean9012 months ago

Then we have to support oEmbeds too.

comment:12 in reply to: ↑ 5 ; follow-up: adamsilverstein12 months ago

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.hoereth12 months ago

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

a.hoereth12 months ago

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

comment:13 in reply to: ↑ 12 ; follow-up: a.hoereth12 months ago

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.

comment:14 in reply to: ↑ 11 ; follow-up: a.hoereth12 months ago

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.

comment:15 in reply to: ↑ 14 adamsilverstein12 months ago

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.

comment:16 in reply to: ↑ 13 ; follow-up: adamsilverstein12 months ago

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!

adamsilverstein12 months ago

rollup patches

comment:17 in reply to: ↑ 16 adamsilverstein12 months ago

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)

comment:18 follow-up: a.hoereth12 months ago

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.

Version 0, edited 12 months ago by a.hoereth (next)

comment:19 in reply to: ↑ 18 adamsilverstein12 months ago

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.

comment:20 markjaquith12 months ago

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.

markjaquith12 months ago

markjaquith12 months ago

comment:21 follow-up: markjaquith12 months ago

  • Owner set to markjaquith
  • Resolution set to fixed
  • Status changed from new to closed

In 24192:

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

fixes #24254

comment:22 in reply to: ↑ 21 adamsilverstein12 months ago

Replying to markjaquith:

In 24192:

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

fixes #24254

:(

Note: See TracTickets for help on using tickets.