WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20923 closed enhancement (fixed)

Add support for mobile uploads

Reported by: koke Owned by: nacin
Milestone: 3.4.1 Priority: normal
Severity: normal Version: 3.4
Component: Media Keywords: mobile has-patch commit
Focuses: Cc:

Description

Starting with iOS6, Mobile Safari is able to upload photos.

The current implementation calls _device_can_upload to decide if it should show the upload form, which returns false for any version of iOS

The best solution would probably be using Javascript for that check, instead of user agent detection, since other mobile platforms might follow in the future

Attachments (4)

20923.diff (666 bytes) - added by georgestephanis 3 years ago.
WP_Browser_Detection.7z (7.9 KB) - added by tomauger 3 years ago.
Robust, non-server user agent and browser capability detection - alpha version, for discussion
20923.2.diff (561 bytes) - added by nacin 3 years ago.
20923.3.diff (537 bytes) - added by nacin 3 years ago.

Download all attachments as: .zip

Change History (35)

comment:1 @nacin3 years ago

  • Milestone changed from Awaiting Review to 3.4.1

At the moment, the only platforms for which we deny _device_can_upload() is iOS, so no need for any kind of JavaScript check.

azaozz, georgestephanis, and I discussed this last night, but we couldn't come to a consensus on how to consistently identify all three iOS devices. It seems the iPod Touch does not include an OS #_# indicator in its user agent, and the iOS 6 dev preview doesn't update the user agents (they still read 5.1).

Because this is not a straightforward assumption, it will miss the boat for 3.4. However, we can likely ship a fix in 3.4.1 once we can nail this down.

comment:2 @koke3 years ago

OK, since the public release of iOS6 might come around the same time as 3.5, I think there's no hurry in getting this into 3.4

comment:3 follow-up: @koke3 years ago

I was suggesting a Javascript check since I think checking if the feature is available is way better than checking for a specific user-agent.
I've seen it just work with the embedded browser in the iOS app, which has a different user agent

comment:4 @georgestephanis3 years ago

Here's the UA test strings loaded into OSX Safari for each:

iPad: Mozilla/5.0 (iPad; U; CPU OS 4_3_3 like Mac OS X; en-us) AppleWebKit/533.17.9 (KHTML, like Gecko) Version/5.0.2 Mobile/8J2 Safari/6533.18.5

iPod: Mozilla/5.0 (iPod; U; CPU iPhone OS 4_3_3 like Mac OS X; en-us) AppleWebKit/533.17.9 (KHTML, like Gecko) Version/5.0.2 Mobile/8J2 Safari/6533.18.5

iPhone: Mozilla/5.0 (iPhone; U; CPU iPhone OS 4_3_3 like Mac OS X; en-us) AppleWebKit/533.17.9 (KHTML, like Gecko) Version/5.0.2 Mobile/8J2 Safari/6533.18.5

And here's a regex test that should give us an accurate result:

return ( preg_match( '#(iPod|iPad|iPhone); U; CPU( iPhone)? OS ([\d_]+) like Mac OS X#', $ua, $data ) && version_compare( '6', $data[3], '<=' ) );

comment:5 in reply to: ↑ 3 ; follow-up: @azaozz3 years ago

Replying to koke:

I've seen it just work with the embedded browser in the iOS app, which has a different user agent.

You mean checking navigator.userAgent vs. $_SERVER['HTTP_USER_AGENT']? Thinking that by the time iOS6 is released there will be definite differences / easy way to detect it in both UA strings.

comment:6 in reply to: ↑ 5 @koke3 years ago

Replying to azaozz:

You mean checking navigator.userAgent vs. $_SERVER['HTTP_USER_AGENT']? Thinking that by the time iOS6 is released there will be definite differences / easy way to detect it in both UA strings.

No, more like

var file_test = document.createElement("input");
file_test.setAttribute("type", "file");
if (file_test.disabled === false) {
// Supports upload
} else {
// Doesn't support upload
}

