#47580 closed defect (bug) (fixed)
Prevent output of invalid HTML in `_default_wp_die_handler()`
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Administration | Keywords: | has-patch has-dev-note |
Focuses: | accessibility | Cc: |
Description
The default handler for wp_die()
wraps (by default) error messages with a paragraph tag.
For a large number of wp_die()
calls in WordPress, a plain text string is passed and the HTML displayed is valid: <p>This is an error message.</p>
.
Also for a large number of wp_die()
calls in WordPress, an HTML string is passed and the HTML displayed is invalid: <p><h1>You need a higher level of permission.</h1><p>Sorry, you are not allowed to manage terms in this taxonomy.</p></p>
It seems that both use cases are valid and should be supported. wp_die()
already has $args
that are passed to the handlers. It seems like we could add key there for whether the handler should wrap in its own HTML or display the message as is.
Supporting a new argument would make it possible to have a slightly more complex error message while also maintaining the simple output of general error messages.
Related #47413, which addresses the issue when filling out a comment.
Attachments (2)
Change History (23)
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
6 years ago
#2
@
6 years ago
- Component changed from General to Administration
- Milestone changed from Awaiting Review to Future Release
#3
in reply to:
↑ description
@
6 years ago
Replying to jeremyfelt:
wp_die()
already has$args
that are passed to the handlers. It seems like we could add key there for whether the handler should wrap in its own HTML or display the message as is.
A key in $args
might be cleaner, but also seems a bit like an overkill. Maybe a simple strpos( $message, '<p>' )
check before adding the <p>
tag would suffice here?
Another option suggested by @audrasjb is to use <div>
instead of <p>
to allow for nested elements.
#4
@
6 years ago
One more example of invalid HTML:
https://core.trac.wordpress.org/browser/trunk/src/index.php?rev=45649&marks=33-55#L32
The passed string avoids to set the initial opening tag <p>
, which is smart, but then forgets to not pass the last closing tag </p>
. Actual result in the HTML: </p></p>
.
#6
@
6 years ago
- Keywords has-patch dev-feedback added; needs-patch removed
After considering checking for tags in the $message
and replacing p
by div
, I think it's better to use div
for the die message wrapper.
We can check if $message
contains <p>
, but it means that we need to check for other tags as well. h1
for example, can't be a child element of p
. A full covered check can make the current function more complex.
On the other hand, replacing p
by div
doesn't increase the complexity of _default_wp_die_handler
.
What I did:
- Use
div
as$message
wrapper in the function_default_wp_die_handler
. The error message now is always wrapped bydiv.wp-die-message
. - Update CSS rules in
common.css
and internal CSS for error page to keep the current die message unchanged in term of visual. - Update this to match with the new wrapper: https://core.trac.wordpress.org/browser/trunk/src/index.php?rev=45649&marks=33-55#L32
This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.
6 years ago
#8
@
6 years ago
- Keywords needs-testing added
- Owner set to audrasjb
- Status changed from new to accepted
#9
@
6 years ago
- Keywords dev-feedback needs-testing removed
Good to go on my side. As said during the meeting, I like the <div>
approach because it‘s more secure concerning backward compatibility :)
#11
follow-up:
↓ 16
@
5 years ago
- Keywords needs-refresh added
@dinhtungdu thanks for the patch, but after a full review, I think we should separate p
and .wp-die-message
in two CSS rules (even if they have the same CSS declarations) in src/wp-admin/css/common.css
.
#12
@
5 years ago
- Keywords dev-feedback added; needs-refresh removed
Thank @audrasjb, I updated the patch with your comment :)
This ticket was mentioned in Slack in #core by pento. View the logs.
5 years ago
#16
in reply to:
↑ 11
;
follow-up:
↓ 17
@
5 years ago
Replying to audrasjb:
a full review, I think we should separate
p
and.wp-die-message
in two CSS rules (even if they have the same CSS declarations) insrc/wp-admin/css/common.css
.
Just curious, what was the reason for separating the rules? It seems that would make it harder to keep them consistent in the future.
#17
in reply to:
↑ 16
;
follow-up:
↓ 18
@
5 years ago
Replying to SergeyBiryukov:
Replying to audrasjb:
a full review, I think we should separate
p
and.wp-die-message
in two CSS rules (even if they have the same CSS declarations) insrc/wp-admin/css/common.css
.
Just curious, what was the reason for separating the rules? It seems that would make it harder to keep them consistent in the future.
IMO, we don't need to keep them consistent in the future, they are two separated elements. We have their CSS rules identical for now because I want to keep the style as it was. In the future, it's more likely that we'll change the style for .wp-die-message
.
#18
in reply to:
↑ 17
;
follow-up:
↓ 19
@
5 years ago
Replying to dinhtungdu:
IMO, we don't need to keep them consistent in the future, they are two separated elements. We have their CSS rules identical for now because I want to keep the style as it was. In the future, it's more likely that we'll change the style for
.wp-die-message
.
It just seems inconsistent to me that they're combined in _default_wp_die_handler() styles, but separated in common.css
. I would think it's better to reduce duplication for now and separate them in the future if needed.
#19
in reply to:
↑ 18
@
5 years ago
Replying to SergeyBiryukov:
It just seems inconsistent to me that they're combined in _default_wp_die_handler() styles, but separated in
common.css
. I would think it's better to reduce duplication for now and separate them in the future if needed.
Yeah, it makes sense now :). What do you think @audrasjb ?
Discussed during today's accessibility bug-scrub and agreed ensuring valid HTML output is always valuable. Moving to Future Release.