Create linting rule for large tables
Closed, ResolvedPublic5 Estimated Story Points
Actions

Assigned To

Authored By

	Jdlrobson
	Apr 11 2023, 10:32 PM

Description

We'd like to add a linting rule that heuristically identifies large tables so that we can understand the problem better and find solutions. Since it's not 100% clear how to solve this problem, this should be a hidden linting rule so that editors are not made aware of pages with lint problems that they cannot fix.

Proposal

A linting rule exists that identifies "large" tables. A large table will be defined given the definition below.

Definition of large table

With Vector 2022's limited width any table with more than 5 columns is likely to be problematic so for the purpose of this lint, for now we're identify large tables using the following JavaScript (we can modify it later as we understand the issue better):

Array.from(document.querySelectorAll('table')).filter((table) => table.querySelectorAll('tr > td, tr > th')[0].parentNode.children.length > 5).length > 0

QA testing

Visiting https://en.wikipedia.beta.wmflabs.org/wiki/Special:LintErrors should not show a section for "large-tables"
Visiting https://en.wikipedia.beta.wmflabs.org/wiki/Special:LintErrors/large-tables should list any articles which have large tables. This list should include https://en.wikipedia.beta.wmflabs.org/wiki/A_very_large_table

Sign off steps

Make sure we have a ticket to analyze https://en.wikipedia.org/wiki/Special:LintErrors/large-tables and do related follow up. - T336316

QA Results - Beta

AC	Status	Details
1	✅	T334528#8839063
2	✅	T334528#8839063

QA Results - Prod

AC	Status	Details
1	✅	T334528#8839110
2	✅	T334528#8839110

Details

Subject	Repo	Branch	Lines +/-
Wrong Key for messages	mediawiki/core	master	+16 -16
Bump parsoid to 0.18.0-a8	mediawiki/vendor	master	+483 -95
Create linting rule for large tables	mediawiki/services/parsoid	master	+325 -0
Create linting rule for large tables	mediawiki/extensions/Linter	master	+13 -0
Create linting rule for large tables	mediawiki/core	master	+2 -0

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Open	bwang	T361737 [EPIC] With the current layout, some elements extend beyond the content area and overlap sticky page tools
Duplicate	ovasileva	T66577 Large tables created by editors are not responsive
Resolved	Jdlrobson	T334528 Create linting rule for large tables

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Jdlrobson triaged this task as High priority.Apr 11 2023, 10:32 PM

Jdlrobson mentioned this in T66577: Large tables created by editors are not responsive.

Sbailey subscribed.Apr 11 2023, 10:54 PM

• ssastry edited projects, added Parsoid; removed MediaWiki-Parser.Apr 12 2023, 1:13 AM

Jdlrobson updated the task description. (Show Details)Apr 12 2023, 4:41 PM

MSantos moved this task from Needs Triage to Linting on the Parsoid board.Apr 13 2023, 2:15 PM

Jdlrobson updated the task description. (Show Details)Apr 13 2023, 4:45 PM

ovasileva set the point value for this task to 5.Apr 13 2023, 4:48 PM

LGoto moved this task from Incoming to Ready for Development on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.Apr 17 2023, 5:27 PM

Jdlrobson moved this task from Ready for Development to Incoming on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.Apr 17 2023, 5:32 PM

Jdlrobson removed ovasileva as the assignee of this task.Apr 20 2023, 5:32 PM

Hacked up a bit of test code in php to aid in the process of adapting the javascript snippet to be a linter rule in Parsoid using Parsoids DOM utils. Hoping Parsoid team members with more expertise in html pattern possibilities can comment on the snippet and this codes deficiencies or edge cases where it would fail to detect tables with more than 5 columns based on the html that can exist and be generated by Parsoid. This code can be run standalone :-)

<?php
/* Experimental php DOM manipulation code to detect tables with more than 5 cells per row. This will need
    to be recoded as a Parsoid linter rule using Parsoids DOMutils, but this code is handy as a stand alone testbed.
 * Shannon Bailey  4/20/23  Version 0.1  Wikimedia Foundation
*/

