Page MenuHomePhabricator

jquery.makeCollapsible: Refactor to use CSS instead of JavaScript to do the expansion/collapse (including initial state)
Closed, ResolvedPublic3 Story Points

Description

Right now we select all state-collapsed elements when the document is ready and hide them (without animation, but still, they are hidden *after* they are first shown, no matter how short that initial show is). The problem with that is that it is a exponentially slow operation (ON).

By using CSS instead the initial state will be reflected immediately without any delay. Just like CSS "a { color: green; }" will make all anchor tags green (even anchor tags parsed after the CSS was parsed), likewise CSS like ".client-js .mw-collapsed { hidden }" will make them hidden without delay.

On top of this, we can build further and also optimise the cost of animations. Instead of animating with javascript, lets let JavaScript only toggle classes and the CSS will take care of the state and (if CSS3 is supported) make the transition animated.

ux notes

As a result of this issue, when loading pages that have templates with show/hide functionality for additional content, the templates start out open, then close, which results in a poor experience (e.g. text reflowing, elements jumping around on the page).

Check out:
https://en.wikipedia.org/wiki/Labastide-Esparbairenque
https://en.wikipedia.org/wiki/Solo_(music)

developer notes

The issue is demonstrated in https://en.wikipedia.org/wiki/Labastide-Esparbairenque?useskin=vector (see page issue)

A simple css rule targetting client-js and hiding uninitialised collapsable initially collapsed content elements would be a good start. This rule would ensure content is shown with js disabled.

The code that handles the collapsing is in https://github.com/wikimedia/mediawiki/blob/master/resources/src/jquery/jquery.makeCollapsible.js

The fix should be easy but we will probably need to seek input from people familiar with this code as part of our code review process
https://phabricator.wikimedia.org/T42812#3644347

Testing criteria

  • With this viewable in the page, refresh the browser. When the page loads the content that is expandable should be hidden by default
  • Disable JavaScript and the content should be visible by default
  • Explore other pages, which include the text "expand" - are there any examples of pages where content disappears on page load like this (?):

Details

Reference
bz40812

Related Objects

Event Timeline

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

@Krinkle is the collapsible code using delegate events? Seems like a good candidate if not

Jdlrobson added a subscriber: Jhernandez.
This comment was removed by Jdlrobson.
Jdlrobson updated the task description. (Show Details)Apr 3 2018, 4:37 PM
Jdlrobson set the point value for this task to 3.
alexhollender updated the task description. (Show Details)Apr 3 2018, 5:18 PM
TheDJ added a comment.EditedApr 4 2018, 9:25 AM

Small note. There are several techniques used here, all with their own limitations and use cases. For en.wikipedia

  1. toc (core, state kept client side)
  2. watchlist notifications (default gadget, state kept client side)
  3. central notice notifications (extension, state kept client side)
  4. NavFrame (collapsible divs MediaWiki:Common.js, state kept in wikitext)
  5. collapsible (collapsible tables MediaWiki:Common.js, state kept in wiki text and interpreted client side )
  6. mw-collapsible (collapsible div/tables/lists/etc provided by core, state kept in wiki text and interpreted client side)

The long term goals of mw-collapsible is to replace collapsible and NavFrame, but this has not yet been achieved, due to several blockers

en.wikipedia uses some custom work to reduce the page jumping, when it knows that the area is going to be hidden anyways.

/* Reduce page jumps by hiding collapsed/dismissed content */
.client-js .mw-special-Watchlist #watchlist-message,
.client-js .NavFrame.collapsed .NavContent,
.client-js .collapsible.collapsed > tbody > tr:not(:first-child) {
	display: none;
}

This works for en.wp's collapsible and NavFrame stuff. It does not work for:

  1. collapsible's autocollapse (requires JS logic to determine state of the element)
  2. mw-collapsible (requires too much JS logic to predetermine state)
TheDJ added a comment.EditedApr 4 2018, 9:39 AM

Challenges for mw-collapsible is:

  1. it determines it's behaviour based on the html element that it is applied to. This might cause the fallback CSS to have to vary.
  2. you need to cancel out stuff in print and noscript (make the auto hiding css apply to media screen only and use .client-js ? )
  3. the area that is hides can be dynamically determined. This dynamic behaviour is the biggest problem. Worse, it's class definitions are mixed with cases that could be non-dynamic. making it possible to distinguish between these cases from CSS is likely needed (so extra css classes).
  4. State of the toggle needs to match the state of the CSS

page with different use cases, incl. dynamic ones: https://test.wikipedia.org/wiki/Test_suite_for_mw-collapsible

