Page MenuHomePhabricator

[Bug] Max-Width Layout: Sidebar overlaps footer when its height is longer than the content height
Closed, ResolvedPublic5 Estimated Story PointsBUG REPORT

Description

Steps to reproduce

Visit https://en.wikipedia.beta.wmflabs.org/wiki/Test

Expected results

When sidebar is longer than content, it pushes the footer down resulting in no overlap.

Actual results

When sidebar is longer than content, sidebar gives no care in the world and floods the footer. It is also possible for the sidebar to be cut off (by .mw-page-container's overflow: hidden).

Test.png (1×3 px, 523 KB)

Environments observed

Developer Notes

Why does this happen?
The problem occurs because the sidebar is absolutely positioned, and therefore cannot push the footer down with its height. Note that the sidebar is also absolutely positioned in legacy Vector, but the overlap does not occur with the footer because the footer is made to be the width of the content. Therefore, the sidebar has its own "lane" and never touches the footer.

Why is the sidebar absolutely positioned?
In an effort to maintain our DOM order for accessibility reasons (T240489), the content continues to be ordered before the header and before the sidebar. Therefore, both the header and sidebar are absolutely positioned in order to accommodate this constraint and make these elements visually appear before the content. I'm not sure if there is any other way to meet this constraint without absolutely positioning these elements.

What is the fix?
There are several ways we could go about this each one with its own suck factor. Other suggestions are welcome:

Make the content have a min-height of some arbitrary value x or make it have a min-height of the viewport height:

Potential problems:

  • This puts a bandaid on the problem and may mitigate its frequency, but we are really just guessing/hoping that the sidebar will not be longer than that x height value. The sidebar's height is not within our control and may still exceed x.

Change the design: Make the footer width match the content width. This might look something like:

footer-same-width-as-content.png (1×4 px, 285 KB)

Potential problems:

  • Are we okay with the sidebar extending past the footer? (This solution would not address that).
  • What do we do about the grey area (e.g. if the footer spills into the grey area). Do we remove the grey and replace with white?
  • The mw-page-container currently has an overflow: hidden style applied to it to hide the sidebar when it is collapsing. If we keep the style then the sidebar could also be hidden if it extends past the page container. What to do about this?
  • POC: https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/610468 (but above concerns/questions still stand)

Change the DOM order: Move the sidebar next to the content and remove its absolute positioning

Potential problems:

  • Are we okay moving the DOM order around and accepting whatever effect it has on accessibility (where sidebar might be ordered before content)? There was a ton of time spent on T246420 trying to maintain/achieve a DOM order that kept the content above the header/sidebar with this consideration in mind and a lot of time carefully thinking about the tab order and trying to make it work with the checkbox hack. If we go this route, will probably need to have these discussions again and T240489 may very well be a blocker for this task.
  • Even if we go this route, what do we use to position the sidebar next to the content? Floats? Flexbox? I made a POC of what using a floated sidebar might look like. It seemed to work mostly well, but I would need to look into addressing a bug when sidebar is closed and viewport width is small.
  • POC: https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/610458 (but need to address a bug with sidebar closed at small widths)

QA Steps

With > 1750 pixel viewport width

  1. Visit https://en.wikipedia.beta.wmflabs.org/wiki/Test as an anon. Resize your browser to > 1750px
  2. Verify that the footer width matches the content width.
  3. Click the sidebar button to show the sidebar.
  4. Verify that the footer width matches the content width.
  5. Verify that the sidebar is NOT overlapping the footer and all the text in the sidebar and in the footer is legible.
  6. Verify that the sidebar is not cut off. The sidebar may extend past the white part of the page and into the grey. That is okay for now.
  7. Verify that all of the links in the sidebar are clickable.

With 1000 pixel viewport width

  1. Visit https://en.wikipedia.beta.wmflabs.org/wiki/Test as an anon. Resize your browser to 1000px.
  2. Verify that the footer width matches the content width.
  3. Click the sidebar button to make the sidebar appear.
  4. Verify that the footer width matches the content width.
  5. Verify that the sidebar is NOT overlapping the footer and all the text in the sidebar and in the footer is legible.
  6. Verify that the sidebar is not cut off. The sidebar may extend past the white part of the page and into the grey. That is okay for now.
  7. Verify that all of the links in the sidebar are clickable.

QA Results - Beta

Event Timeline

nray triaged this task as High priority.Jul 8 2020, 8:48 PM
nray created this task.

Change 610458 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/Vector@master] POC: Move Sidebar next to content

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

