Page MenuHomePhabricator

ProofreadPage should not use obsolete HTML attributes
Closed, ResolvedPublic

Description

SpecialProofreadPages.php uses (at least) valign, cellpadding / cellspacing, border="0", align, but it should use CSS instead.

Similar changes should be made to ProofreadPage.body.php as well.

Details

Related Gerrit Patches:
mediawiki/extensions/ProofreadPage : masterFix style issues in Special:IndexPages
mediawiki/extensions/ProofreadPage : masterMakes Special:IndexPages output valid HTML 5
mediawiki/extensions/ProofreadPage : masterImprove pr_quality CSS

Event Timeline

He7d3r created this task.Nov 30 2014, 11:23 AM
He7d3r raised the priority of this task from to Needs Triage.
He7d3r updated the task description. (Show Details)
He7d3r changed Security from none to None.
He7d3r added a subscriber: He7d3r.
GOIII added a subscriber: GOIII.Nov 30 2014, 10:37 PM

One of my long time headaches is exactly this point. For example -- w3.org validator results always start with a dozen or two warnings that this thing or that thing has been deprecated... blah... blah... blah... cause every transcluded page or pages from the Page: namespace to the main namespace generates a small, color coded, proofreading status bar along with it.

Here's the start of the 'display the proofreading status of transcluded pages' section ...but the crux of the issues begins on Line 570 .

Whatever or however the current pr_staus bar is generated from there, it should render more along the lines of something like..

mock PR status bar
$output = "<table class='pr_quality pr-quality' style='border-collapse:separate; border-spacing:0px 0px; border-width:1px; border-style:dotted; line-height:40%; margin: -0.45em 0.00em 0.00em 0.01em; text-align:center;'>
<tr>
<td class='qualityf' style='padding:0; width:0;'>&#x3000;</td>
<td class='quality4' style='padding:0;' width=\"$q4\"></td>
<td class='quality3' style='padding:0;' width=\"$q3\"></td>
<td class='quality2' style='padding:0;' width=\"$q2\"></td>
<td class='quality1' style='padding:0;' width=\"$q1\"></td>
<td class='quality0' style='padding:0;' width=\"$q0\"></td>
$void_cell
</tr>
</table>";

... and the $void_cell would just be something like....

	"<td class='qualitye' style='padding:0;' width=\"{$qe}\"></td>"

... instead.
I'd sure appreciate any effort to make this change in what amounts to simple inline .css styling in the end.

GOIII added a comment.EditedDec 3 2014, 12:06 AM

Well, ProofreadPage.body.php has been taken care of by https://gerrit.wikimedia.org/r/176639

That just leaves SpecialProofreadPages.php to do as linked in the description.

GOIII added a comment.EditedDec 3 2014, 7:57 AM

Here's what I could come up with for SpecialProofeadPages.php -- also see Wikisource:Sandbox

