Page MenuHomePhabricator

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

Description

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

Screenshot_2016-09-01-21-43-20.png (854×480 px, 45 KB)

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

Screen Shot 2016-09-27 at 4.04.56 PM.png (305×1 px, 42 KB)

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:

Screen Shot 2016-10-03 at 17.14.23.png (564×722 px, 61 KB)

and not like this:
Screen Shot 2016-10-03 at 17.14.30.png (340×708 px, 26 KB)

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:

Screen Shot 2016-10-03 at 4.40.44 PM.png (335×1 px, 94 KB)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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

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

Screenshot_2016-09-02-13-53-50.png (854×480 px, 75 KB)

@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

@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 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:

Screen Shot 2016-09-28 at 1.44.08 PM.png (466×411 px, 64 KB)

(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

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.

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

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.

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)

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.

iPhone 6 iOS 9.3.2.PNG (1×640 px, 53 KB)

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.

ios.gif (686×372 px, 791 KB)

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.

ios.gif (686×372 px, 791 KB)

@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.