Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#41596 closed defect (bug) (fixed)

New Text Widget recognizes HTML but does not render it in the front end

Reported by: mrfoxtalbot's profile mrfoxtalbot Owned by: westonruter's profile westonruter
Milestone: 4.8.2 Priority: normal
Severity: normal Version: 4.8
Component: Widgets Keywords: has-patch has-unit-tests commit fixed-major
Focuses: ui Cc:

Description

If you paste HTML code into the visual tab on the new text widget and press "save" it will automatically recognize it and render it properly inside the widget (including any inline CSS styles). The error comes when you go to the front end, where it will show the original HTML code instead of interpreting it.

Here is a short video showing the error:
https://youtu.be/KsCYbX5LSzM

I first noticed this happening on wp.com (that´s where I made the video) but I was able to reproduce later it on a clean 4.8.1 installation (no plugins and using TwentySeventeen)

Attachments (1)

41596.1.diff (8.1 KB) - added by westonruter 7 years ago.
https://github.com/xwp/wordpress-develop/pull/247

Download all attachments as: .zip

Change History (15)

#1 @joyously
7 years ago

@westonruter

#2 follow-up: @westonruter
7 years ago

@mrfoxtalbot interesting. Did you get the admin pointer when you pasted the HTML the first time?

See the pointer here: https://make.wordpress.org/core/2017/08/01/fixes-to-text-widget-and-introduction-of-custom-html-widget-in-4-8-1/

The pointer won't appear anymore since dismissing.

It could be a nice enhancement to TinyMCE to convert pasted HTML into rendered HTML actually as well.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


7 years ago

#4 @westonruter
7 years ago

@azaozz why would TinyMCE automatically convert pasted raw HTML code into rendered HTML upon save?

#5 @azaozz
7 years ago

This doesn't look like TinyMCE's doing. The "normal" behavior is to keep it encoded just like in the main editor. This is also in all contenteditable areas. Probably some of the "fixes" for keeping the bad/old html hacks working interferes?

#6 follow-up: @westonruter
7 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.8.2
  • Version changed from 4.8.1 to 4.8

It actually turns out to be due to esc_attr() and how it does not double-encode/double-escape any entities. So if you enter &lt;code&gt; into the editor then it will get saved in the DB as that but then the text value will get written into the text hidden input as value="&lt;code&gt;" and thus get read out as <code>.

@mrfoxtalbot So actually, you'll see that if you modify the widget after that initial save and then save again, you'll then notice on the frontend that it then renders the same as in the widget.

So I've got a proposed change that will allow esc_attr() to force-allow double-escaping of entities, and that fixes the problem in my testing: https://github.com/xwp/wordpress-develop/pull/247

Maybe it would be better to just use htmlspecialchars() directly instead of esc_attr().

@azaozz thoughts?

#7 in reply to: ↑ 6 @azaozz
7 years ago

Replying to westonruter:

Nice catch! Wondering if we should be using format_for_editor like in the main editor, or directly do htmlspecialchars( $text, ENT_NOQUOTES, get_option( 'blog_charset' ) );, see: https://core.trac.wordpress.org/browser/tags/4.8/src/wp-includes/formatting.php#L3691. I prefer using the filter but that may bring some ...problems with plugins that expect it to only run for the main editor perhaps?

We also need to make sure there is no </textarea string in the content, see: https://core.trac.wordpress.org/browser/tags/4.8/src/wp-includes/class-wp-editor.php#L289.

#8 in reply to: ↑ 2 @mrfoxtalbot
7 years ago

Yep, the admin pointer did come up fine at first.

Replying to westonruter:

@mrfoxtalbot interesting. Did you get the admin pointer when you pasted the HTML the first time?

See the pointer here: https://make.wordpress.org/core/2017/08/01/fixes-to-text-widget-and-introduction-of-custom-html-widget-in-4-8-1/

The pointer won't appear anymore since dismissing.

It could be a nice enhancement to TinyMCE to convert pasted HTML into rendered HTML actually as well.

#9 follow-up: @westonruter
7 years ago

  • Keywords needs-testing has-unit-tests added
  • Owner set to azaozz
  • Status changed from new to reviewing

@azaozz thanks for that. I wasn't aware of format_for_editor(). The best option to get around the double-encoding issue seems then to get away from using a input[type=hidden] and instead utilize a textarea[hidden]. Please review 41596.1.diff.

#10 in reply to: ↑ 9 @azaozz
7 years ago

  • Keywords commit added; needs-testing removed

Replying to westonruter:

41596.1.diff looks good and works properly here.

#11 @westonruter
7 years ago

  • Owner changed from azaozz to westonruter
  • Status changed from reviewing to accepted

#12 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 41260:

Widgets: Prevent visual Text widget from decoding encoded HTML.

Also apply the_editor_content filters on widget text with format_for_editor() as is done for the post editor.

Amends [40631].
Props westonruter, azaozz.
See #35243.
Fixes #41596.

#13 @westonruter
7 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for 4.8.2

#14 @ocean90
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 41392:

Widgets: Prevent visual Text widget from decoding encoded HTML.

Also apply the_editor_content filters on widget text with format_for_editor() as is done for the post editor.

Merge of [41260] to the 4.8 branch.

Amends [40631].
Props westonruter, azaozz.
See #35243.
Fixes #41596.

Note: See TracTickets for help on using tickets.