Page MenuHomePhabricator

Reduce page jumps in table of contents
Closed, ResolvedPublic

Description

After the resolving of T42812, I noticed that there is at least one area where there are also reflows present before the page is loaded: table of contents. For users with JavaScript, (typically) [hide] also appears late and moves heading afterwards.

Is it a problem at all and should we fix it the same way as we fixed T42812?

Other thing we can do is probably redo TOC on .mw-collapsible. However, labels in .mw-collapsible and in .toc are not the same.

QA steps

Visit the table of contents on desktop
https://en.wikipedia.beta.wmflabs.org/wiki/Albert_Einstein
Confirm that the table of contents does not have any visual change (to the heading) during the loading of the page.

"Contents" should not jump to the left on page load (gif shows broken and fixed state)

Please test in IE8, IE9 as well as the usual browsers to confirm.

Event Timeline

stjn created this task.May 19 2018, 12:14 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 19 2018, 12:14 PM

There is a patch pending for this that uses similar approach to T42812: https://gerrit.wikimedia.org/r/#/c/372515/

Change 372515 had a related patch set uploaded (by Bartosz Dziewoński; owner: Fomafix):
[mediawiki/core@master] Hide TOC with CSS instead of JavaScript

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

Nirmos added a subscriber: Nirmos.May 19 2018, 4:40 PM
Vvjjkkii renamed this task from Reduce page jumps in table of contents to 6ocaaaaaaa.Jul 1 2018, 1:09 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from 6ocaaaaaaa to Reduce page jumps in table of contents.Jul 1 2018, 6:32 PM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.

Change 445258 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Expanding/Collapsing table of contents should not cause reflow

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

Change 372515 merged by jenkins-bot:
[mediawiki/core@master] Hide TOC with CSS instead of JavaScript

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

Restricted Application added a project: Readers-Web-Backlog. · View Herald TranscriptJul 11 2018, 8:59 PM

Patch got merged from a volunteer so this should go through some QA.
Note, momentarily I saw two toggles:


That said I haven't been able to reproduce this.

Change 445300 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Cached HTML/JS shouldn't show two toggle links

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

^ @Fomafix @matmarex not sure if this is necessary as cannot replicate but might want to merge this to be safe.

Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
stjn added a comment.Jul 11 2018, 9:28 PM

Didn’t look into a patch before, some small questions (not arguing against the patch or anything):

  1. Does it mean that now you should just check the checkbox if you are modifying the TOC with scripts (Ctrl+F ‘TOC hidden’ in Russian Wikipedia’s Common.js)?
  2. Is CSS-based solution entirely accessible? Here it says that IE11 doesn’t read CSS content.
matmarex added a comment.EditedJul 11 2018, 10:52 PM
  1. Is CSS-based solution entirely accessible? Here it says that IE11 doesn’t read CSS content.

I don't have Jaws or NVDA, but with Windows Narrator, it indeed doesn't read it. I am not sure if it is worth the effort to support this browser.

We should probably add role="button" to the <input type="checkbox"> though!

Here's how Windows Narrator reads it:

Old versionCurrent versionWith role="button" added
IE 11"'hide'; button" / "'show'; button""Unchecked checkbox""Off; button" / "On; button"
Edge"'hide'; button" / "'show'; button""Unchecked 'hide' checkbox" / "Checked 'show' checkbox""'hide'; button" / "'show'; button"

(added Jul 16: Firefox gives the same results as Edge; Chrome doesn't seem to work with Narrator)

  1. Does it mean that now you should just check the checkbox if you are modifying the TOC with scripts (Ctrl+F ‘TOC hidden’ in Russian Wikipedia’s Common.js)?

Yes. But you will need to keep the old code for a while and use it if the checkbox doesn't exist, because cached page HTML without it will continue to be used until it expires (in a week from the deployment? not sure).

Is CSS-based solution entirely accessible? Here it says that IE11 doesn’t read CSS content.

If we wanted to, I could see us addressing this by only using the CSS temporarily and disabling the pseudo selector of toctogglelabel and adding the text to the label in JS.

The problem from https://phabricator.wikimedia.org/T195053#4417392 is only present for 5 minutes until the cache for the style/script cache has expired and it only happens on new edited or purged pages which contain the new HTML.

To prevent this problem the toc.js change in https://gerrit.wikimedia.org/r/372515 can deployed in a prior release.

I think the double button problem is acceptable. It happens only for users, which make the following steps:

  1. User X loads a page before deployment. -> Style/CSS cache contains the old toc.js. This cache expires after 5 minutes
  2. Deployment
  3. User X or any other user edits or purges the page Y.
  4. User X loads the page Y. -> HTML contains the new code with a toggle button. The toc.js contains the old code from cache which also adds a toggle button. Two toggle buttons are visible.

Five minutes after step 1 the cache has expired and the problem can not occur anymore.

Unable to reproduce

ovasileva reassigned this task from ABorbaWMF to Ryasmeen.Jul 12 2018, 5:42 PM
ovasileva added a subscriber: ABorbaWMF.
Jdlrobson triaged this task as Normal priority.Jul 12 2018, 11:03 PM

I think the double button problem is acceptable. It happens only for users, which make the following steps:

  1. User X loads a page before deployment. -> Style/CSS cache contains the old toc.js. This cache expires after 5 minutes
  2. Deployment
  3. User X or any other user edits or purges the page Y.
  4. User X loads the page Y. -> HTML contains the new code with a toggle button. The toc.js contains the old code from cache which also adds a toggle button. Two toggle buttons are visible.

Five minutes after step 1 the cache has expired and the problem can not occur anymore.

Indeed. We support compatibility between the old HTML and the new CSS, but not the old CSS and the new HTML. We've never done that for other changes, either. I think it would be nice if we can solve that in some generic way, but at this time it wouldn't be worth the effort for those few minutes.

In theory, for this particular change, we could avoid it by deploying the stylesheet changes at least 5 minutes before the rest of the changes.

Jon's patch https://gerrit.wikimedia.org/r/445300 actually fixes the double button problem, although it's a bit unintuitive why (or perhaps a bit of a lucky coincidence). I tried to explain it on the patch. It doesn't even need to be deployed before other changes.

I think we should merge it, even though we usually ignore these five-minute issues: this is a relatively visible area, the patch is tiny and easy to support/remove later, and (most importantly ;) ) is has already been written and reviewed.

