Page MenuHomePhabricator

[Zero] Banner broken occasionally on article link tap
Closed, ResolvedPublic

Description

As reported by @DFoy

Repro steps courtesy of @DFoy:

Steps to reproduce banner issue on ios9 (chrome and safari). May exist on Android - untested.

  • Add your phone IP to the partner configuration, wait for ~10 min for propagation. I used MTN Ghana (620-01).
  • Navigate to "m.wikipedia.org" in the browser.
  • The first article screen seen will have the banner in the correct position.
  • Click a link in the article to another Wikipedia article.
  • The second article (and all subsequent articles) will show the banner at the bottom of the screen (behind the UI). Scroll down to see the banner embedded in the middle of the article.

Event Timeline

dr0ptp4kt assigned this task to jhobs.
dr0ptp4kt raised the priority of this task from to Needs Triage.
dr0ptp4kt updated the task description. (Show Details)
dr0ptp4kt added a project: Zero.
dr0ptp4kt moved this task to Development in progress on the Zero board.
dr0ptp4kt added subscribers: dr0ptp4kt, Yurik, DFoy.
dr0ptp4kt set Security to None.

Steps to reproduce banner issue on ios9 (chrome and safari). May exist on Android - untested.

  1. Add your phone IP to the partner configuration, wait for ~10 min for propogation. I used MTN Ghana (620-01).
  2. Navigate to "m.wikipedia.org" in the browser.
  3. The first article screen seen will have the banner in the correct position.
  4. Click a link in the article to another Wikipedia article.
  5. The second article (and all subsequent articles) will show the banner at the bottom of the screen (behind the UI). Scroll down to see the banner embedded in the middle of the article.

Have been unable to reproduce in Chrome browser emulator for either Android or iOS devices so far. I'll have access to an actual iOS device later this evening that should help in case I can't get it by then. It's possible this could be some kind of iOS 9 issue (I've heard of several things breaking since the latest OS update).

Ok so I've been able to reproduce this on an Android device now, but only the first article I navigate to via an in-article link has the problem (not subsequent ones). The banner appears to always be rendered just below the viewport, as is evident when I scroll away the address bar and the banner shifts the same distance as the address bar's height.

While typing this comment I actually confirmed my statement above as I was finally able to reproduce it on a computer (no idea why doing the same steps suddenly worked) and found that the HTML for the banner is located directly below the div with id "mw-mf-viewport". Given the way this is rendering and the fact that I can't reproduce it with 100% consistency (cached pages seem to play a factor), I'm thinking potentially something is being loaded incorrectly by ResourceLoader or something.

It looks like this only happens when RL complains about a deprecation warning with the line "document.write". I'll try using jQuery or mw.loader.load instead, but I'm unsure what the effects of this might be on limited-javascript browsers. Any knowledge you could drop here @dr0ptp4kt or @Yurik? Probably worth bringing @Jdlrobson in on this task too since it appears to be related to MFE & RL.

@dr0ptp4kt sent this via email, logging here for transparency:

Timo (Krinkle) had emailed wikitech-l in August about a deprecation of document.write with subject '[Wikitech-l] [BREAKING CHANGE] Use of "document.write" no longer supported'. See the archive for details. I believe there's at least one necessary use of document.write in W0 (outside of RL IIRC) to accommodate weak JS support. It's unclear to me whether the current bug is is manifesting due to that (e.g., if it is in JS served by RL or if they've overridden document.write.prototype in some fashion) or if it's something else, but thought I'd share this info anyway.

Quick update on this: I am unable to reproduce this on my local machine, which does not show console deprecation warnings for some reason. Because of this I'm almost certain the bug is related to the warning (specifically the document.write one), but because I can't replicate it locally, I'll have no idea if any patch I've written has fixed it until it's already been deployed (which is obviously a Very Bad Idea). Going to look to pair with another engineer or two to get some fresh eyes on it.

Change 242661 had a related patch set uploaded (by Jhobs):
Fix deprecation warnings

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

I'm still uncertain if the patch set above will fix the issue, but at the very least it fixes some deprecation issues that are about to be deployed on Thursday, so I think it's safe to rush deploy if nobody finds any issues with it. @Yurik

jhobs renamed this task from Banner broken on external link on iOS to [Zero] Banner broken on external link on iOS.Sep 30 2015, 8:01 PM
jhobs renamed this task from [Zero] Banner broken on external link on iOS to [Zero] Banner broken occasionally on article link tap.

Change 242661 merged by jenkins-bot:
Fix deprecation warnings

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

@Yurik has committed to lightning deploying this on Monday.

@Yurik can you confirm you will be lightning deploying this today? I don't see anything on the calendar.
We've seen lots of issues in the past week around the mobile site being broken. Is there any possibility that Zero code could be leaking
into the normal site and breaking things?

Change 243801 had a related patch set uploaded (by Yurik):
Fix deprecation warnings

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

Change 243801 merged by jenkins-bot:
Fix deprecation warnings

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

Not seeing any errors with X-Subdomain=M and X-CS=Test
I see "You are about to leave Zero-rated (free) Wikipedia. Standard data charges may apply." when I click an external link (btw that needs some design love on tablet)

@DFoy anything else you need before I sign this off?

I did some live testing last night, and the exit interstitial placement and missing banner bugs are resolved. @jhobs will add an interstitial header text override ability but it doesn't seem to fall in the scope of this Phabricator task.