Page MenuHomePhabricator

[Visual refinements] Table of contents overlaps the footer or page boundary depending on article length
Closed, ResolvedPublic5 Estimated Story PointsBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

Screen Shot 2022-07-14 at 9.23.49 AM.png (1×2 px, 702 KB)

What happens?:
The table of contents overlaps the footer.

What should have happened instead?:

Maybe this is fine, but perhaps we might expect the table of contents to stop being fixed and sit above the footer?

Software version (skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson renamed this task from Table of contents overlaps the footer to Table of contents overlaps the footer or page boundary depending on article length.Jul 19 2022, 7:37 PM
Jdlrobson added a subscriber: MilkyDefer.

This is happening because the TOC container is sticky positioned and it's a sibling to the footer. The most straightforward way to fix this is to move the TOC or the footer so that they don't occupy the same container (mw-page-container-inner). However this would impact our existing grid implementation, which uses mw-page-container-inner as the grid container in order to position the header, TOC, main menu, content, and footer.

The simplest solution could be to remove the footer and header from mw-page-container-inner as they are both always full width anyway, and don't benefit much from grid.

Since this involves DOM and grid changes anyway, this could be a good opportunity to rework the grid so that the <main> becomes the grid container. Rather than moving the header and footer out of mw-page-container-inner, we move the TOC and main menu into <main>. This would give us more options to position elements in the main content (i.e. page titlebar, page toolbar), and would solve a few issues at once including T308055 and T310388 (fixes without needing grid). The solution would be similar to this POC patch, but without using display: contents

@bwang, even simpler, I think we can just move .mw-footer-container outside the grid as a sibing of .mw-page-container.vector-layout-grid. This seems less risky as the header is somewhat using the grid since the elements inside it need to align with the elements in the content.

I don't think we need to touch the header? What do you think?

@Jdlrobson The elements in the header need to align with the elements in the content, but this done entirely without grid. All the grid does is ensure the header sits in the first row and spans all the columns. Even if we wanted to use grid to align the elements inside the header in the future, we would have to define a new grid container, so the header itself doesn't need to be in mw-page-container-inner.

To me it just seems a bit clunky and arbitrary to only move the mw-footer-container out of mw-page-container-inner. Moving both limits our grid implementation to only elements that need it, and it hopefully makes our DOM structure more logically consistent. It would also allow us to simplify grid styles down the line, because we can use the main menu checkbox is a sibling of the grid container rather than a child. I believe the only risk with moving the header is that we need to also move the main menu checkbox and update the CSS selectors that rely on the main menu state.

ovasileva raised the priority of this task from Medium to High.Aug 15 2022, 5:49 PM

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

[mediawiki/skins/Vector@master] Move Footer out of grid container

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

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

[mediawiki/skins/Vector@master] Move Header from grid container

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

@Jdlrobson Looking at this now, cached HTML does make moving the header out of the grid container slightly more annoying than I thought, I'm thinking it could be better to file a ticket for moving the header for now.

Edit: Heres the task T315598

bwang removed bwang as the assignee of this task.Aug 18 2022, 5:36 PM
bwang moved this task from Doing to Code Review on the Web-Team-Backlog (Kanbanana-2022-23-Q1) board.

Change 824566 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Table of Contents: Move sticky positioning to sidebar-toc

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

Looked into this some more. The issue seems to be the fact that .mw-table-of-contents-container is the sticky element and that is a sibling of the footer.
To remedy this I'm pretty sure we can move the sticky positioning to .sidebar-toc

Following up from grooming today, after taking another another look at the approaches I think that moving the footer out of the grid is the simplest solution. It does involve DOM changes, but due to the fact that the grid doesn't apply any styles to the footer, it's very simple to handle cached HTML (only 1 additional line of CSS). It's also quite easy to test cached HTML locally so the risk should be low.

The solution of [[ https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/824566 | moving the mw-sticky-header-element ]] still has some spacing issues to be worked out, and it would likely involve reworking the TOC styles which are fragile and complicated. I'm not confident that approach will work without incurring more CSS complexity and/or visual changes.

I'm still trying to collect my thoughts around this, but the more I think about it, the more moving footer/header out of the grid seem counter-intuitive. We added the grid to control all layout partly because Alex wanted more flexibility/alignment between footer/header elements and content.

The position: fixed issue is easier for me to wrap the head around - the footer is a sibling of a sticky element. We need to fix that.

My hunch is we're just using grid wrong and need to do some more reading about it.

Let's find some time to talk through this next week (I'm out Thursday and am unlikely to find some time tomorrow and want to do some more reading around grid based layouts).

Gotcha, yeah it would help to chat next week. In the meantime I'll try exploring a solution where we move the sticky styles to a new container div, and hopefully avoid reworking TOC CSS

<div class="mw-table-of-contents-container">
	<div class="mw-sticky-header-element">
              {{#data-toc}}{{>TableOfContents}}{{/data-toc}}
        </div>
</div>

Change 824265 abandoned by Bernard Wang:

[mediawiki/skins/Vector@master] Move Footer out of grid container

Reason:

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

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

[mediawiki/skins/Vector@master] Create new sticky toc container

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

bwang removed bwang as the assignee of this task.Aug 30 2022, 6:53 PM

Change 824566 abandoned by Jdlrobson:

[mediawiki/skins/Vector@master] Table of Contents: Move sticky positioning to sidebar-toc

Reason:

See https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/826605

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

Change 826605 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Create new sticky toc container

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

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

[mediawiki/skins/Vector@master] Followup: Removed code for cached HTML from T313060

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

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

[mediawiki/skins/Vector@master] Followup: Removed code for cached HTML from T313060

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

Test Result - Beta|Prod

Status: ✅ PASS
Environment: beta/xyzwiki
OS: macOS Monterey
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

Visit https://en.wikipedia.beta.wmflabs.org/wiki/Dog
Scroll to the bottom of the page
Screen Shot 2022-07-14 at 9.23.49 AM.png (1×2 px, 702 KB)
✅ AC1: The table of contents should not overlap the footer.

Screen Recording 2022-09-06 at 2.23.32 PM.mov.gif (608×632 px, 839 KB)

Change 831160 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Don't print table of contents on page with no table of contents

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

Pixel flagged an issue on pages where there is no table of contents e.g. Special:BlankPage there is now an additional padding.

Change 831160 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Don't print table of contents on page with no table of contents

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

Change 829044 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Followup: Removed code for cached HTML from T313060

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

Jdlrobson renamed this task from Table of contents overlaps the footer or page boundary depending on article length to [Visual refinements] Table of contents overlaps the footer or page boundary depending on article length.Sep 13 2022, 11:33 PM

Looking good, resolving