Change 446062 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] Set role=button on TOC show/hide checkbox

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

Volker_E added a subscriber: Volker_E.EditedJul 16 2018, 6:14 PM
  1. Is CSS-based solution entirely accessible? Here it says that IE11 doesn’t read CSS content.

I don't have Jaws or NVDA, but with Windows Narrator, it indeed doesn't read it. I am not sure if it is worth the effort to support this browser.
We should probably add role="button" to the <input type="checkbox"> though!

Updated test comparison table with VoiceOver/MacOS:

Old versionCurrent versionWith role="button" added
IE 11/Windows Narrator"'hide'; button" / "'show'; button""Unchecked checkbox""Off; button" / "On; button"
Edge/Firefox/Windows Narrator"'hide'; button" / "'show'; button""Unchecked 'hide' checkbox" / "Checked 'show' checkbox""'hide'; button" / "'show'; button"
Safari/VoiceOver"'hide'; button" / "'show'; button""hide, unchecked, checkbox" / "checked, show, checkbox""'hide'; button" / "'show'; button"
Chrome/VoiceOver"'hide'; button" / "'show'; button""hide, unchecked, tickbox" / "checked, show, tickbox""'hide'; button" / "'show'; button"

(added Jul 16: Firefox gives the same results as Edge; Chrome doesn't seem to work with Narrator)

Change 446062 merged by jenkins-bot:
[mediawiki/core@master] Set role=button on TOC show/hide checkbox

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

Krinkle added a comment.EditedJul 16 2018, 11:36 PM

Jon's patch https://gerrit.wikimedia.org/r/445300 actually fixes the double button problem, although it's a bit unintuitive why (or perhaps a bit of a lucky coincidence). I tried to explain it on the patch. It doesn't even need to be deployed before other changes.

If we deploy the new TOC and this patch at the same time, I expect it will do the following:

  • First 5 minutes:
    • A) Old HTML remains viewed with old JS/CSS module list, and old contents of those modules. Neither patch is in effect.
    • B) New HTML on recently saved pages will also load "mediawiki.toc.styles" module, changes the style url, bypasses cache, gets new CSS. Still has old JS for now.
  • After 5 minutes:
    • C) Old html gets newer version of "mediawiki.toc" JS and newer version of any CSS request it has (will not load "mediawiki.toc.styles").
    • D) New HTML gets newer version of "mediawiki.toc" JS.

As I understand it, the problem is that use case B (new HTML/CSS, old JS) will cause duplicate buttons to be created. I agree that this patch (updating the CSS) should prevent that.

EDIT: I mistakenly thought the patch adds CSS to mediawiki.toc.styles, but it adds it to mediawiki.toc (the JS module). See T195053#4429441.

However it seems @Fomafix has a different experience. I'm fine with merging, but I'd really like to get confirmation from @Fomafix, given this has been a major project that they have worked on for a year. So if @Fomafix prefers to revert it for now, or to think about it some more, I'd be inclined to let us do that. That way, it can also be a learning experience.

Fomafix added a comment.EditedJul 17 2018, 4:40 AM

I tested https://gerrit.wikimedia.org/r/445300 as cherry-pick again. It does not prevent double toggle buttons.

The style

.toctogglespan ~ .toctoggle {
	display: none;
}

in resources/src/mediawiki.toc.styles/screen.less instead of resources/src/mediawiki.toc/toc.css to be in the module mediawiki.toc.style instead of mediawiki.toc prevents the double toggle buttons.

New modules gets loaded immediately. Changed modules gets loaded up to 5 minutes later. Is this a bug in ResourceLoader?

