Page MenuHomePhabricator

Cannot collapse/uncollapse headings: TypeError: Cannot read property 'className' of null(…)
Closed, ResolvedPublic5 Estimated Story Points

Description

The MobileFormatter:prepareHeadings method is over-zealous about which headings are collapsible. This could be leading to various edge cases where headings work inappropriately resulting in strange UI's being exposed to our readers.

Suggested implementation

As @TheDJ suggests this could be remedied by a refactor of MobileFormatter::makeSections.

  • Repurpose prepareHeadings to prepareHeading
  • Before inserting sectionBody a call to prepareHeading should be run on the $node (the heading)

Acceptance Criteria

  • Add a test to capture the example in https://en.m.wikipedia.beta.wmflabs.org/wiki/Headings_in_infobox - expected behaviour is that the heading inside the infobox should not have 'section-heading' class or an onlick event.
  • In certain cases no headings should be transformed, for example any headings inside a div e.g. "Off the beaten path" in this example should be left untouched.
Headings_in_infobox:60 Uncaught TypeError: Cannot read property 'className' of null(…)

Sign off notes

  • Update T130898 to reflect any changes as a result of this work

Event Timeline

pmiazga created this task.Nov 30 2016, 11:17 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 30 2016, 11:17 PM
pmiazga renamed this task from Cannot collapse/uncollapse headings inside infoboxes to Cannot collapse/uncollapse headings.Dec 1 2016, 9:13 PM
bmansurov triaged this task as Medium priority.Dec 1 2016, 9:14 PM
bmansurov added a project: Readers-Web-Backlog.
bmansurov moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
bmansurov added subscribers: ovasileva, bmansurov.

@ovasileva making normal as it's an edge case.

Aklapper renamed this task from Cannot collapse/uncollapse headings to Cannot collapse/uncollapse headings: TypeError: Cannot read property 'className' of null(…).Feb 12 2017, 5:17 PM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 11 2017, 7:38 PM

Happy to review a patch from a volunteer if someone wants to have a go at this!

Jdlrobson set the point value for this task to 1.Apr 20 2017, 11:19 PM
TheDJ added a subscriber: TheDJ.EditedApr 24 2017, 7:19 PM

The fault tree of this problem is
1: skins.minerva.toggling/init.js tries to upgrade window.mfTempOpenSection to a 'mobile.toggle/Toggler'
2: it filters headers as follows: $container.find( '> h1,> h2,> h3,> h4,> h5,> h6' ). This is correct for Toggler, but not for this situation, since mfTempOpenSection is added to EVERY header initially. It's original addition does not match this filter.
3: Because of this mismatch in the 'upgrade', the onclick handler is not removed from this 'ineligible' header.
4: At the same time the Toggler initialisation will fail for this header
5: After the skipped onclick removal, it also does: delete window.mfTempOpenSection; to remove this function from the global context
6: That however never works, since delete doesn't operate on non-configurable properties of an object, which this is. And I have no idea why you would do this to begin with, since the global scope is already polluted, cleaning up doesn't really seem useful
7: Thus when clicking the 'non-eligible'-header (because in an infobox), you hit the window.mfTempOpenSection function.
8: The clickhandler function tries to find the section var block=document.getElementById("mf-section-"+id);, but the section was never added, since this is an 'ineligible' header, so the block is not in the HTML
9: The block variable is directly used, without checking if it was actually found.
10: JS error thrown.

Suggested solution:
Change MobileFormatter::prepareHeadings into MobileFormatter::prepareHeading, and prep only those headings for which a section has been created as well (See MobileFormatter::makeSections)

Jdlrobson moved this task from Upcoming to Needs Prioritization on the Readers-Web-Backlog board.

Description needs to be clearer.

Jdlrobson updated the task description. (Show Details)Apr 27 2017, 5:47 PM
Jdlrobson removed the point value for this task.
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.
Jdlrobson removed Jdlrobson as the assignee of this task.Apr 29 2017, 12:07 AM
Jdlrobson updated the task description. (Show Details)May 1 2017, 4:20 PM
Jdlrobson set the point value for this task to 5.May 4 2017, 5:11 PM
Jdlrobson updated the task description. (Show Details)May 4 2017, 6:20 PM

Change 353293 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Only wrap top level headings

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

Change 353293 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Only wrap top level headings

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

phuedx reassigned this task from Jdlrobson to ABorbaWMF.May 15 2017, 11:00 AM
phuedx added subscribers: ABorbaWMF, phuedx.

@ABorbaWMF: Could you test section collapsing (clicking on top-level headings to hide/show their content) works as expected on https://en.m.wikipedia.beta.wmflabs.org?

I can verify that clicking on "Heading" on https://en.m.wikipedia.beta.wmflabs.org/wiki/Headings_in_infobox no longer causes an error.

Tested against a few pages on beta using different combinations of devices and browsers using browserstack. Everything looks good.

@phuedx If you've tested the error case, I think this can move to signoff.

phuedx removed ABorbaWMF as the assignee of this task.May 16 2017, 4:47 AM

@Jdlrobson wrote the change and I tested and merged it. Who's the lucky signer offer?

This is a technical task and @ABorbaWMF has tested and the beta cluster example is now working as expected. I've added a note to test the Wikivoyage example in the calendar.

Jdlrobson closed this task as Resolved.May 16 2017, 9:39 AM
Jdlrobson claimed this task.