Page MenuHomePhabricator

Beta footer appears obscured on diff pages
Closed, ResolvedPublic2 Story Points

Description

See the screenshot from @Issimo_15's report (T141002#2603329):

This should be fixed by adding space at bottom of page so that the footer can be scrolled past. The height of sticky element should equal extra bottom padding

QA Plan

Test should hold true for any browser in the list https://www.mediawiki.org/wiki/Reading/Web/QA_device_and_browsers_list

Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/265240?mobileaction=beta and make sure that the page footer is not covered by the user information. When you scroll down the page you should be able to see the whole page footer and then the user information box should appear.

Basically, it should look like this:


and not like this:

Visit a few other pages (ensuring you have opted into the beta mode of the site and it should not be possible to scroll beyond the footer. The terms of use should always sit at the bottom of the page like so:

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 2 2016, 7:49 AM
phuedx renamed this task from Beta footer appears obscured on diff pages to [SUBTASK] Beta footer appears obscured on diff pages.Sep 2 2016, 7:52 AM
Issimo_15 added a comment.EditedSep 2 2016, 12:57 PM

Also, as you can see in this screenshot, a part of the editor took the footer's color.

Jdlrobson added subscribers: Jdlrobson, ovasileva, bmansurov and 7 others.

@Nirzar how do you want to deal with this? We have lots of custom styling on special pages and it would be great to unify these designs

@Nirzar @ovasileva This should actually be a subtask of pushing the footer change to stable.

phuedx renamed this task from [SUBTASK] Beta footer appears obscured on diff pages to Beta footer appears obscured on diff pages.Sep 19 2016, 4:20 PM
MBinder_WMF set the point value for this task to 2.Sep 19 2016, 4:27 PM
Yurik removed a subscriber: Yurik.Sep 21 2016, 3:05 PM
Jdlrobson assigned this task to Nirzar.EditedSep 26 2016, 5:45 PM

@Nirzar I repeat my question:
https://phabricator.wikimedia.org/T144579#2605365

Moving back to needs analysis till we have that answer.
I think we can solve this in many different ways:

  1. Move the diff information into the footer

1B) ... and making the footer fixed for that page.

  1. Stop the diff footer being position fixed
  2. Add JS which turns off position fixed when you scroll to footer.
  3. Adding space at bottom of page so that the footer can be scrolled past (diff toolbar sits above it)

(Each have varying levels of difficulty)
I'm surprised this was estimated as 2 and put as ready for dev.

Adding space at bottom of page so that the footer can be scrolled past (diff toolbar sits above it)

This should be the fix for this issue. If we have sticky elements,
height of sticky element = extra bottom padding

Jdlrobson removed Nirzar as the assignee of this task.Sep 27 2016, 11:03 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

The recently added mock by @Jdlrobson looks good.

#mw-mf-userinfo has min-height: 5em; and 1em padding
It's not a border-box element.