Krinkle added a comment.EditedJul 17 2018, 4:54 AM

I tested https://gerrit.wikimedia.org/r/445300 as cherry-pick again. It does not prevent double toggle buttons.
The style

.toctogglespan ~ .toctoggle {
	display: none;
}

in resources/src/mediawiki.toc.styles/screen.less instead of resources/src/mediawiki.toc/toc.css to be in the module mediawiki.toc.style instead of mediawiki.toc would prevent the double toggle buttons.

Excellent. You're absolutely right. I wrongly thought Jon's patch adds CSS to "mediawiki.toc.styles", but the patch currently adds the CSS to "mediawiki.toc". Thus, re-reading T195053#4429164 shows that use case B would get the old JS still (and with that, the old CSS).

New modules gets loaded immediately. Changed modules gets loaded up to 5 minutes later. Is this a bug in ResourceLoader?

The difference is not between "new" or "changed" modules. The difference is between stylesheet modules (referenced by URL in the HTML), and dynamic JS/CSS modules (referenced by mw.loader).

This difference is known behaviour and something we choose in 2011. It was a compromise between atomic perfection and simplicity+usability+performance+scalability. In order to keep the system simple for developers, fast in terms of performance, and scalable for Wikipedia – we could not make guarantees between cache-roll of stylesheets and cache-roll of dynamic modules. To change this, would require a significantly more complex system, and we believed it could not perform fast enough for our scale. Or at least, that is what we believed in 2011. Perhaps we should try again :-)

Stylesheet module URLs are cached for 5 minutes. All URLs in older page cache will get new styles within 5 minutes from the same URL. All stylesheets on a single page always come from the same moment in time. ResourceLoader guarantees, that there can not be an older and newer versions mixed of stylesheet modules.

The manifest for mw.loader is also cached for 5 minutes. And any page, both older and newer will get a new manifest within 5 minutes. All dynamically loaded modules always come from the same moment in time. There is no mix of new/old changed modules.

These two systems are not related and will expire at different times. In the case of the TOC change, for newly edited pages, the stylesheet URL will expire first.

Change 445300 merged by jenkins-bot:
[mediawiki/core@master] Cached HTML/JS shouldn't show two toggle links

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

Thank you for looking into this, @Krinkle @Fomafix! I also missed that the styles were in the wrong module.

I think we're all done here. The only outstanding patch https://gerrit.wikimedia.org/r/445258 is kind of unrelated (the purpose of it is to keep the width of the TOC box when it is collapsed).

Looks good to me. I did notice that the [hide] button does not appear on IE8. I don't know if that is expected.

IE8


IE9

Edge

Safari

Firefox

Chrome

IE8 does not support :checked and therefor can not supported. The previous JavaScript-based implementation (mediawiki.toc) also did not support IE8 because for IE8 all JavaScript is disabled.

ovasileva closed this task as Resolved.Jul 19 2018, 10:37 AM
ovasileva added a subscriber: ovasileva.

looks good, thanks @ABorbaWMF!

Re: 68527cf47935

Can anyone check the new "show" or "hide" label on IE11 + screen reader? (I don't have an IE11 capable machine.)

IE11 + screen reader reportedly does not speak :before or :after generated content. So a user who tabs to the label after "Contents" might be getting silence instead of "show" or "hide" - a potential "empty form label" accessibility error.

Re: 68527cf47935
Can anyone check the new "show" or "hide" label on IE11 + screen reader? (I don't have an IE11 capable machine.)
IE11 + screen reader reportedly does not speak :before or :after generated content. So a user who tabs to the label after "Contents" might be getting silence instead of "show" or "hide" - a potential "empty form label" accessibility error.

@ABorbaWMF, @Ryasmeen - could one of you look into this?

@MattFitzpatrick We discussed it earlier on the task: Windows Narrator with IE 11 will unfortunately read the show/hide button only as "Off; button" / "On; button". I don't know if more advanced screen readers can handle it better. With Edge, it works better, being read as "'hide'; button" / "'show'; button".

  1. Is CSS-based solution entirely accessible? Here it says that IE11 doesn’t read CSS content.

I don't have Jaws or NVDA, but with Windows Narrator, it indeed doesn't read it. I am not sure if it is worth the effort to support this browser.
We should probably add role="button" to the <input type="checkbox"> though!
Here's how Windows Narrator reads it:

Old versionCurrent versionWith role="button" added
IE 11"'hide'; button" / "'show'; button""Unchecked checkbox""Off; button" / "On; button"
Edge"'hide'; button" / "'show'; button""Unchecked 'hide' checkbox" / "Checked 'show' checkbox""'hide'; button" / "'show'; button"

(added Jul 16: Firefox gives the same results as Edge; Chrome doesn't seem to work with Narrator)

Change 481541 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/core@master] Revert "Cached HTML/JS shouldn't show two toggle links"

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

Change 481541 merged by jenkins-bot:
[mediawiki/core@master] Revert "Cached HTML/JS shouldn't show two toggle links"

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