Make WordPress Core

Opened 12 years ago

Closed 11 years ago

#22712 closed defect (bug) (fixed)

Admin using deprecated valign attribute

Reported by: josh401's profile josh401 Owned by: azaozz's profile azaozz
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.4.2
Component: Template Keywords: has-patch
Focuses: Cc:

Description

Hi,

I noticed when I validated one of my plugins' admin pages, I received approx. 40 error messages regarding the use of "v-align:top;" within the css structure. (Please see attached screenshot)

Is there a way to use a different css to target these elements instead of the deprecated v-align usage?

Thanks!

Attachments (7)

trac1.png (80.2 KB) - added by josh401 12 years ago.
Screenshot of deprecated v-align validation errors
trac2-validation-error-valign.png (76.7 KB) - added by josh401 12 years ago.
validation error 'valign'
22712.diff (539 bytes) - added by MikeHansenMe 12 years ago.
remove valign="top" from do_settings_fields called by do_settings_sections
trac3-valign-fixed.png (74.9 KB) - added by josh401 12 years ago.
valign fixed validation report
22712.markup.diff (42.5 KB) - added by MikeHansenMe 12 years ago.
this removes all the markup pages need to be checked to see if any additional css is needed
22712 - markup and styles.diff (274.3 KB) - added by Marventus 12 years ago.
22712-class-wp-posts-list-table.php.diff (591 bytes) - added by neoxx 11 years ago.

Download all attachments as: .zip

Change History (39)

@josh401
12 years ago

Screenshot of deprecated v-align validation errors

#1 @SergeyBiryukov
12 years ago

  • Component changed from Warnings/Notices to Validation

#2 @josh401
12 years ago

Sorry, just wanted to clarify (can't seem to find an edit button).

The initial usage resulting in the notices is v-align="top", displayed in the HTML structure of the rendered page.

#3 @MikeHansenMe
12 years ago

  • Cc mdhansen@… added

@josh401 did you call a function in your plugin that produced this invalid code? If so what one. This code can be found throughout the admin core code. Just to clarify because I first thought this was a problem with your plugin based on the screenshot.

Also related: #16135

Last edited 12 years ago by SergeyBiryukov (previous) (diff)

#4 follow-up: @josh401
12 years ago

@MikeHansenMe,

I don't think so. I'm not very fond of tables... and I think I'd remember if I used that particular invalid HTML. I just went back through my admin panel code... and no traces of any usage of v-align="top".

I'll try and find another plugin which is using the same code as me for displaying admin panel options... and see if the same thing happens there.

I don't think it's my plugin. I think it's how the core code produces the tables when using the callback functions I use. But, I could be wrong.. (and would hate to be). So, I'll seek out another plugin which behaves the same... and report back.

Last edited 12 years ago by josh401 (previous) (diff)

#5 in reply to: ↑ 4 ; follow-up: @SergeyBiryukov
12 years ago

Replying to josh401:

I just went back through my admin panel code... and no traces of any usage of v-align="top".

Note that it's valign, not v-align.

Replying to MikeHansenMe:

This code can be found throughout the admin core code.

Core indeed uses <tr valign="top"> in 12 files. In HTML5, the valign attribute on the tr element is obsolete. W3C recommends to use CSS instead: http://www.w3.org/TR/html-markup/tr.html

#6 @josh401
12 years ago

@SergeyBiryukov,

Thanks for the clarification. Just checked.. no traces of valign either.

Searching for another plugin to replicate now...

#7 in reply to: ↑ 5 @bpetty
12 years ago

  • Cc bpetty added
  • Milestone changed from Awaiting Review to Future Release

Replying to SergeyBiryukov:

Core indeed uses <tr valign="top"> in 12 files. In HTML5, the valign attribute on the tr element is obsolete. W3C recommends to use CSS instead: http://www.w3.org/TR/html-markup/tr.html

It's actually also used with table cells (td) and table headers (th), so I see 96 matches across 18 PHP files, as well as in the wp-includes/js/tinymce/plugins/wpeditimage/editimage.html template file.

Last edited 12 years ago by bpetty (previous) (diff)

#8 @josh401
12 years ago

Here is another plugin which doesn't seem to have the valign in the source code of the plugin. This is "scripts and styles".
http://wordpress.org/extend/plugins/scripts-n-styles/

(Screenshot attached) (trac2-validation-error-valign.png)

@josh401
12 years ago

validation error 'valign'

@MikeHansenMe
12 years ago

remove valign="top" from do_settings_fields called by do_settings_sections

#9 @MikeHansenMe
12 years ago

  • Keywords has-patch added

That patch should solve the problem but as mentioned above there are about 95 other instances throughout core that also need to be fixed.

#10 @josh401
12 years ago

@MikeHansenMe,

Thanks. Will test now and post back a new screenshot.

#11 @josh401
12 years ago

It was line 1149 for me? Not sure why? Did you use WP 3.5? This screenshot is using WP 3.4.2.

Brilliant!! It worked. All I'm left with are these 'nonce' errors (Screenshot attached). Should I begin a new trac ticket... or am I doing something wrong?

Thank you for your immediate attention and solution!

@josh401
12 years ago

valign fixed validation report

#12 @bpetty
12 years ago

  • Summary changed from Plugins admin pages options using v-align top to Admin using deprecated valign attribute

It would be best if each of these types of issues (specifically referring to validation errors) were handled individually on new tickets.

#13 @neoxx
12 years ago

  • Cc mail@… added

In addition to the current patch I'd suggest we also remove the deprecated code for all occurrences of valign="top" in wp-admin:

./wp-admin/comment.php (in this file the usage is inconsistent)
./wp-admin/custom-background.php
./wp-admin/custom-header.php
./wp-admin/edit-form-comment.php
./wp-admin/edit-tag-form.php
./wp-admin/includes/class-wp-media-list-table.php
./wp-admin/includes/class-wp-ms-sites-list-table.php
./wp-admin/includes/class-wp-posts-list-table.php
./wp-admin/includes/file.php
./wp-admin/includes/media.php
./wp-admin/includes/template.php
./wp-admin/my-sites.php
./wp-admin/network/settings.php
./wp-admin/options-discussion.php
./wp-admin/options-general.php
./wp-admin/options-media.php
./wp-admin/options-privacy.php
./wp-admin/options-reading.php
./wp-admin/options-writing.php

#14 @bpetty
12 years ago

  • Keywords needs-patch added; has-patch removed

Right, this does need an extensive patch removing all occurrences of the valign attribute and adding the equivalent CSS styles to those same locations.

#15 follow-up: @josh401
12 years ago

I have no experience with core development... but, I'm fairly educated with code and plugin development. If I can help in any way whatsoever, please let me know.

Thank you.

#16 in reply to: ↑ 15 @azaozz
12 years ago

Looking at the places where valign is used, seems all are left over from long ago. Also about half are overwritten with css already.

Replying to josh401:

I have no experience with core development...

All that's needed here is to remove the attributes and then check the affected screens and add a little of css to keep the table cells aligned to the top (most valign attributes are on <tr>).

Thinking such patch would be a good way to "get your feet wet" :)

