Page MenuHomePhabricator

Regression: Collapsible forms stopped working on Mobile special page
Closed, ResolvedPublic3 Estimated Story Points

Description

Tested with Google Chrome 84. Seems to have started in 1.36.0-wmf.3/271fb28d99c5

  1. Visit the desktop site https://www.mediawiki.org/wiki/Special:AbuseLog
    • Observe the form starts in collapsed state.
    • Click on the form header 'search the abuse log'
    • Observe the form expanded.
  1. Now visit the mobile site https://m.mediawiki.org/wiki/Special:AbuseLog
    • Observe the form starts in expanded state (the code says otherwise should happen)
    • Now click on the form header
    • Observe that nothing happens, the form fails to collapse
  1. Visit another mobile site https://m.mediawiki.org/wiki/Special:Contributions/Jdlrobson with AMC enabled
    • Observe the form starts in collapsed state.
    • Click on the form header
    • Observe nothing happens, the form fails to expand

Note: You need AMC mode enabled for the mobile links above.

Seems to be regression from T257265/T250851
Related: T257127

QA Results - Beta

ACStatusDetails
1T260642#6461094
2T260642#6461094 the observed behavior is acceptable per T260642#6484520
3T260642#6461094

QA Results - Prod

ACStatusDetails
1T260642#6499304
2T260642#6499304 the observed behavior is acceptable per T260642#6484520
3T260642#6504873

QA Results - Prod

ACStatusDetails
1T260642#6566742
2T260642#6566742 the observed behavior is acceptable per T260642#6484520
3T260642#6566742

Event Timeline

Ammarpad created this task.Aug 18 2020, 3:00 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 18 2020, 3:00 AM
Ammarpad updated the task description. (Show Details)Aug 18 2020, 6:43 AM
Ammarpad added subscribers: Peter.ovchyn, Jdlrobson.
Ammarpad renamed this task from Collapsible forms in inconsistent state and stopped working on Mobile to Collapsible forms stopped working on Mobile.Aug 18 2020, 6:53 AM
Jdlrobson renamed this task from Collapsible forms stopped working on Mobile to Regression: Collapsible forms stopped working on Mobile special pag.Aug 18 2020, 3:02 PM
Jdlrobson renamed this task from Regression: Collapsible forms stopped working on Mobile special pag to Regression: Collapsible forms stopped working on Mobile special page.Aug 18 2020, 3:06 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Krinkle.

Given the problems we've been having, it might make sense to update mediawiki.page.ready so that collapsible and sortable elements are enabled on the special page namespace e.g. the configuration only applies to other namespaces. That might make the config name confusing however.

https://github.com/wikimedia/mediawiki/blob/master/resources/src/mediawiki.page.ready/ready.js#L23

cc @Krinkle

Jdlrobson triaged this task as High priority.Aug 18 2020, 3:11 PM

Patching all these special pages to explicitly call the relevant method, while tedious is probably the best solution here.

I am not sure what method you're referring here.

Jdlrobson added a comment.EditedSep 4 2020, 4:26 PM

Sorry. I'll be clearer. The special pages should call makeCollapsible(); or tablesorter(); themselves as well as listing the dependencies via addModules/addModuleStyles rather than depend on mediawiki.page.ready

In the case of https://m.mediawiki.org/wiki/Special:AbuseLog this code would be added to the module mediawiki.htmlform :

if ( mw.loader.getState(''jquery.makeCollapsible') === 'registered' && $('.mw-collapsible').length ) {
  mw.loader.using(['jquery.makeCollapsible']).then(() => {
    $('.mw-collapsible').makeCollapsible(); } )
  });
}

(applying makeCollapsible twice has no effect so no need to worry about that)

Thanks, but I am not sure about the suggested approach.

Given the problems we've been having, it might make sense to update mediawiki.page.ready so that collapsible and sortable elements are enabled on the special page namespace e.g. the configuration only applies to other namespaces.

In my view updating in one place would be better than doing so in each special page. These pages were working normally before this change.

That might make the config name confusing however.