Change 610458 abandoned by Nray:
[mediawiki/skins/Vector@master] POC: Move Sidebar next to content

Reason:
Just putting this here as a demo. Not really review worthy

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

nray updated the task description. (Show Details)

Change 610468 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/Vector@master] POC: Make footer match content width

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

Change 610468 abandoned by Nray:
[mediawiki/skins/Vector@master] POC: Make footer match content width

Reason:

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

nray renamed this task from [Bug] Max-Width Layout: Sidebar spills into footer when its height is longer than the content height to [Bug] Max-Width Layout: Sidebar overlaps footer when its height is longer than the content height.Jul 9 2020, 4:38 PM
nray updated the task description. (Show Details)
nray updated the task description. (Show Details)

The mw-page-container currently has an overflow: hidden style applied to it to hide the sidebar when it is collapsing. If we keep the style then the sidebar could also be hidden if it extends past the page container. What to do about this?

@nray can you explain again why .mw-page-container has overflow:hidden? In what situation is this required? Removing it doesn't seem to cause any horizontal scrollbar that I can see.

I ask because, IMO, removing that rule and having the footer match the content-width seem like and acceptable solution to me. I don't see how the sidebar extending below the footer is an issue, since that's what legacy Vector does.

@Jdrewniak The overflow: hidden is solely for the sidebar animation (when toggled off) so that the sidebar gets hidden by mw-page-container. We could probably tweak the sidebar opacity transition so that it is 0 before it even reaches the mw-page-container though and get rid of this style.

Change 610468 restored by Nray:
[mediawiki/skins/Vector@master] POC: Make footer match content width

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

Change 610468 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Max-Width Layout: Make footer width match content width to avoid overlap with sidebar

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

Change 612640 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/Vector@master] Max-width layout: Make page container fill viewport if content height is small

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

nray updated the task description. (Show Details)

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps
With > 1750 pixel viewport width
  1. Visit https://en.wikipedia.beta.wmflabs.org/wiki/Test as an anon. Resize your browser to > 1750px
  2. ✅ AC1: Verify that the footer width matches the content width.

Screen Shot 2020-07-14 at 7.03.04 PM.png (684×1 px, 161 KB)

  1. Click the sidebar button to show the sidebar.
  2. ✅ AC2: Verify that the footer width matches the content width.

Screen Shot 2020-07-14 at 7.04.27 PM.png (717×1 px, 203 KB)

  1. ✅ AC3: Verify that the sidebar is NOT overlapping the footer and all the text in the sidebar and in the footer is legible.
  2. ✅ AC4: Verify that the sidebar is not cut off. The sidebar may extend past the white part of the page and into the grey. That is okay for now.
  3. ✅ AC5Verify that all of the links in the sidebar are clickable. I clicked them all! They all worked.
With 1000 pixel viewport width
  1. Visit https://en.wikipedia.beta.wmflabs.org/wiki/Test as an anon. Resize your browser to 1000px.
  2. ✅ AC6: Verify that the footer width matches the content width.

Screen Shot 2020-07-14 at 7.08.51 PM.png (683×986 px, 142 KB)

  1. Click the sidebar button to make the sidebar appear.
  2. ✅ AC7: Verify that the footer width matches the content width.

Screen Shot 2020-07-14 at 7.10.21 PM.png (769×991 px, 205 KB)

  1. ✅ AC8: Verify that the sidebar is NOT overlapping the footer and all the text in the sidebar and in the footer is legible.
  2. ✅ AC9: Verify that the sidebar is not cut off. The sidebar may extend past the white part of the page and into the grey. That is okay for now.
  3. ✅ AC10: Verify that all of the links in the sidebar are clickable. All links in the sidebar are clickable.

@nray you are the Michael Jordan of test steps.

Change 612640 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Max-width layout: Make page container fill viewport if content height is small

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

ovasileva subscribed.

Looking good on production, resolving!

Screen Shot 2020-07-20 at 9.07.22 AM.png (990×2 px, 310 KB)

Change 614771 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/Vector@wmf/1.35.0-wmf.41] Max-width layout: Make page container fill viewport if content height is small

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

Change 614771 abandoned by Nray:
[mediawiki/skins/Vector@wmf/1.35.0-wmf.41] Max-width layout: Make page container fill viewport if content height is small

Reason:
We decided to let this ride the train instead

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