#17 follow-up: @josh401
12 years ago

Thinking such patch would be a good way to "get your feet wet" :)

Thanks Andrew. Do you have any links you can point me to on how to get started?

#18 in reply to: ↑ 17 @SergeyBiryukov
12 years ago

Replying to josh401:

Do you have any links you can point me to on how to get started?

Take a look at http://make.wordpress.org/core/handbook/submitting-a-patch/.

#19 @josh401
12 years ago

Thanks, Sergey.

Please allow me 24 to 48 hours to familiarize myself with the process, and I'll be back with a patch.

@MikeHansenMe
12 years ago

this removes all the markup pages need to be checked to see if any additional css is needed

#20 follow-up: @Marventus
12 years ago

  • Keywords has-patch added; needs-patch removed

Hello. I am working with Josh in the Ultimate Tinymce plugin project.
Here's a diff that takes care of the styles issue. Basically, I replaced all instances of valign ("top", "middle", and "bottom" -- no "baseline" values used) with corresponding css classes ("aligntop", "alignmiddle", "alignbottom") and added these classes to the wp-admin/css/admin.css and wp-admin/css/admin.min.css files. I think this should resolve the problem but additional testing would be great.
Cheers!

#21 in reply to: ↑ 20 @SergeyBiryukov
12 years ago

Replying to Marventus:

There's no need to patch minified files, a post-commit bot takes care of that.

#22 @josh401
12 years ago

Thank you, everyone! @Marventus... thanks for the help! @MikeHansenMe.. thank you very much! I thoroughly love and appreciate everything here. It is so nice to work with professionals!

#23 @MikeHansenMe
12 years ago

After taking a quick check around the admin, I could not see anywhere that the alignment was off. It may be worth going through more to determine if we need to add aligntop, alignmiddle, or alignbottom classes or if it is adding unnecessary code.

#24 @Marventus
12 years ago

@Josh, no pb!
@Mike, I noticed the exact same thing, but to be honest, I was afraid of just not leaving anything in case there's something I was missing. That's why I submitted another diff (I hope you don't mind, ;-) ).

Last edited 12 years ago by Marventus (previous) (diff)

#25 @nacin
11 years ago

  • Component changed from Validation to Template

#26 @josh401
11 years ago

Great to see this get some attention. I was JUST NOW working in the admin and noticed this was still prevalent.

Thanks Andrew!

#27 @azaozz
11 years ago

Looking at this again: most of the valign="top" are in table.widefat. There is already

.widefat td {
    vertical-align: top;
}

that covers almost all cases, and all of the th have vertical-align set.

Don't think we need to add new alignment classes. Better to remove all valign and if something goes out of vertical alignment, can target it directly in css.

Version 0, edited 11 years ago by azaozz (next)

#28 @josh401
11 years ago

Better to remove all valign and if something goes out of vertical alignment, can target it directly in css.

Absolutely agreed. The table.form-table is where the majority of the ones I see exist. I use it pretty consistently, and have never had a need to vertically align anything in that <tr> element. If I did, I'd do as you said with the CSS. I certainly wouldn't mind to see it go :)

#29 @azaozz
11 years ago

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

In 27029:

Remove all "valign" attributes from tables in wp-admin, props MikeHansenMe, Marventus. Fixes #22712.

#30 @azaozz
11 years ago

  • Milestone changed from Future Release to 3.9

#31 @neoxx
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I think you've forgotten one occurrence. - Patch (22712-class-wp-posts-list-table.php.diff) is attached.

#32 @azaozz
11 years ago

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

In 27037:

Remove one more valign, props neoxx, fixes #22712.

Note: See TracTickets for help on using tickets.