I am not sure whether apart from confusing name there's any other reason. Confusable name can be changed to reflect expanded scope.

In my view updating in one place would be better than doing so in each special page.

I think it's better to have explicit dependencies if possible as it provides self-explanatory documentation. If a special page knows it needs a form to be made collapsible it needs to do that itself. Doing this inside the module mediawiki.htmlform looks like a good place to do this as it means one place for each special page (assuming they are using HtmlForm which they should be) and makes it clear that these forms should be treated different from user generated content. That then means mediawiki.page.ready is used exclusively for content.

This lack of knowledge of which pages have collapsible forms that rely on mediawiki.page.ready makes me a little uncomfortable. If we ever want to move away from this code to another library in current form we would need to search for the collapsible class across the codebase. However, if the module is used with the makecollapsible call then things are explicitly clear.

@Jdlrobson The patch from Fomafix effectively does this by making makeCollapsed self-contained including its activation logic. See https://gerrit.wikimedia.org/r/c/mediawiki/core/+/503101. This would allow special pages to queue that module as you say, to make them responsible and aware of their dependency, but also still have the benefit of not needing additional modules or payloads just to call one function, keeping the interface declarative instead, which seems preferred.

ovasileva set the point value for this task to 3.Sep 9 2020, 4:44 PM

Change 503101 had a related patch set uploaded (by Jdlrobson; owner: Fomafix):
[mediawiki/core@master] Move activating of makeCollapsible out of 'mediawiki.page.ready'

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

Change 503101 merged by jenkins-bot:
[mediawiki/core@master] Move activating of makeCollapsible out of 'mediawiki.page.ready'

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

Edtadros reassigned this task from Edtadros to Jdlrobson.Sep 15 2020, 1:50 AM
Edtadros added a subscriber: Edtadros.

Test Result - Beta

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

Test Artifact(s):

QA Steps

✅ AC1. Visit the desktop site https://en.wikipedia.beta.wmflabs.org/wiki/Special:AbuseLog

  • Observe the form starts in a collapsed state.
  • Click on the form header 'search the abuse log'
  • Observe the form expanded.

❌ AC2. Now visit the mobile site https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:AbuseLog

  • Observe the form starts in expanded state (the code says otherwise should happen)
  • Now click on the form header
  • Form should collapse

✅ AC3. Visit another mobile site https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:Contributions/Abuse_filter

  • Observe the form starts in a collapsed state.
  • Click on the form header
  • Form should expand and collapse when clicked.

Edtadros updated the task description. (Show Details)Sep 15 2020, 1:55 AM

Change 627543 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Collapsible HTMLForms need to add JS module

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

Change 627543 merged by jenkins-bot:
[mediawiki/core@master] Collapsible HTMLForms need to add JS module

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

@Jdlrobson in the second step, the form briefly flashes the expanded state then collapses. Is this as expected?

I think this is fine. Likely an existing problem.

Edtadros updated the task description. (Show Details)Sep 23 2020, 4:14 PM
Edtadros added a comment.EditedSep 28 2020, 4:33 PM

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps

✅ AC1. Visit the desktop site https://en.wikipedia.org/wiki/Special:AbuseLog

  • Observe the form starts in a collapsed state.
  • Click on the form header 'search the abuse log'
  • Observe the form expanded.

