WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 12 months ago

Last modified 7 months ago

#18788 closed enhancement (fixed)

Remove redundant type attributes from script and style tags

Reported by: solarissmoke Owned by: wonderboymusic
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.3
Component: Script Loader Keywords: 3.4-early has-patch
Focuses: administration Cc:

Description

Now that the admin is using the HTML5 doctype everywhere, the type="text/javascript" and type="text/css" attributes on script and style tags are unnecessary (if they weren't anyway), and I think they can be safely removed to trim a few hundred bytes from core. Should be a simple search and replace exercise.

Attachments (2)

no-cdata.diff (10.2 KB) - added by tw2113 5 years ago.
removed script CDATA tags. Left script types on though.
no-cdata1.diff (12.2 KB) - added by tw2113 5 years ago.
forgot wp-includes

Download all attachments as: .zip

Change History (29)

#1 @azaozz
5 years ago

  • Keywords 3.4 added
  • Milestone changed from Awaiting Review to Future Release

We have to confirm that first for older mobile browsers. They still might be needed for some. Also the //<![CDATA[ bits inside of <script> are not needed.

#2 @tw2113
5 years ago

  • Cc michael.d.beckwith@… added

@tw2113
5 years ago

removed script CDATA tags. Left script types on though.

@tw2113
5 years ago

forgot wp-includes

#3 @tw2113
5 years ago

  • Keywords has-patch added

I did my best to stick to just <script></script> blocks. I didn't edit any of them in RSS/XML based feed files.

#4 @retlehs
4 years ago

  • Cc retlehs added

#5 @Volker_E.
4 years ago

  • Cc Volker_E. added

#6 @SergeyBiryukov
3 years ago

  • Keywords 3.4 removed

HTML comment tags in wp-admin/includes/file.php can also be removed:
http://core.trac.wordpress.org/browser/tags/3.5.1/wp-admin/includes/file.php#L969

#7 @SergeyBiryukov
3 years ago

  • Keywords 3.4-early added

Fixing the keyword for reference.

#9 @nacin
3 years ago

  • Component changed from Administration to Script Loader
  • Focuses administration added

#10 @wonderboymusic
21 months ago

  • Milestone changed from Future Release to 4.2

#11 follow-up: @wonderboymusic
21 months ago

In 31034:

Inline <script>s that are only printed in the admin for pages that are served with the HTML5 doctype absolutely do not need CDATA comments.

Props tw2113 for the initial patch.
See #18788.

#12 follow-up: @wonderboymusic
21 months ago

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

type is optional in HTML5, not required. I don't think it hurts to keep it.

#13 in reply to: ↑ 12 ; follow-up: @retlehs
21 months ago

Replying to wonderboymusic:

type is optional in HTML5, not required. I don't think it hurts to keep it.

Since it's optional, why keep it?

#14 in reply to: ↑ 13 ; follow-up: @AramZS
20 months ago

Replying to retlehs:

Replying to wonderboymusic:

type is optional in HTML5, not required. I don't think it hurts to keep it.

Since it's optional, why keep it?

Always better to be more specific than less, right?

#15 in reply to: ↑ 14 @solarissmoke
20 months ago

Replying to AramZS:

Always better to be more specific than less, right?

The HTML5 specification is explicitly clear about what a missing type attribute means, so specificity isn't an issue. Removing it means less bloat and easier code to read.

#16 follow-up: @retlehs
20 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening because type still isn't removed.

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


19 months ago

#18 @DrewAPicture
19 months ago

  • Keywords 4.2-beta added

@wonderboymusic: Any idea what's left here? Letting this ride into Beta 1, see [31034].

#19 in reply to: ↑ 16 ; follow-up: @helen
19 months ago

Replying to retlehs:

Reopening because type still isn't removed.

Curious if anybody tried checking into that on older browsers, per the first comment from azaozz.

#20 in reply to: ↑ 19 @retlehs
19 months ago

Replying to helen:

Curious if anybody tried checking into that on older browsers, per the first comment from azaozz.

Is there a list of supported browsers that need to be tested? It's fine in IE8. It's probably fine in IE7, but I don't have a VM with it up and running yet.

(Not sure if any of this even matters considering a HTML5 doctype is used)

Last edited 19 months ago by retlehs (previous) (diff)

#21 @DrewAPicture
19 months ago

  • Owner set to wonderboymusic
  • Status changed from reopened to assigned

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


19 months ago

#23 @DrewAPicture
19 months ago

  • Keywords 4.2-beta removed
  • Milestone changed from 4.2 to Future Release

Per consensus in the above-linked Slack discussion, punting this to a future release.

#24 @wonderboymusic
12 months ago

  • Milestone changed from Future Release to 4.2
  • Resolution set to fixed
  • Status changed from assigned to closed

#25 @sephr
7 months ago

This change breaks XHTML support in many areas. CDATA sections are not redundant for people emitting strict XHTML5 served as application/xhtml+xml, like myself.

I've been emitting an XHTML5 theme ever since 2009, so I depend on the continued support of valid syntax.

Last edited 7 months ago by sephr (previous) (diff)

#26 in reply to: ↑ 11 @sephr
7 months ago

Replying to wonderboymusic:

In 31034:

Inline <script>s that are only printed in the admin for pages that are served with the HTML5 doctype absolutely do not need CDATA comments.

Props tw2113 for the initial patch.
See #18788.

CDATA has nothing to do with the HTML5 doctype in the first place... It has to do with content type. Using the HTML5 doctype does not magically force an XHTML document into allowing invalid XHTML syntax.

#27 @sephr
7 months ago

To clarify, all of the wp-admin/ changes are fine. This only broke XHTML support when @solarissmoke added wp-includes/.

wp-includes is not limited to the admin interface, and throughout this issue it seems like the intention was only to remove CDATA in the admin interface.

Issue description: "Now that the admin is..."

@wonderboymusic's comment "Inline <script>s that are only printed in the admin for pages..."

The first patch should have been accepted, but not the second one.

Note: See TracTickets for help on using tickets.