Change 424165 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] mw-collapsible no longer causes page jump

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

Note on slow connections, collapsed content will not be accessible until the JavaScript has fully loaded, however this is arguably less of a problem then the existing reflows.

This sounds exactly like what I was afraid of in T42812#3351094 and what matmarex and Krinkle assured me would not happen.

@Nirmos Let me reassure you :) It works like this: [this is a bit simplified, but mostly correct]

  1. The page is loaded with the collapsed content being visible.
  2. The base CSS (https://gerrit.wikimedia.org/r/#/c/424165/2/resources/src/jquery/jquery.renderBlocking.less) is loaded, but because it only applies to the client-js class, it is not actually used.

    If the user has JavaScript disabled, this is the end. "Collapsed" content is still accessible.
  1. If JavaScript is enabled, a very short inline script adds the client-js class to the <html> tag. This causes the CSS to apply and hide the collapsed content. At this point the collapsed content is not accessible.

    (Everything up to this point normally happens before the page is rendered and seen by the user for the first time.)
  1. If JavaScript is enabled, all other JavaScript and CSS code loads, including code to add the [collapse]/[expand] buttons and styles for them.

    Collapsed content is thus accessible again, unless the base CSS loaded successfully but an an Internet connection error or something prevented the other JS/CSS from loading. That is a rare case and we generally consider it acceptable, since doing it this way makes the normal case much nicer.

This actually turned out a little trickier than I imagined since the toggle button is creating via JavaScript. I've put the code on the reading web staging environment so people can test this with real content (hopefully that reassures you @Nirmos :))

http://reading-web-staging.wmflabs.org/wiki/Labastide-Esparbairenque?useskin=vector
http://reading-web-staging.wmflabs.org/wiki/Spain

I've copied the test page from TheDJ - I still need to go through all those and work out which ones by design should cause a FOUC ...
https://reading-web-staging.wmflabs.org/w/index.php?title=User:Jdlrobson/sandbox-collapsible&mobileaction=toggle_view_desktop

Thanks for the input @matmarex I'll continue this up tomorrow!

Looking through examples on the page shared by TheDJ i dont think it's going to be technically feasible to support custom toggle labels on collapsed elements. I wonder if we can discourage those and encourage marking up the button in html rather than passing the label via data atrributes.

Change 425211 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Only load mediawiki.jquery.styles if page HTML suggests needed

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

@Niedzielski to add his review but more reviews from the team are requested. /cc @pmiazga, @Jhernandez, @Jdrewniak

stjn added a subscriber: stjn.EditedApr 16 2018, 3:32 PM

Out of interest: have you tested it with animations provided by jquery.makeCollapsible? I myself added a similar hack for mw-collapsible tables in Russian Wikipedia (since we used a lot of them in some places) and it had to be disabled because it was causing the block to play animation on the first open and then open-close like a standard collapsible boxes afterwards. If that is resolved on the global level, then great.

UPD to this comment: yes, that’s exactly what happens for me (Firefox 59, Windows 8.1) with examples provided above.

Change 427949 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Introduce ResourceLoaderLessVarFileModule

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

Change 425211 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Skins: getDefaultModules can now define render blocking CSS

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

@stjn Good point. It looks like this happens because we add the mw-collapsed class before the collapse animation begins, rather than after it ends. It did not make a difference before this patch, but it matters now, since we're adding CSS like div.mw-collapsed .mw-collapsible-content { display: none; }, because it causes the content to be hidden instantly with this CSS rule before the animation runs. It should be easy enough to rearrange the code so that we add mw-collapsed at the end, I'll point this out on the Gerrit change.

One patch has already been merged and https://gerrit.wikimedia.org/r/#/c/425211/ has a +1 from Krinkle
https://gerrit.wikimedia.org/r/#/c/427949/ and https://gerrit.wikimedia.org/r/#/c/424165/ have no reviews
I think these patches should be in a stable state now so reviews welcomed.

Change 427949 merged by jenkins-bot:
[mediawiki/core@master] Introduce ResourceLoaderLessVarFileModule

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

Jdlrobson added a comment.EditedApr 26 2018, 6:15 PM

https://gerrit.wikimedia.org/r/#/c/425211/ has a +1 from @pmiazga and I've updated https://gerrit.wikimedia.org/r/#/c/424165/ to address the Jenkins issues. Conversation on that ticket seems to have died down.. I think that's a good thing?

Change 429271 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Dequeue render blocking styles

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

Change 425211 merged by jenkins-bot:
[mediawiki/core@master] Skins: getDefaultStyles can now define render blocking CSS

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

Change 429271 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Dequeue render blocking styles

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

stjn added a comment.EditedMay 1 2018, 5:42 PM

I am not a reviewer, but wouldn’t it make more sense to have a standard class like .mw-collapsible-fallback for elements that were not yet processed than to have CSS declarations as big as table.mw-collapsed:not(.mw-made-collapsible) thead:first-child tr:first-child th:last-child:before in the code? This (part with .mw-collapsed:not(.mw-made-collapsible)) seems somewhat overspecific.

@stjn something like that would require us updating all the content of the wiki pages using mw-collapsible as it would need to be in the initial page HTML. Doable but tricky and would need a bot. The not selector is well supported so although the generated selectors are hard to read in the output code they should be well supported. We can always move away from it at a later time.

Just https://gerrit.wikimedia.org/r/#/c/424165/ remaining. Has a +1. My team has committed to getting this merged with me tomorrow, so if anyone has any feedback today please get it in ASAP!

stjn awarded a token.May 1 2018, 10:54 PM
stjn added a comment.May 1 2018, 11:19 PM

Just https://gerrit.wikimedia.org/r/#/c/424165/ remaining. Has a +1. My team has committed to getting this merged with me tomorrow, so if anyone has any feedback today please get it in ASAP!

On second thought: is this available to look at somewhere, by the way? This link seems like it runs an old version with those bugs present. Some examples don’t show animation at all, some show it in phases (tables), some work fine and some run into the same error I described above.

No problem! I've updated our staging server so you can take a look at it. You can now test on http://reading-web-staging.wmflabs.org/wiki/California which mirrors content from production. Feel free to create some test cases in your English Wikipedia sandbox and you should be able to view them there. That link should now work.

stjn added a comment.EditedMay 1 2018, 11:42 PM

Collapsed by default tables in California page show [Collapse] first and then turn to [Expand]. Is this expected? On ‘Expanded by default’ and ‘Collapsed by default’ sections in your page there are no CSS-generated [Collapse] labels.

Other than that I can’t notice any potential issues, especially with the stuff that I directly use (links to Russian Wikipedia: custom collapsible table rows which don’t get changed by the change, and custom collapsible divs). Great stuff to see this fixed on global level.

Collapsed by default tables in California page show [Collapse] first and then turn to [Expand]. Is this expected? On ‘Expanded by default’ and ‘Collapsed by default’ sections in your page there are no CSS-generated [Collapse] labels.

Great catch! I've fixed this and updated the reading-web-staging labs instance. Thanks!

The patch is +2ed but blocked on some Jenkins trouble in T189329

Change 424165 merged by jenkins-bot:
[mediawiki/core@master] sortable tables/mw-collapsible no longer causes page jump

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

Jdlrobson removed Jdlrobson as the assignee of this task.May 3 2018, 1:25 AM

This should be fixed on the beta cluster now. The change was a tricky one and we have almost a week until next train so if anyone reading can spare some time to test this change out on the beta cluster it would be most appreciated :)