It seems we use min-height as the user groups can spill onto a second group (e.g. https://en.m.wikipedia.org/wiki/Special:MobileDiff/741615580)

I think the best solution is probably to add a class to the body element whenever a fixed footer is being used e.g. feature-fixed-footer.

When this is present:

  • Restrict #mw-mf-userinfo to 6em height
  • add a margin-bottom: 8em; to footer
  • Remove bottom margin on mw-mf-diffarea

This means on certain pages, the footer information may get cut off. I'm not sure how important that is however:

(I'm not planning on working on this today but if the above sounds sane go for it!)

The approach in T144579#2675497 seems fine to me. Alternatively, we could add the styles directly for each page that is affected by it, but I think the tradeoff of a few extra bytes for a new class is worth following DRY principles (and future-proofing it).

fyi @ovasileva @Jhernandez @bmansurov if we don't ship this today this is going to push back release of related articles by a week.

I see the space is already reserved after the diff content. I suggest we make the sticky element stick to the bottom of the content and not to the bottom of the page. @Nirzar, would you be OK if we implemented this suggestion? Otherwise, it is confusing that the footer comes before this sticky element with the proposed solution.

@Jdlrobson - I think it's best to remove this as a blocker to pushing the footer to stable - we can go back and fix it later

Nirzar added a comment.Oct 3 2016, 8:07 PM

talked to @bmansurov i think we should go ahead with margin fix that is proposed here. this is a blocker for new footer and footer is blocker for related pages. we can come back to improving diff pages later. for now. the proposed solution should work.

Jdlrobson moved this task from Doing to To Do on the Reading-Web-Sprint-82-Xpect-Rspec board.

Position fixed is always positioning an element relative to the window. You can position it absolutely to the bottom of the page but it will not stay there when scrolled. Can you clarify what you mean @bmansurov ?

Never mind, let's go with the original solution. I was thinking of adding absolute positioning the the element in question and adding the element itself after the content but before the footer. It may not work. ;)

Change 313895 had a related patch set uploaded (by Jdlrobson):
Should be possible to scroll to footer on diff page

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

Jdlrobson claimed this task.Oct 3 2016, 8:59 PM

Not the most amazing solution but it should be good enough for pushing the new footer to stable. Reviews welcomed so we can switch focus to enabling Related Articles.

bmansurov updated the task description. (Show Details)Oct 3 2016, 9:15 PM
bmansurov moved this task from Code Review to Needs QA on the Reading-Web-Sprint-82-Xpect-Rspec board.

Change 313895 merged by jenkins-bot:
Should be possible to scroll to footer on diff page

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

Jdlrobson updated the task description. (Show Details)Oct 3 2016, 11:41 PM
phuedx updated the task description. (Show Details)Oct 4 2016, 9:23 AM
Jdlrobson updated the task description. (Show Details)Oct 4 2016, 4:39 PM
Jdlrobson updated the task description. (Show Details)

I tested this on multiple browsers but came across an issue on iOS 9 Safari (iPhone 6 iOS 9.3.2) as seen in the screencap below the page footer bounces right back under the user information when I scrolled to it.

Jdlrobson removed Jdlrobson as the assignee of this task.Oct 5 2016, 11:13 PM
jhobs claimed this task.Oct 7 2016, 4:01 PM
jhobs moved this task from To Do to Doing on the Reading-Web-Sprint-82-Xpect-Rspec board.
jhobs removed jhobs as the assignee of this task.EditedOct 7 2016, 4:06 PM
jhobs moved this task from Doing to To Do on the Reading-Web-Sprint-82-Xpect-Rspec board.

I've been working on this task all of this morning (forgot to update Phabricator until a few minutes ago) and have so far only been able to reproduce the iOS 9 problem once. The one time I reproduced it, I couldn't tell what was going on that was causing it or how to fix it. I'm going to throw this one back up for grabs in case anyone has some iOS 9 ideas I'm unaware of.

@jhobs, I can reproduce it 100% of the time. Just make sure the iOS version is 9.3 (I haven't tried others) and visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/265240?mobileaction=beta. Then scroll to the bottom of the page. That's it.

@bmansurov can you try looking at it then? I just tried again and it's not happening for me in iOS Simulator, version 9.3 selected.

@jhobs, here is what I have. I'm just curios why this is not happening on your end.

bmansurov claimed this task.Oct 7 2016, 7:13 PM
bmansurov moved this task from To Do to Doing on the Reading-Web-Sprint-82-Xpect-Rspec board.

I'll upload the patch as soon as gerrit is up again.

Change 314747 had a related patch set uploaded (by Bmansurov):
Get rid of footer margin collapsing on Special:MobileDiff

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

Change 314747 merged by jenkins-bot:
Get rid of footer margin collapsing on Special:MobileDiff

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

@jhobs, here is what I have. I'm just curios why this is not happening on your end.

@bmansurov Just following up on this: it turns out my version of iOS Simulator was out of date, which was somehow causing this to not be reproducible (although I'm baffled as to why). I updated it to the latest and was able to reproduce it on 9.3. Thanks for fixing it up while I sorted out those issues Baha.

Testing again on iOS 9 Safari (iPhone 6 iOS 9.3.2), this time i followed the link @jhobs said above and now this is fixed.

ovasileva closed this task as Resolved.Oct 12 2016, 9:15 AM
bmansurov removed bmansurov as the assignee of this task.Oct 12 2016, 7:09 PM