-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTML API: Introduce WP_HTML::tag()
for safely creating HTML.
#5884
base: trunk
Are you sure you want to change the base?
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are just some notes. I'll update the tag list to include any missing tags I found.
@@ -2286,15 +2286,16 @@ public function is_tag_closer() { | |||
* | |||
* For boolean attributes special handling is provided: | |||
* - When `true` is passed as the value, then only the attribute name is added to the tag. | |||
* - When `false` is passed, the attribute gets removed if it existed before. | |||
* - When `false` or `null` is passed, the attribute gets removed if it existed before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes seems worth landing on its own.
/** | ||
* Returns whether a given element is an HTML tag name. | ||
* | ||
* @todo Verify this list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I scraped this following list from MDN and w3.org. It looks like there may be a few more elements in this list, but I notice there are some missing as well. I can push a change to merge the lists.
HTML elements
A
ABBR
ACRONYM
ADDRESS
AREA
ARTICLE
ASIDE
AUDIO
B
BASE
BDI
BDO
BIG
BLOCKQUOTE
BODY
BR
BUTTON
CANVAS
CAPTION
CENTER
CITE
CODE
COL
COLGROUP
COMMAND
CONTENT
DATA
DATALIST
DD
DEL
DETAILS
DFN
DIALOG
DIR
DIV
DL
DT
EM
EMBED
FIELDSET
FIGCAPTION
FIGURE
FONT
FOOTER
FORM
FRAME
FRAMESET
H1
H2
H3
H4
H5
H6
HEAD
HEADER
HGROUP
HR
HTML
I
IFRAME
IMAGE
IMG
INPUT
INS
KBD
KEYGEN
LABEL
LEGEND
LI
LINK
MAIN
MAP
MARK
MARQUEE
MATH
MENU
MENUITEM
META
METER
NAV
NOBR
NOEMBED
NOFRAMES
NOSCRIPT
OBJECT
OL
OPTGROUP
OPTION
OUTPUT
P
PARAM
PICTURE
PLAINTEXT
PORTAL
PRE
PROGRESS
Q
RB
RP
RT
RTC
RUBY
S
SAMP
SCRIPT
SEARCH
SECTION
SELECT
SHADOW
SLOT
SMALL
SOURCE
SPAN
STRIKE
STRONG
STYLE
SUB
SUMMARY
SUP
SVG
TABLE
TBODY
TD
TEMPLATE
TEXTAREA
TFOOT
TH
THEAD
TIME
TITLE
TR
TRACK
TT
U
UL
VAR
VIDEO
WBR
XMP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In deeper inspection here I feel like this is the wrong approach. We should be able to instead determine if we're in foreign content and apply the rules there accordingly. That way we don't have to keep a list of HTML elements updated and we don't have to worry about conflating elements with the same name of HTML elements with foreign elements, e.g. TITLE
inside an SVG.
So I think more important now is getting foreign content detection in place. I've started working on this in #6006.
It may be the case that this relies on the HTML Processor because the rules get complicated with MathML and foriegn content integration points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obviously after exploring #6006 there's no easy way to infer the outside context when creating a tag in isolation. this leads me to think that we need a good way to communicate this to developers.
maybe instead of passing 'self-closing'
we could have people pass 'xml-empty-tag'
or 'empty-tag-in-foreign-content'
. I'd like to communicate that this isn't for IMG
or any HTML tag.
the trickiest part I've come to realize is that we could have an SVG IMG
tag as well, which could adopt the self-closing identity by nature of being inside foreign content, and that would be meaningful.
I don't see this being often used, so I feel comfortable making it a bit awkward. it seems incredibly unlikely to be common that someone is intentionally creating an empty XML element inside foreign content.
/** | ||
* HTML API: WP_HTML class | ||
* | ||
* Provides a public interface for HTML-related functionality in WordPress. | ||
* | ||
* @package WordPress | ||
* @subpackage HTML-API | ||
* @since 6.5.0 | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used WP_HTML_Processor::is_void_tag
a few times and it feels like it should live on a utility class like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it started in a utility class like this! but then we didn't include the utility class for pragmatic reasons.
If I could suggest one thing to put on your list, if it isn't there already: This new method and any future templating methods should be added to the list of auto-escaped functions in the core coding standards. Until then, developers will need to If I have availability to take things up with WPCS when the time comes, I'll be happy to, but I'm not sure that I will, so I thought I'd mention it separately. |
Should also other methods be on that list? Example, |
I'd be grateful for help adding this to where it belongs in the WPCS ruleset. @gziolo that's a good point, but I don't think On this note it would be rather helpful if we could indicate not to escape things to methods like |
Trac ticket: Core-50867
Alternative to #4065
In this patch the
WP_HTML::tag()
method is introduced for safely creating HTML content for a given tag and a number of attributes or inner text. No support is proposed yet for inner tags.In lieu of pushing HTML templating into WordPress 6.5 this limited API provides some of the benefit of templating without raising broader questions about performance, API design, and the like. Templating will cover the needs that this fulfills, but there's a reasonably place for both to exist, so this would not be entirely obviated from the introduction of true templating.