Page MenuHomePhabricator
Create Task
Maniphest T9969

Differential 1-up render doesn't trigger on iOS
Closed, ResolvedPublic
Actions

Description

I have an iPhone 6+s on which I like to punish myself with by reviewing diffs. In general it works out pretty well. However tapping the 'show 20 lines) or 'Show all N lines' links results in a unusable state.

The expanded lines are rendered with a single character per line.

before: http://imgur.com/5ChqV6A
expanded: http://imgur.com/bifmSBm

I apologize for the imgur links, but there doesn't seem to be a way to upload files from a mobile device.

Revisions and Commits

rP Phabricator
D15135rPdbf1d0d7210f Improve reliability of detecting small devices when loading Differential…

Related Objects

Event Timeline

starruler created this task.Dec 12 2015, 1:40 AM
starruler raised the priority of this task from to Needs Triage.
starruler updated the task description. (Show Details)
starruler changed the edit policy from "All Users" to "Custom Policy".
starruler added projects: Bug Report, Differential.
starruler added a subscriber: starruler.
epriestley added a subscriber: epriestley.Dec 12 2015, 1:44 AM

Is Settings → Diff Preferences → Show Unified Diffs set to "On Small Screens" (the default) for you?

epriestley added a comment.Dec 12 2015, 1:45 AM

Oh, there are only two options and neither would break this.

I suspect we may not be detecting the 6+ screen as "small" enough since it probably has a resolution of 9000x6000 pixels or something, given that it is several times larger than an XBox.

starruler added a comment.Dec 12 2015, 1:51 AM

iPhone 5s simulator

Screen Shot 2015-12-11 at 5.50.57 PM.png (1×648 px, 70 KB)

starruler added a comment.Dec 12 2015, 1:52 AM

That screenshot was taken as a 'logged out' user. There were two columns (so not unified diffs?)

chad added a subscriber: chad.Dec 12 2015, 1:58 AM

In Safari/iOS I intermittently get the desktop site. No issues in Chrome.

chad renamed this task from 'Show 20 lines' broken on mobile devices to 'Show 20 lines' broken on mobile Safari.Dec 12 2015, 2:02 AM

image.png (1×750 px, 83 KB)

chad added a comment.Dec 12 2015, 2:03 AM

I'm on iOS 9.2

starruler added a comment.Dec 12 2015, 2:07 AM

I was able to repro on Mac OS X with a recent version of chrome.

1/ shrink window to small size
2/ show 20 lines

Screen Shot 2015-12-11 at 6.05.38 PM.png (814×615 px, 84 KB)

chad added a comment.Dec 12 2015, 2:20 AM

The 1-up renderer is server-side, it's not responsive CSS. Reload the page after you shrink.

starruler added a comment.Dec 12 2015, 2:36 AM

Ah that makes sense, when I load a page on my iPhone (iOS 9.1) it shows desktop site and flashes down to mobile. I guess the server thinks I'm a desktop. Note this happens even on an iPhone 4s simulator (the oldest version I can run).

Maybe it's an issue with Retina displays and physical vs logical pixels.

chad added a project: Safari.Jan 27 2016, 6:40 PM
chad mentioned this in T10229: In iOS Safari, buttons must be tapped twice to fire..
chad added a comment.Jan 27 2016, 7:15 PM

See also T10229 for a similar Safari/Diff Render issue.

chad merged a task: T10229: In iOS Safari, buttons must be tapped twice to fire..Jan 28 2016, 5:14 AM
chad added a subscriber: aljungberg.

This now reproduces in Chrome/iOS as of Chrome 48 (they now use WKWebView like Safari does).

https://developer.apple.com/library/ios/documentation/WebKit/Reference/WKWebView_Ref/

chad renamed this task from 'Show 20 lines' broken on mobile Safari to Differential 1-up render doesn't trigger on iOS.Jan 28 2016, 5:33 AM
epriestley added a revision: D15135: Improve reliability of detecting small devices when loading Differential changesets.Jan 29 2016, 1:00 PM
epriestley added a comment.Jan 29 2016, 1:57 PM

I think D15135 actually fixes this. I can't reproduce it in iOS Simulator after applying that patch.

epriestley added a comment.EditedJan 29 2016, 1:58 PM

I've marked D15135 as fixing this out of a general abundance of sunny optimism.

epriestley claimed this task.Jan 29 2016, 2:48 PM
epriestley triaged this task as Normal priority.
epriestley added a comment.Jan 29 2016, 2:55 PM

Specifically, this is my best guess at what's going on:

  • We correctly guess that the device is probably a phone by examining the User-Agent string and default the page to use the "phone" device class as a starting point, emitting a <body> tag with a "device-phone" CSS class.
  • We emit an appropriate <meta name="viewport" ... /> tag (probably? These tags are a bit black-magic-ish, and maybe the state-of-the-art has changed in the last year or so).
  • Early in the page initialization process, we read the actual viewport dimensions to determine which type of device the client really is. Generally, the intent here is to correct our server-side guess if the UA string wasn't reliable or we guessed wrong.
  • On the affected devices, the viewport width comes back with a number much larger than the device's screen width (for example, iOS Simluator was reading a 900+ pixel device width for an iPhone 4s in portrait mode).
  • We may do bad things (for example, selecting the wrong 1up/2up mode; swapping the page to desktop mode) based on this incorrect reading.
  • Moments later, the reading corrects itself, and we do OK things after that, but we may already have done bad things.

The fix in D15135 is to use the smaller of the measured viewport width or window.screen.availWidth, if the latter is defined. It appears to be defined on affected devices and to present a more reliable way to detect small screen sizes.

epriestley closed this task as Resolved by committing rPdbf1d0d7210f: Improve reliability of detecting small devices when loading Differential….Jan 29 2016, 2:55 PM
epriestley added a commit: rPdbf1d0d7210f: Improve reliability of detecting small devices when loading Differential….
epriestley added a comment.Jan 29 2016, 2:57 PM

This is now deployed to this server (secure.phabricator.com) -- let me know if you're still seeing issues? Behavior seems OK in the simulator and with my physical iPhone 4s, but I don't have a fancy physical 6s to test.

Phabricator · Built by Phacility