Skip to content
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.

Sign up for GitHub

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

Jump to bottom

HTML validation errors #280

Closed
brokul-dev opened this issue Feb 8, 2021 · 8 comments · Fixed by #285
Closed

HTML validation errors #280

brokul-dev opened this issue Feb 8, 2021 · 8 comments · Fixed by #285

Comments

@brokul-dev
Copy link
Contributor

Hi!
I've noticed that there are some html validation errors related to this plugin. Am I doing something wrong or this is how it should work?

image

This is html structure of "code blocks":

image
@brokul-dev
Copy link
Contributor Author

After short research changing this div to span would fix at least problems with div inside pre tag (plus changing div to span in $end_tags - line 485).

https://github.com/westonruter/syntax-highlighting-code-block/blob/develop/syntax-highlighting-code-block.php#L516

There still would be a problem with style block. That should be much higher I guess.

@westonruter
Copy link
Owner

In regards to the style element position, I believe the validator is outdated here. Consider this example:

<!DOCTYPE html>
<html lang=en>
<head>
  <title>Test</title>
</head>
<body>
  <style>body { color:green }</style>
</body>
</html>

This also is marked as an error:

validator w3 org_nu_

However, this is not correct. See the HTML spec where it says style is allowed in the body:

Screenshot 2021-02-08 08 41 31

@westonruter
Copy link
Owner

Changing the div to a span on the other hand:

syntax-highlighting-code-block/syntax-highlighting-code-block.php

Line 516 in 91ae9d7

return $pre_start_tag . get_styles( $attributes ) . '<div>' . $code_start_tag . $content . $end_tags;

That should be doable, even though it's not strictly necessary, as browsers render it just fine. Doing so would also involve changing this CSS:

syntax-highlighting-code-block/style.css

Lines 6 to 8 in 91ae9d7

.wp-block-code > div {
overflow: auto;
}

To something like this:

.wp-block-code > span {
	overflow: auto;
	display: block;
}

@brokul-dev
Copy link
Contributor Author

brokul-dev commented Feb 9, 2021 •
edited
Loading

About style tag:
I think it's up to where you look. I've read that currently the most valid and authoritative HTML doc is one that belongs to WHATWG (read more: here and here).

Here is WHATWG spec about style tag. There is no info about putting style into body. That's why W3C Validator complains about it. It follows WHATWG rules under the hood. So I would say that the only valid place for style tag is head.

About div inside pre:
Validation here works fine: pre tag expects "phrasing content" as children, div is flow content (see more). After changing div to span even without changing its display to block everything looks fine. Did I miss something?

To sum up, of course, you're right. Most browsers should ignore these cases and render everything as expected. But I would follow official recommendations and stick to standards. For example putting style to head is the most acceptable choice, no validator will complain. Question is if it's possible to achieve with current approach?

@westonruter
Copy link
Owner

About style tag:

Strange that the W3C Validator is following the WHATWG version of the spec, and not the W3C's own version. 🤷

See also the whatwg/html#1605 for the discussion about adding style to body. It was closed essentially with a “maybe later” status: whatwg/html#1605 (comment).

The reality is that WordPress core outputs styles in the body. For example, in Twenty Twenty-One: https://github.com/WordPress/wordpress-develop/blob/93db55f9beee344f7d8c1e56b3122a6abff88443/src/wp-content/themes/twentytwentyone/classes/class-twenty-twenty-one-dark-mode.php#L350-L373

The WHATWG spec doesn't allow link[rel=stylesheet] in the body either, but WordPress does this all the time. The Trac ticket was closed per https://core.trac.wordpress.org/ticket/30579#comment:23

Whenever a stylesheet is enqueued after wp_head during page rendering and inline styles are added, they'll be also added in the body via print_late_styles().

In short, there's no way reliable move the style elements to the head for blocks rendered in the body. While it could be possible for blocks that are in the main query, it wouldn't be done for blocks in subqueries. The only way to do so reliably would be to output-buffer the entire page and then move the style elements to the head, but this introduces a lot of overhead.

About div inside pre:
Validation here works fine: pre tag expects "phrasing content" as children, div is flow content (see more). After changing div to span even without changing its display to block everything looks fine. Did I miss something?

The overflow property requires display:block, as I understand.

@brokul-dev
Copy link
Contributor Author

Fair enough, it seems to be overkill to implement something like this.

Coming back to div inside pre. I've just checked and it seems to work fine without div/span inside pre at all. Default overflow also works ok. Or to be sure it would be worth to add overflow to code element (it already has display: block). What do you think about this?

image

@allejo
Copy link
Collaborator

allejo commented Feb 9, 2021

Coming back to div inside pre. I've just checked and it seems to work fine without div/span inside pre at all. Default overflow also works ok. Or to be sure it would be worth to add overflow to code element (it already has display: block). What do you think about this?

@brokul-dev See the reasoning behind the extra <div> here: #80 (comment). However, if we can use a span with display: block that'll be fine as well.

@brokul-dev
Copy link
Contributor Author

I've created a pull request for that: #285

@westonruter westonruter mentioned this issue Feb 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Html error fix (div changed to span) brokul-dev/syntax-highlighting-code-block
3 participants
@westonruter @allejo @brokul-dev