Page MenuHomePhabricator

Fix collapsed TOC menu position when site notice is present
Closed, ResolvedPublic3 Estimated Story PointsBUG REPORT

Description

List of steps to reproduce (step by step, including full links if applicable):

  • Create a site notice, or manually put text inside #siteNotice
  • Collapse the TOC by pressing the "hide" button or by reducing the view
  • Click the TOC button

What happens?:

Screen Shot 2022-06-10 at 1.52.03 PM.png (912×1 px, 270 KB)

What should have happened instead?:

Screen Shot 2022-06-10 at 1.58.53 PM.png (968×1 px, 227 KB)

Developer notes

Introduced with approach taken in T308689
POC for the fix
https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/804616/

Event Timeline

I put up a POC fix with an approach that basically does T308055, and separates the site notice from the rest of the page content. Another solution could be to rework the approach for the collapsed TOC altogether, and duplicate the TOC in the DOM (i.e. https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/794029).

@bwang the approach of moving the ToC after the page title makes sense to me. I see how the collapsed ToC has to be absolutely positioned at narrow breakpoints (to appear as a dropdown menu), so it's needs a parent element such that it can be positioned with a stable top value (e.g. 44px, if that's the height of the ToC button). Duplicating the ToC is not a crazy idea either, but that would be an entire rework of this feature, so I wouldn't recommend it 😉.

Banners are also JavaScript so we could do a banner height calculation to change the top calculation on the menu when a banner is present.

I realized I missed another option, which is to simply move the site notice out of the main element and above the TOC. This would mean the site notice is no longer part of the main landmark, but this might not be such a bad thing as the content isnt related to the page. Doing so would also mean the site notice spans the entire width of the page, and there may be implications on how easily accessible the notice is because it's no longer after the skip link and the main landmark. I'm not sure how important that is, but if fundraising banners go there that might be tricky.

Banners are also JavaScript so we could do a banner height calculation to change the top calculation on the menu when a banner is present.

I was under the impression that this had to work without JS, given the collapsed TOC on narrow screens is a no-JS feature, is that not the case?

@bwang The collapsed TOC does yes, however since banners/site notices are sometimes a JS only feature and are rare ie. not shown on every page view all the time using JS could be an acceptable solution here that's worth considering

ovasileva raised the priority of this task from Medium to High.Jul 1 2022, 1:54 PM

This still seems to be an issue when screen width is below 1000px, but interestingly not when I manually collapse the TOC by clicking the "Hide" button.

https://en.wikipedia.beta.wmflabs.org/wiki/Polar_bear?banner=CommunityBanner

en.wikipedia.beta.wmflabs.org_wiki_Polar_bear_banner=CommunityBanner.png (1×1 px, 539 KB)

This still seems to be an issue when screen width is below 1000px, but interestingly not when I manually collapse the TOC by clicking the "Hide" button.

Ah that's correct, this is because T312749 only resolves the issue when we use grid, which is disabled on smaller resolutions.

Change 822171 had a related patch set uploaded (by Jdrewniak; author: Jdrewniak):

[mediawiki/skins/Vector@master] [POC] CSS Grid for mobile layout

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

Test wiki created on Patch demo by JDrewniak (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/11d86bb4ee/w

Ah that's correct, this is because T312749 only resolves the issue when we use grid, which is disabled on smaller resolutions.

In that case, enabling grid on smaller resolutions could be a potential fix. @Jdlrobson was a narrow-screen CSS grid layout just out of scope for the initial grid work? Or are there other considerations we should take into account? (browser support?)

I wrote a patch and demo exploring what a mobile-friendly CSS grid would look like. It does solve the siteNotice issue by providing the dropdown TOC it's own grid-area: https://patchdemo.wmflabs.org/wikis/11d86bb4ee/wiki/Hot_dog?useskin=vector-2022

@Jdrewniak please test in IE11. https://patchdemo.wmflabs.org/wikis/11d86bb4ee/wiki/Hot_dog?useskin=vector-2022 - as written that would break the display of the sidebar. The reason we're not using grid at lower resolutions would be to avoid this issue.
{F35423245}

Another challenge here is banners can technically insert themselves absolutely anywhere, so while this might work for the initial case, it won't work all the time.

Couldn't we just use JS here and inside the click handler for the button set the "top" value of the button based on document.getElementById('vector-toc-collapsed-button').offsetTop ?

Yesterday @Jdlrobson explained to me how the narrow-width layout on Vector 2022 is serving a dual purpose: providing a single column layout for narrow screens and equally the layout for grade-C browsers. If we were to introduce CSS Grid for the narrow layout, we would have to develop a third layout for grade C browsers (which is not out of the question, but it's better suited for T313066).

Taking that into account, I think the JS approach Jon outlines above would be a sufficient fix here.

I just realized we missed a super simple solution that doesn't require JS or grid on small screens 🤦. All we need to do is move the site notice anywhere above mw-table-of-contents-container (which makes more sense anyway), and then the TOC menu's absolute positioning will be accurate.

POC here: https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/823196

Change 823196 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] POC: fix collapsed TOC menu positioning with site notice without JS or major CSS changes

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

Test wiki created on Patch demo by JDrewniak (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/98609a01e3/w

👆 @bwang that looks like a much better approach!

Ok! then ill just reuse the patch above for the actual fix https://gerrit.wikimedia.org/r/823196

Change 823196 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Fix collapsed TOC menu positioning with site notice without JS or major CSS changes

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

ovasileva subscribed.

Looks like this was already tested in production and passes. Resolving.

Edtadros subscribed.

Change 822171 abandoned by Jdrewniak:

[mediawiki/skins/Vector@master] [POC] CSS Grid for mobile layout

Reason:

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

Test wiki on Patch demo by JDrewniak (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/11d86bb4ee/w/

Test wiki on Patch demo by JDrewniak (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/98609a01e3/w/