I've only tested it on iOS and Chrome so far, but that was the idea

comment:7 follow-ups: @nacin3 years ago

We do a lot of things in PHP based on whether the device can upload, so a JS check is not really feasible without doing things like setting a cookie, etc. Much easier to just parse the user agent, given there *will* be obvious signs.

@georgestephanis3 years ago

comment:8 @georgestephanis3 years ago

Ticket attached that will implement what I'd mentioned up above.

PHP's internal version_compare() function will handle the conversion of the underscores to dots and comparisons. Hurrah, outsourcing!

comment:9 @georgestephanis3 years ago

  • Keywords has-patch added; needs-patch removed

comment:10 in reply to: ↑ 7 @koke3 years ago

Replying to nacin:

We do a lot of things in PHP based on whether the device can upload

When I looked into this, it seemed to me that it was just used for showing that message:

$ grep -ri _device_can_upload .
./wp-admin/includes/media.php:	if ( ! _device_can_upload() ) {
./wp-includes/class-wp-customize-control.php:		if ( ! _device_can_upload() ) {
./wp-includes/functions.php:function _device_can_upload() {
./wp-includes/media.php:			'supported' => _device_can_upload(),

comment:11 in reply to: ↑ 7 @daniloercoli3 years ago

Replying to nacin:

We do a lot of things in PHP based on whether the device can upload, so a JS check is not really feasible without doing things like setting a cookie, etc. Much easier to just parse the user agent, given there *will* be obvious signs.

I would suggest to use browser-based tests wherever possible. Keeping track of device capabilities using fixed sets of user agents becomes a challenge over the longer term.

comment:12 @daniloercoli3 years ago

  • Cc ercoli@… added

comment:13 @omarabid3 years ago

Isn't this a bad practice? If the browser changes its user agent (in a future release or update), then the code needs to get update. This will open up another ticket and more time is wasted doing maintenance.

I don't have an idea about the features required for mobile uploads; but why don't you test if the required JavaScript functions do actually exist, and make a device/Os/browser agnostic solution?

comment:14 follow-up: @georgestephanis3 years ago

There's a reason that we've always based this off User Agent -- not JS tests in the browser. That is, quite simply, because we have yet to see a browser test that works.

The typical method for spotting input type support is as follows ...

test = document.createElement( 'input' );

test.setAttribute( 'type', 'file' );

if ( test.type == 'file' ) {

document.write( 'This browser supports file inputs.' );

} else {

document.write( 'This browser does NOT support file inputs.' );

}

Now, if you run that in iOS, it will always return true and yield the first output, as it recognizes that file is a valid property for input type. They just choose not to let you do anything with it.

If you look at the patch that I submitted up above, it actually tests to get the iOS version number, and compares it against version 6 -- the first version that will actually let you use the file inputs. So it won't need to be maintained in the future, it'll just be an ifcheck against older versions of iOS that don't support file uploads. Also, if the UA changes in the future, then the first part of the ifcheck will fail, and it will display the upload option! We only really need to focus on catching the existing iOS < 6, as all future versions will support it.

comment:15 in reply to: ↑ 14 @koke3 years ago

Replying to georgestephanis:

There's a reason that we've always based this off User Agent -- not JS tests in the browser. That is, quite simply, because we have yet to see a browser test that works.

See #6, testing against test.disabled seems to work. I have this page if you want to test it in different devices

comment:16 @mrroundhill3 years ago

  • Cc dan@… added

comment:17 @nacin3 years ago

If we wish to address this for 3.4.1, I suggest we slip in some user agent detection into _device_can_upload(). For 3.5, we should create a new API that uses cookies to pass client-based detection information back to the server.

@tomauger3 years ago

Robust, non-server user agent and browser capability detection - alpha version, for discussion

comment:18 @tomauger3 years ago

  • Cc tomaugerdotcom@… added

@nacin, to that point, I'd be happy to contribute my work on user-agent detection. I'm attaching some early code that leverages Jon Stoppani's Browscap.php library. His licensing is current MIT Open Source. My wrapper (not yet quite pluginized, but easily done) adds a bunch of conditionals such as is_ie(), is_modern() or is_mobile(). The real work is done by Gary Keith and the Browser Capabilities Project team, which regularly update a large .ini file containing every user-agent string known to man, including obscure mobile microbrowsers etc. So it is completely independent of any server-side detection. The only caveat is that this is a reasonably large download, so it gets cached and then downloaded on a periodic basis. This mechanic can probably be improved.

Anyway, here's the code, just for an audit to determine the viability of including such detection capabilities natively in core. IMO, with the browser landscape becoming ever more complex, something like this will become more and more important for responsive device-aware design and development. I'm happy to continue to build out this project beyond a plugin if the team thinks it's warranted.

comment:19 @nacin3 years ago

For 3.4.1 I like 20923.diff but would rather put that inside the strpos() checks, for speed and to be safer.

Last edited 3 years ago by nacin (previous) (diff)

comment:20 follow-up: @nacin3 years ago

To clarify, for 3.5, we can move this to be client-side. I do prefer checking capabilities. We could really use a basic framework for JS-based capability checking that can then be set in a cookie for the rest of the session. This isn't the first time such a situation has come up. Can possibly piggyback on the existing user-settings bits.

comment:21 in reply to: ↑ 20 @azaozz3 years ago

Replying to nacin:

Agree. Still need to be careful the JS based capability checks actually work for all devices and are not based on browser bugs, etc. Several of the mobile browsers "lie" about certain things, uploading is one.

@nacin3 years ago

comment:22 @nacin3 years ago

  • Keywords commit added

Marking 20923.2.diff for commit.

comment:23 follow-up: @georgestephanis3 years ago

On that last patch, use $ua instead of $_SERVERHTTP_USER_AGENT? as it's already captured in a variable, no?

I'd write up a new .patch but I'm on a tablet currently.

comment:24 in reply to: ↑ 23 @nacin3 years ago

Replying to georgestephanis:

On that last patch, use $ua instead of $_SERVERHTTP_USER_AGENT? as it's already captured in a variable, no?

Wasn't paying attention, that's good to me.

I'd write up a new .patch but I'm on a tablet currently.

Meta. :)

@nacin3 years ago

comment:25 @beaucollins3 years ago

Chatted with nacin on IRC. I think this is the most pragmatic fix at this point.

User Agent matching to "un-blacklist" iPhone, iPad and iPod UA strings running "OS 6_0" works for me. Moving forward it would be ideal to do some JS detection, but in this specific case it isn't necessary since it was specifically disabling iOS user agents.

Last edited 3 years ago by beaucollins (previous) (diff)

comment:26 @nacin3 years ago

Also per IRC, the regular expression will become #OS ([\d_]+) like Mac OS X#. The current 6.0 user agent that beaucollins had did not have U; in it. Since this expression is inside a block already checking for iOS devices (rather than 20923.diff) we can get away with this change.

comment:27 @nacin3 years ago

In [21141]:

Add initial support for browser-based uploads in iOS 6. see #20923 for trunk.

comment:28 @nacin3 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [21142]:

Add initial support for browser-based uploads in iOS 6. fixes #20923 for trunk.

comment:29 @nacin3 years ago

By "for trunk" in [21142] I of course meant "for 3.4."

comment:30 @nacin3 years ago

I opened #21083 to continue this discussion for 3.5.

comment:31 @beaucollins3 years ago

With the updated regex I can confirm that both the "multi file" and "single file" uploaders work using Mobile Safari with an iPhone running iOS 6.0 beta 2 as well as an iPad running iOS 6.0 beta 2.

Last edited 3 years ago by beaucollins (previous) (diff)
Note: See TracTickets for help on using tickets.