mock SpecialProofeadPages.php
<ol class="special">
<li><table style="display:inline-table; border-collapse:separate; border-spacing:0px 0px; margin:0 0 0 0;">
<tr>
<td style="white-space:nowrap; overflow:hidden; vertical-align:baseline;">[[Index:Devonshire Characters and Strange Events.djvu|Index:Devonshire Characters and Strange Events.djvu]] ?[970 pages]</td>
<td style="padding-right:0.5em; padding-left:0.5em;">
<table style="display:inline-table; border-collapse:separate; border-spacing:0px 0px; margin:0 0 0 0; height:0.75em; vertical-align:middle;">
<tr>
<td class="qualityf" style="padding:0; width:0px;"></td>
<td class="quality4" style="padding:0; width:770px;"></td>
<td class="quality3" style="padding:0; width:130px;"></td>
<td class="quality2" style="padding:0; width:6px;"></td>
<td class="quality1" style="padding:0; width:24px;"></td>
<td class="quality0" style="padding:0; width:6px;"></td>
<td class="qualitye" style="padding:0; border:1px dotted #B8B8B8; width:24px;"></td>
</tr>
</table>
</td>
</tr>
</table></li>
<li><table style="display:inline-table; border-collapse:separate; border-spacing:0px 0px; margin:0 0 0 0;">
<tr>
<td style="white-space:nowrap; overflow:hidden; vertical-align:baseline;">[[Index:Popular Science Monthly Volume 14.djvu|Index:Popular Science Monthly Volume 14.djvu]] ?[876 pages]</td>
<td style="padding-right:0.5em; padding-left:0.5em;">
<table style="display:inline-table; border-collapse:separate; border-spacing:0px 0px; margin:0 0 0 0; height:0.75em; vertical-align:middle;">
<tr>
<td class="qualityf" style="padding:0; width:0px;"></td>
<td class="quality4" style="padding:0; width:816px;"></td>
<td class="quality3" style="padding:0; width:0px;"></td>
<td class="quality2" style="padding:0; width:0px;"></td>
<td class="quality1" style="padding:0; width:0px;"></td>
<td class="quality0" style="padding:0; width:28px;"></td>
<td class="qualitye" style="padding:0; border:1px dotted #B8B8B8; width:32px;"></td>
</tr>
</table>
</td>
</tr>
</table></li>
</ol>
GOIII added a comment.Dec 4 2014, 4:07 AM

Ehh... the results for the mainspace mini-pr_quality status bar aren't exactly perfect after the merge.
see https://test2.wikipedia.org/s/9gi for example.

A few points to address or modifications needed (imho)...

  1. remove the line-height: and margin: lines from the .pr_quality (table) entry in the .css. Both of these values are usurped by the settings loaded for the containing DIV, #contentSub instead.
  2. Add border-color: to the .pr_quality (table) settings in the .css and set it to #B8B8B8
  3. change the height: value of .pr_quality (td) from 0.2em to 0.3750em or 0.50em. the current value makes the bar too thin to be read properly.
  4. Put the opening & closing <tr> tags on their own lines as well as the closing </table> tag. Not sure if its the test bed or our code, but there is an extra BR being inserted after the table but still inside the containing DIV, #contentSub.
  5. Add class=qualitye to the $void_cell table cell. Makes things consistent and allows for future css manipulation if need be.

The most glaring issue is the lack of rendering of pages transcluded in a range that have not been created. In the linked example to the testbed above, positions 81 to 100 are being called from the Page namespace. 7 of the 20 are red, 2 of the 20 are blue and 2 of the 20 are gray - leaving 9 pages called but not depicted in the status bar (is this the void cell?)

Anyway, something doesn't seem right (not that it was right before) in the way page status of the entire range being called are being depicted. Seems like percentages of a whole rather fixed widths in px should be the values for width:

Change 177498 had a related patch set uploaded (by Tpt):
Improve pr_quality CSS

https://gerrit.wikimedia.org/r/177498

Patch-For-Review

Tpt added a subscriber: Tpt.Dec 4 2014, 7:48 AM

A few points to address or modifications needed (imho)...

  1. remove the line-height: and margin: lines from the .pr_quality (table) entry in the .css. Both of these values are usurped by the settings loaded for the containing DIV, #contentSub instead.

Done

  1. Add border-color: to the .pr_quality (table) settings in the .css and set it to #B8B8B8

Done

  1. change the height: value of .pr_quality (td) from 0.2em to 0.3750em or 0.50em. the current value makes the bar too thin to be read properly.

I've choosen 0.4em that looks better IMHO

  1. Put the opening & closing <tr> tags on their own lines as well as the closing </table> tag. Not sure if its the test bed or our code, but there is an extra BR being inserted after the table but still inside the containing DIV, #contentSub.

Done

  1. Add class=qualitye to the $void_cell table cell. Makes things consistent and allows for future css manipulation if need be.

Done