$testHtmlText = '<!DOCTYPE html>
<html>
   <head>
      <title>HTML Table</title>
   </head>
	
   <body>
      <table border = "1" width = "100%">
         <thead>
            <tr>
               <td colspan = "4">This is the head of the table</td>
            </tr>
         </thead>
         
         <tfoot>
            <tr>
               <td colspan = "4">This is the foot of the table</td>
            </tr>
         </tfoot>
         
         <tbody>
            <tr>
               <td>Cell 1</td>
               <td>Cell 2</td>
               <td>Cell 3</td>
               <td>Cell 4</td>
               <td>Cell 5</td>
               <td>Cell 6</td>
            </tr>
         </tbody>
         
      </table>
   </body>
	
</html>';

function processHTML( string $html ) {
    $doc = new DOMDocument();
    $doc->loadHTML('<?xml encoding="UTF-8">' . $html);

    /* Starting with Jon Robsons quick hack javascript to detect a table with more 5 cells wide
        Array.from(document.querySelectorAll('table')).filter((table) =>
            table.querySelectorAll('tr > td, tr > th')[0].parentNode.children.length > 5).length > 0

     for fun, I asked chatGPT to convert the javascript to PHP it and got this:

     $tables = array_filter( iterator_to_array( $doc->getElementsByTagName( 'table' ) ),
        function ( $table ) {
            return $table->getElementsByTagName( 'tr' )->item( 0 )->childNodes->length > 5;
    } );

    which works on test html if there is only a single <tr> inside a <table>, or the first one
    has more than 5 <td>, but fails when such a table is not the first <td>s due to the code using item(0).

    So I hacked that a bit more and this work for the test html, but I am concerned this is not
    correct for more patterns that can be generated by Parsoid, and this needs to be modified to use
    Parsoid DOM utils, not PHP's DOM library.
    */

    $tables = array_filter( iterator_to_array( $doc->getElementsByTagName( 'table' ) ),
        function ( $table ) {
            $count = 0;
            $subTables = $table->getElementsByTagName( 'tr' );
            $subTableCount = $subTables->length;
            for ( $i = 0; $i < $subTableCount; $i++ ){
                $item = $subTables->item( $i );
                $count += $item->childNodes->length > 5;
            }
            return $count;
        } );

    $hasTablesWithMoreThanFiveColumns = count( $tables );

    echo "the html has $hasTablesWithMoreThanFiveColumns tables with more than 5 columns\n";

//    echo $doc->saveHTML();
}

processHTML( $testHtmlText );

• Mabualruz claimed this task.Apr 24 2023, 1:16 PM

• Mabualruz moved this task from Incoming to Doing on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.

ovasileva moved this task from Doing to Ready for Development on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.Apr 24 2023, 4:22 PM

• Mabualruz moved this task from Ready for Development to Doing on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.Apr 24 2023, 5:17 PM

Change 912393 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/services/parsoid@master] Create linting rule for large tables

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

Change 912394 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/extensions/Linter@master] Create linting rule for large tables

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

Change 912395 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/core@master] Create linting rule for large tables

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

• Mabualruz moved this task from Doing to Code Review on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.Apr 28 2023, 4:32 PM

Jdlrobson updated the task description. (Show Details)May 1 2023, 6:11 PM

Change 912395 merged by jenkins-bot:

[mediawiki/core@master] Create linting rule for large tables

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

Change 912394 merged by Sbailey:

[mediawiki/extensions/Linter@master] Create linting rule for large tables

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

I'm not yet seeing the change reflected on https://en.wikipedia.beta.wmflabs.org/wiki/Special:LintErrors/large-tables

• Mabualruz moved this task from Needs More Work to Code Review on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.May 2 2023, 9:25 PM

@Jdlrobson the category is disabled by default as requested, it need config patch to enable it.

• Mabualruz moved this task from Code Review to Needs More Work on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.May 2 2023, 9:53 PM

Per the QA testing, we want the category accessible e.g. at URL https://en.wikipedia.beta.wmflabs.org/wiki/Special:LintErrors/large-tables but not visible on https://en.wikipedia.beta.wmflabs.org/wiki/Special:LintErrors (and also at production) for sign off for this ticket.

The config patch should not make the category appear for anyone who doesn't know the magic URL.

• Mabualruz added a comment.May 3 2023, 12:18 PM

This comment was removed by • Mabualruz.

Change 913944 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/extensions/Linter@master] Create linting rule for large tables

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