@alexhollender
@ABorbaWMF i will add some testing steps tomorrow!

TheDJ added a comment.EditedMay 3 2018, 7:55 AM

I have resynced Common.js/css on beta labs with en.wp's version and I have imported Help:Collapsing from there as well, seems ok for me so far.

I note that many of the collapsible on en.wikipedia pages, do NOT actually use mw-collapse, but this change will bring that transition another step closer.

Jdlrobson updated the task description. (Show Details)May 3 2018, 7:46 PM
Jdlrobson assigned this task to alexhollender.

I've added some testing instructions for you and @ABorbaWMF take a look!

@Jdlrobson @ABorbaWMF I checked out a bunch of other states (which all seem to also have expandable infoboxes) and all looks good. Passing this over to you Anthony.

Looks good to me on staging. Used the California article. The expand/collapse seems to work well and turning off Javascript displays content as expected.

ovasileva closed this task as Resolved.May 8 2018, 10:35 AM
ovasileva added a subscriber: ovasileva.

looks good - thanks all!

So, how should all the custom toggles be repaired across wikis (example)? Is adding .mw-made-collapsible a proper fix?

TheDJ added a comment.May 11 2018, 1:46 PM

@Jack_who_built_the_house both revisions look identical to me... "mw-made-collapsible" is added automatically (it basically means: "this is a collapsible that has now been processed by the JS).

Thanks! I guess the method we use was acquired from some documentation that is now outdated.