✅ AC2. Now visit the mobile site https://en.m.wikipedia.org/wiki/Special:AbuseLog

  • Observe the form starts in expanded state (the code says otherwise should happen also see T260642#6484520)
  • Now click on the form header
  • Form should collapse

❓ AC3. Visit another mobile site https://en.m.wikipedia.org/wiki/Special:Contributions/Abuse_filter

  • Observe the form starts in a collapsed state.
  • Click on the form header
  • Form should expand and collapse when clicked.

@Jdlrobson is there another page that has the form. I get the following:

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps

✅ AC3. Visit another mobile site https://en.m.wikipedia.org/wiki/Special:Contributions/Abuse_filter

  • Observe the form starts in a collapsed state.
  • Click on the form header
  • Form should expand and collapse when clicked.

@Jdlrobson is there another page that has the form. I get the following:

Edtadros updated the task description. (Show Details)Sep 30 2020, 12:41 PM
Edtadros reassigned this task from Edtadros to ovasileva.Sep 30 2020, 12:59 PM
ovasileva closed this task as Resolved.Sep 30 2020, 1:07 PM

Looks good, thanks @Edtadros!

matmarex reopened this task as Open.Tue, Oct 13, 8:33 PM
matmarex added a subscriber: matmarex.

I'm reverting the patch because it caused T265134: "wikitable sortable mw-collapsible mw-collapsed" is no longer displaying sortable headings when there are two tr lines with th elements.

Looks like @Jdlrobson has previously outlined an alternative approach for this task:

Sorry. I'll be clearer. The special pages should call makeCollapsible(); or tablesorter(); themselves as well as listing the dependencies via addModules/addModuleStyles rather than depend on mediawiki.page.ready

In the case of https://m.mediawiki.org/wiki/Special:AbuseLog this code would be added to the module mediawiki.htmlform : (…)

This shouldn't cause that regression again, and I personally like it a lot more (quoting another comment, "better to have explicit dependencies if possible as it provides self-explanatory documentation"), so I'll try to submit a patch to do it.

Change 633759 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] Revert "Move activating of makeCollapsible out of 'mediawiki.page.ready'"

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

Change 633826 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] mediawiki.htmlform: Ensure collapsible forms are enabled

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

Jdlrobson lowered the priority of this task from High to Medium.Wed, Oct 14, 5:13 PM
ovasileva removed ovasileva as the assignee of this task.Thu, Oct 15, 5:12 PM
ovasileva added a subscriber: ovasileva.

Change 633759 merged by jenkins-bot:
[mediawiki/core@master] Revert "Move activating of makeCollapsible out of 'mediawiki.page.ready'"

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

Change 633826 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.htmlform: Ensure collapsible forms are enabled

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

@matmarex Can you confirm whether it is just the ordering of activation that's the issue, or whether there's more? It seems to me like such fragile contract should is a bug and is worth fixing by itself, e.g. by actually adding the missing supporting for more table structures in makeCollapsible, or by embracing this kind of reliance as a first-class primitive in mw.hook and creating some form of priority/ordering concept, e.g. mw.hook( 'wikipage.ready-after' ) or some such.

I'm asking because there's numerous other developments in the skin on-going that have been planned specifically around this code no longer being part of mediawiki.page.ready, which I can't tell right now whether that needs to be reimagined in a different way or whether this is just temporary while we fix the root cause.

What do you mean by "there's more"? There is certainly some root problem in jquery.makeCollapsible or jquery.tablesorter, which could presumably be debugged and resolved, but I did not have time to debug this further, and just making them run in this order seemed reliable enough (see T64878).

I don't know anything about planned developments, but you might want to debug this then if it's blocking some other work.

Jdlrobson updated the task description. (Show Details)Tue, Oct 20, 5:17 PM
Edtadros reassigned this task from Edtadros to ovasileva.Wed, Oct 21, 2:33 AM

Test Result - Prod

Status: ✅ PASS
Environment: mediawiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps

✅ AC1. Visit the desktop site https://en.wikipedia.org/wiki/Special:AbuseLog

  • Observe the form starts in a collapsed state.
  • Click on the form header 'search the abuse log'
  • Observe the form expanded.

✅ AC2. Now visit the mobile site https://en.m.wikipedia.org/wiki/Special:AbuseLog

  • Observe the form starts in expanded state (the code says otherwise should happen also see T260642#6484520)
  • Now click on the form header
  • Form should collapse

✅ AC3. Visit another mobile site https://m.mediawiki.org/wiki/Special:Contributions/Jdlrobson

  • Observe the form starts in a collapsed state.
  • Click on the form header
  • Form should expand and collapse when clicked.

Edtadros updated the task description. (Show Details)Wed, Oct 21, 2:33 AM
ovasileva closed this task as Resolved.Wed, Oct 21, 8:15 AM

Looks good! Resolving.