• Mabualruz moved this task from Needs More Work to Code Review on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.May 3 2023, 1:57 PM

• Mabualruz moved this task from Code Review to Needs More Work on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.May 4 2023, 2:07 PM

• Mabualruz moved this task from Needs More Work to Code Review on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.May 4 2023, 4:23 PM

Jdlrobson moved this task from Code Review to QA on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.May 4 2023, 7:59 PM

Jdlrobson updated the task description. (Show Details)

Test Result - Beta

Status: ❌
Environment: beta
OS: macOS Ventura
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: Visiting https://en.wikipedia.beta.wmflabs.org/wiki/Special:LintErrors should not show a section for "large-tables"

Screenshot 2023-05-04 at 9.22.36 PM.png (1×3 px, 504 KB)

❌ Visiting https://en.wikipedia.beta.wmflabs.org/wiki/Special:LintErrors/large-tables should list any articles which have large tables. This list should include https://en.wikipedia.beta.wmflabs.org/wiki/A_very_large_table
@Mabualruz I don't get any results, is there something I need to add to the search criteria?

Screenshot 2023-05-04 at 9.24.37 PM.png (1×3 px, 453 KB)

The Parsoid patch that identifies the lints is not yet merged and deployed. So, you should test once that is done -- the vendor patch will likely be Monday.

Change 915788 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/services/parsoid@master] Create linting rule for large tables

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

• Mabualruz moved this task from Needs More Work to Code Review on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.May 5 2023, 3:36 PM

The patches will have final touches on Monday and will be ready for QA, Thanks for @Sbailey, and @ssastry. I will heave a pairing time with @ssastry Monday early to finalise the patches.

Change 916907 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/core@master] Wrong Key for messages

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

Change 912393 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Create linting rule for large tables

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

We need to check the possibility of performance when there is a very long table ( row count > 1000):

We can check the 1st header row and the 1st normal rows.
We can use row's childNodes-> length to reduce functional complexity

We check the number of rows before checking and we can decide to do a full scan or a smaller check depending on that. And that would require to define a threshhold.

• Mabualruz moved this task from Code Review to QA on the Web-Team-Backlog (Web Team FY2022-23 Q4 Sprint 2) board.May 8 2023, 4:23 PM

This will be testable on the beta cluster after the content transform team have deployed, which should hopefully be shortly.

Change 917387 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/vendor@master] Bump parsoid to 0.18.0-a8

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

Jdlrobson updated the task description. (Show Details)May 8 2023, 5:49 PM

Jdlrobson updated the task description. (Show Details)

Change 917387 merged by jenkins-bot:

[mediawiki/vendor@master] Bump parsoid to 0.18.0-a8

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

https://en.wikipedia.beta.wmflabs.org/wiki/Special:LintErrors/large-tables seems to be working on beta

Test Result - Beta

Status: ✅
Environment: beta
OS: macOS Ventura
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: Visiting https://en.wikipedia.beta.wmflabs.org/wiki/Special:LintErrors should not show a section for "large-tables"

Screenshot 2023-05-09 at 3.45.47 PM.png (901×1 px, 156 KB)

✅ AC2: Visiting https://en.wikipedia.beta.wmflabs.org/wiki/Special:LintErrors/large-tables should list any articles which have large tables. This list should include https://en.wikipedia.beta.wmflabs.org/wiki/A_very_large_table

Screenshot 2023-05-09 at 3.44.12 PM.png (1×1 px, 186 KB)

LGTM this can now also be verified on production and resolved:
https://www.mediawiki.org/wiki/Special:LintErrors/large-tables

Jdlrobson reassigned this task from ovasileva to Edtadros.May 9 2023, 10:51 PM

Jdlrobson updated the task description. (Show Details)May 9 2023, 10:54 PM

Test Result - Prod

Status: ✅ mediawiki
OS: macOS Ventura
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: Visiting https://en.wikipedia.org/wiki/Special:LintErrors should not show a section for "large-tables"

Screenshot 2023-05-09 at 3.56.34 PM.png (855×1 px, 142 KB)

✅ AC2: Visiting https://www.mediawiki.org/wiki/Special:LintErrors/large-tables should list any articles which have large tables.
I clicked on a few of the pages and verified that they contained large tables that were not easily viewed on mobile.

Screenshot 2023-05-09 at 3.56.50 PM.png (1×1 px, 196 KB)