The most glaring issue is the lack of rendering of pages transcluded in a range that have not been created. In the linked example to the testbed above, positions 81 to 100 are being called from the Page namespace. 7 of the 20 are red, 2 of the 20 are blue and 2 of the 20 are gray - leaving 9 pages called but not depicted in the status bar (is this the void cell?)

The void cell is only used for toc display when there are pages that does not exists yet. Red links in normal transclusions are considered as quality1.

Anyway, something doesn't seem right (not that it was right before) in the way page status of the entire range being called are being depicted. Seems like percentages of a whole rather fixed widths in px should be the values for width:

Done

GOIII updated the task description. (Show Details)Dec 4 2014, 7:52 AM
GOIII removed a project: Patch-For-Review.
GOIII removed a subscriber: Tpt.
GOIII added a subscriber: Tpt.
GOIII added a comment.Dec 4 2014, 8:09 AM

The void cell is only used for toc display when there are pages that does not exists yet. Red links in normal transclusions are considered as quality1.

Nevertheless if the range being transcluded contains pages that haven't been created yet, the math creating the various status colors should account for that as well as reflect it on the status bar.

  • Quality0 thru Quality4 + Qualitye = total number of possible pages in a source file's Index (Special page index list)

or

  • the entire range being transcluded in the <pages index...> command (mini mainspace pr_quality status bar).

And I realize people are not suppose transcluded ranges with pages in Page: namespace not yet created -- but they still do!

Seems like percentages of a whole rather than fixed widths in px should be the values for width:

Er. I didn't actually test that. I was just making an observation that might have needed to make that switch. Let us see what happens in the testbed afterwards I suppose.

Change 177498 merged by jenkins-bot:
Improve pr_quality CSS

https://gerrit.wikimedia.org/r/177498

Tpt moved this task from Backlog to Top priority on the ProofreadPage board.Mar 19 2015, 7:37 AM
GOIII added a comment.Mar 22 2015, 5:29 AM

Stop using true HTML tables altogether? Suggested replacement....

ProofreadPage.body.php
	$void_cell = $ne ? '<div class="qualitye" style="display: table-cell; width:' . $qe . '%;"></div>' : '';
	$output = '
<div class="pr_quality noprint" style="display: inline-table;">
<div style="display: table-row-group;">
<div style="display: table-row;">
<div class="quality4" style="display: table-cell; width:' . $q4 . '%;"></div>
<div class="quality3" style="display: table-cell; width:' . $q3 . '%;"></div>
<div class="quality2" style="display: table-cell; width:' . $q2 . '%;"></div>
<div class="quality1" style="display: table-cell; width:' . $q1 . '%;"></div>
<div class="quality0" style="display: table-cell; width:' . $q0 . '%;"></div>
' . $void_cell . '
</div>
</div>
</div>';
ext.proofreadpage.article.css
.pr_quality {
	border-collapse: collapse;
	border: 1px dotted #B8B8B8;
	text-align: center;
	height: 0.43em;
	width: 100px;
}

.pr_quality .quality4, .pr_quality .quality3,
.pr_quality .quality2, .pr_quality .quality1,
.pr_quality .quality0, .pr_quality .qualitye {
	padding: 0;
}

Now would be a good time to consider switching this from loading under content-sub once and for all and use indicator tags instead (renders like top-icon class). All that needs is to select a unique name for the tag and wrap the faux table in <indicator name="prp-Quality-progress"></indicator> but some one else will need to make that a reality code wise.

Utilizing display: inline-table would also be used in the replacement of the current mis-aligned list-item-content found in SpecialProofreadPages.php but I'm having trouble identifying the associated .css file -- if there is one? -- for that variant of the quality progress bar.

@Tpt ?

Tpt added a comment.Mar 22 2015, 8:54 AM

We should maybe avoid to use "display: table-*" are they are not supported by IE 8 that is still widely used. So +1 to avoid the use of <table> but maybe when IE8- market share will be lower.

About <indicator>: it is a good idea. I think we should investigate on it. +1 for using it, if the community agrees on the move to the top right side of the title

SpecialProofreadPage.php still use a table with invalid HTML5 attributes and includes only ext.proofreadpage.base.css I think that a first step is to make it use the same HTML as the other quality display.

GOIII added a comment.EditedMar 22 2015, 9:55 AM

Holding off for IE8's death date is fine by me, but I thought it was IE7 and lower that couldn't deal with rows, groups, columns & cells.

++++1 for switching to indicator tags

As for SpecialProofreadPage.php, the absolute first step would be to change...

<table style=\"line-height:70%;\" border=0 cellpadding=5 cellspacing=0 >

to

<table style=\"border-spacing: 0px 0px; line-height:70%;\">

just to make testing easier. Those old attributes keep usurping attempts to find a "better" height / line-height that would finally line things up with the ordered list numbering. Any padding would need to be set against the table-cells (TH, TD) themselves (if any padding is needed at all; progress bar table doesn't need it for sure).

Beyond that, I'm wondering what the point of setting line-height (in % to boot) on the Table achieves when its ol.special > li line-height that needs tweaking.... or is that why the Index: link & info needed its own table in the first place.

@Tpt - here is what I came up with - considering the "math" for arriving at a width value is different from the one used to calculate it for the pr_quality progress-bar - so I left that alone (& is still not 100% HTML5 compliant as a result due to width=$ )...

SpecialProofreadPages.php
	$void_cell = $num_void ? '<td class="qualitye" style="padding:0; border:1px dotted #B8B8B8;" width=' . {$num_void} . '></td>' : '';


	$output = '
<table style="border-collapse:collapse; line-height:1.0;">
<tr>
<td style="padding:0; white-space:nowrap;">' . {$plink} {$dirmark}[$pages] . '</td>
<td style="padding:0;">
<div style="box-sizing:border-box; line-height:0.70; padding:0.20em 0.80em; overflow:hidden; vertical-align:inherit;">
<table style="border-spacing:0px 0px; text-align:center;">
<tr>
<td class="qualityf" style="padding:0;" width="2">&#8200;</td>
<td class="quality4" style="padding:0;" width=' . $q4 . '></td>
<td class="quality3" style="padding:0;" width=' . $q3 . '></td>
<td class="quality2" style="padding:0;" width=' . $q2 . '></td>
<td class="quality1" style="padding:0;" width=' . $q1 . '></td>
<td class="quality0" style="padding:0;" width=' . $q0 . '></td>
' . $void_cell . '
</tr>
</table>
</div>
</td>
</tr>
</table>';

I'm sure I got ahead of myself with the [escape] quotes; please double check that. It can be reduced further by exporting attributes & values to .css

Now would be a good time to consider switching this from loading under content-sub once and for all and use indicator tags instead (renders like top-icon class). All that needs is to select a unique name for the tag and wrap the faux table in <indicator name="prp-Quality-progress"></indicator> but some one else will need to make that a reality code wise.

I moved that to a specific task: T93608: Switch ProofreadPage to use indicator tags.

Change 200870 had a related patch set uploaded (by Tpt):
Makes Special:IndexPages output valid HTML 5

https://gerrit.wikimedia.org/r/200870

Ankry added a subscriber: Ankry.Apr 1 2015, 11:31 AM

Change 200870 merged by jenkins-bot:
Makes Special:IndexPages output valid HTML 5

https://gerrit.wikimedia.org/r/200870

Change 200870 merged by jenkins-bot:
Makes Special:IndexPages output valid HTML 5
https://gerrit.wikimedia.org/r/200870

@Tpt , now that I am able to see the merged results of the above in action on test2.wikipedia.org, I see we've made a slight error in ext.proofreadpage.special.indexpages.css that throws the line item's content to reside on it's own line instead of "in-line" with the bullet number.

In...

.prp-indexpages-row { 
 »       display: table;
 »       border-spacing: 10px 0; 
 »       white-space: nowrap; 
 }

... the value for display: probably should be inline-table; (and the fall back for non-supporting browsers should somehow use table; or inline-block; I suppose?).

In addition, "technically" no inline element (span) should be used to wrap/contain a table -- even when display is set to table-cell or whatever--- so in spite of the above tweak being applied or not, the Special: page fails w3.orgs HTML5 validator.

I think this can be easily "fixed" by using a DIV with display: set to inline instead of the current span for 'wrapping' the html side of the output.

@GOIII Maybe open a new task for that?

GOIII added a comment.Jul 4 2015, 5:41 AM

@GOIII Maybe open a new task for that?

Sorry - don't know how I missed this all this time, H man.

Better yet, how about showing me how to amend the .css myself using the Gerrit Patch Uploader?

He7d3r added a comment.Jul 4 2015, 6:21 PM

@GOIII: You can clone the repository with

git clone https://git.wikimedia.org/git/mediawiki/extensions/ProofreadPage.git

and than make the changes on your local copy. After that, commit your changes and generate the patch for
https://tools.wmflabs.org/gerrit-patch-uploader/
by using one of the commands listed on top of that page.

There is more info on git at https://www.mediawiki.org/wiki/Gerrit/Tutorial.

Change 222848 had a related patch set uploaded (by Tpt):
Fix style issues in Special:IndexPages

https://gerrit.wikimedia.org/r/222848

Change 222848 merged by jenkins-bot:
Fix style issues in Special:IndexPages

https://gerrit.wikimedia.org/r/222848

GOIII added a comment.Jul 10 2015, 1:10 AM

@He7d3r, @Tpt

Checking https://test2.wikipedia.org/wiki/Special:IndexPages against w3.orgs current validator for HTML now reports all ProofreadPage extension related parts meet the current specification although the Special: page itself still fails due to an improper <link> tag syntax/usage that wasn't there the last time we checked against the validator.

The error is being generated is due to the new <link> tag appearing outside of the expected HEAD element in the BODY element instead. In order for that to be valid, the current rel= attribute should be replaced with itemprop= (or simply have the tag generated within the HEAD element). I suspect the module affected, ext.wikimediaBadges, (Flow? Notification?) is acting 'flakey' right now thanks to this incorrect usage.

Regardless, this task can be assigned & closed and another opened for the LINK tag issue if need be in my view.

GOIII triaged this task as Medium priority.Jul 10 2015, 1:11 AM
Tpt closed this task as Resolved.Jul 10 2015, 2:14 AM
Tpt claimed this task.

The HTML is now valid.

About the <link> tag itn't a bug but something intentional: the link to not essential style sheets are set in the bottom of the page in order to don't slow down the initial rendering of the page. It is something invalid in strict HTML but done by all major websites and supported by browsers.

GOIII added a comment.Jul 10 2015, 3:49 AM
In T76284#1443794, @Tpt wrote:

About the <link> tag isn't a bug but something intentional: the link to not essential style sheets are set in the bottom of the page in order to don't slow down the initial rendering of the page. It is something invalid in strict HTML but done by all major websites and supported by browsers.

That is all well and good but doesn't change the fact that this error was not reported the "first" time I checked with the validator. If I'm not mistaken, its now most likely reflecting a change in the overall practice of setting position: to top or bottom depending on the circumstances which seem to have been implemented since that previous check took place.

Also, I was not arguing the wisdom of the link attribute in question being generated almost at the end of the source HTML code under the BODY element rather than within the HEAD element for performance reasons -- I'm guessing that "location" for it would be just fine if the appropriate syntax for such location was applied at the same time.

Either way, just because something happens to "work" does not necessarily translate to being "correct". Maybe the current behavior can be improved upon for that matter. Anyway, it seemed too relevant to just ignore and created task 105436 -- Link tag generated for modules loading bottom fails HTML validation service just in case.

GOIII moved this task from Top priority to Done on the ProofreadPage board.Jun 12 2016, 3:58 AM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptJun 12 2016, 3:58 AM