Edtadros updated the task description. (Show Details)May 9 2023, 11:00 PM

Jonesey95 subscribed.May 11 2023, 1:50 PM

Please fill in https://www.mediawiki.org/wiki/Help:Extension:Linter/large-tables with details about the purpose of this lint and what editors are expected (or not) do to when encountering it. (main reason why I'm re-opening)

In T334528#8839110, @Edtadros wrote:

✅ AC1: Visiting https://en.wikipedia.org/wiki/Special:LintErrors should not show a section for "large-tables"

I don't agree with this, I think trying to hide it is more confusing than not. Because it's in the database, it still shows up in ?action=info, and external tools like lintHint, etc. If it's not ready for editors yet, we can put it in a separate "Ignore" category or something.

In T334528#8845446, @Legoktm wrote:

In T334528#8839110, @Edtadros wrote:

✅ AC1: Visiting https://en.wikipedia.org/wiki/Special:LintErrors should not show a section for "large-tables"

I don't agree with this, I think trying to hide it is more confusing than not. Because it's in the database, it still shows up in ?action=info, and external tools like lintHint, etc. If it's not ready for editors yet, we can put it in a separate "Ignore" category or something.

I made a similar comment in T334527, which is still open. I don't really know what "NOTE: Blocked on T334527" means at the top of this task, but that task is still open, and this one was closed. That confuses me. Maybe the "hiding" of this new Linter tracking flag was not yet fully and properly enabled prior to the flag's deployment?

I don't agree with this, I think trying to hide it is more confusing than not. Because it's in the database, it still shows up in ?action=info, and external tools like lintHint, etc. If it's not ready for editors yet, we can put it in a separate "Ignore" category or something.

@Legoktm can you move this feedback to T334527 which is the task we're using to talk about motivation here. To be clear this is temporary while we fine tune the linting rule which is throwing up some false positives.

Jdlrobson updated the task description. (Show Details)May 11 2023, 5:01 PM

• Mabualruz mentioned this in T336528: P99 performance of Linting went from 1-2s to 8-11 seconds.May 11 2023, 5:04 PM

Documentation has been add. Thanks for the prompt!

ReleaseTaggerBot added a project: MW-1.41-notes (1.41.0-wmf.10; 2023-05-23).May 16 2023, 5:02 AM

Large-tables lint still requires a follow on patch to avoid a serious performance problem when tables greater than 1000 rows are encountered, looking at the number of rows and if under some threshold, checking them all, and over that threshold, looking at the first few rows and header row.

@Sbailey please see T336528

Got it, excellent, your on it :-)
Will add myself as a reviewer if not already included.
Thanks

Change 915788 abandoned by Mabualruz:

[mediawiki/services/parsoid@master] Linter: Check and Optimise performance for new linting rules

Reason:

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

Change 915788 restored by Mabualruz:

[mediawiki/services/parsoid@master] Linter: Check and Optimise performance for new linting rules

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

Change 916907 abandoned by Mabualruz:

[mediawiki/core@master] Wrong Key for messages

Reason:

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

	F36990282: Screenshot 2023-05-09 at 3.56.50 PM.png
	May 9 2023, 10:58 PM

	F36990280: Screenshot 2023-05-09 at 3.56.34 PM.png
	May 9 2023, 10:58 PM

	F36990270: Screenshot 2023-05-09 at 3.44.12 PM.png
	May 9 2023, 10:47 PM

	F36990268: Screenshot 2023-05-09 at 3.45.47 PM.png
	May 9 2023, 10:47 PM

	F36976318: Screenshot 2023-05-04 at 9.24.37 PM.png
	May 5 2023, 4:28 AM

	F36976316: Screenshot 2023-05-04 at 9.22.36 PM.png
	May 5 2023, 4:28 AM

Create linting rule for large tablesClosed, ResolvedPublic5 Estimated Story PointsActions

Description

Proposal

Definition of large table

QA testing

Sign off steps

QA Results - Beta

QA Results - Prod

Details

Related ObjectsSearch...

Event Timeline

Test Result - Beta

QA Steps

Test Result - Beta

QA Steps

Test Result - Prod

QA Steps

Create linting rule for large tables
Closed, ResolvedPublic5 Estimated Story Points
Actions

Related Objects
Search...