search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

Uploading images via the phone camera is broken on iOS #4185

Open klonos opened 4 years ago

klonos commented 4 years ago

Description of the bug Back in version 1.4 we added the ability to select an image from a mobile device's camera. Sometime since then, we lost it. We should find out how that happened and add it back.

Heres the markup for current input element:

<input data-file-auto-upload="1" data-file-extensions="png,gif,jpg,jpeg" accept="*/*,capture=camera" type="file" id="edit-field-image-und-0-upload" name="files[field_image_und_0]" size="22" class="form-file validate-extension-processed auto-upload-processed">

I think the problem maybe accept="*/*,capture=camera". That may need to be accept="image/*,capture=camera" for iOS.

Steps To Reproduce To reproduce the behavior:

  1. Log into a clean backdrop install from your iPhone or iPad.
  2. Create a post, and scroll to the image field.
  3. Click "upload" and see if you can browse through your camera.

Actual behavior Unable to open images in the camera app iphone

Expected behavior I should be able to load photos from my camera on an iPhone

Additional information Add any other information that could help, such as:

Recommended Solution

Since we are only addressing about image fields in the PR, the change should not have any negative side affects.

This will NOT solve the problem for file fields that also allow images, those will still be unable to select images from the camera on iOS devices.

Original issue:

I've been owning Android phones for many years now, so I have not noticed this, but I have recently asked a colleague to confirm this for me on their iPhone, and it is indeed broken.

Can we please have a few more people test in various Apple devices and confirm?

PR: https://github.com/backdrop/backdrop/pull/3064

Advocate: @laryn

klonos commented 4 years ago

I have just tested this on BrowserStack...

Hmm, not sure what's wrong with the people that have reported this not working for them

klonos commented 4 years ago

...I have also tested this by adding images in CKEditor. Also works 👍

olafgrabienski commented 4 years ago

I've tested uploading images in CKEditor and via the post image field on iOS.

Side note: Images uploaded via the image field with the iPhone didn't appear with the correct orientation, e.g. upside down. I've then enabled the setting "Enable image re-orientation" in the field settings and uploaded a new image from the iPhone, again without re-orientation success. Can someone confirm the issue? If so, we should have another look at https://github.com/backdrop/backdrop-issues/issues/2887.

Graham-72 commented 4 years ago

@olafgrabienski Is re-orientation working when uploading in CKeditor?

olafgrabienski commented 4 years ago

Is re-orientation working when uploading in CKeditor?

@Graham-72 - sorry to not have mentioned it explicitly: In CKEditor it is working.

Graham-72 commented 4 years ago

@olafgrabienski Thanks. I don't have an iPhone but a friend emailed me a photo and it arrived with its EXIF data which gives an orientation value 'Right top', it's a portrait photo. I uploaded as an image field with re-orientation set and is has been turned by 90 degrees, so in this case it worked in Backdrop.

EXIF data shows the version of the iPhone software as 13.1.3.

Can you find out the EXIF data of the image you were testing? Maybe in Backdrop we are not correctly recognising what iOS uses to denote 'upside down'?

Graham-72 commented 4 years ago

Another thought, which version of PHP were you using? We (Backdrop) are relying on PHP function exif_read_data and then we (Backdrop) rely on the numerical value it produces for orientation. Apparently camera manufacturers differ on this (notably Apple of course). My test was with PHP 7.1.33

Graham-72 commented 4 years ago

A further thought! My desktop image browser (IrfanView) may be converting EXIF orientation number codes into 'Right top' etc. so maybe I am not seeing what iOS is actually producing. More tests are needed.

olafgrabienski commented 4 years ago

@Graham-72 Thanks for your questions! Mabe it's just an issue with my phone: When I open the image with the Mac OS preview, the info dialog says "Orientation 3", which means "180 degrees – image is upside down". So, technically it's okay to show it upside down. (Now I'm wondering why with CKEditor the image wasn't displayed upside down. Maybe because of different information about orientation? An info dialog in my alternative Mac OS X file manager says for instance "Orientation 0". I'd say, unless nobody else reports orientation issues, we shouldn't invest further time investigating this.

Apart of that, the PHP version of the website was 7.2.23. (I've used a demo sandbox from https://backdropcms.org/demo/create.)

Graham-72 commented 4 years ago

@olafgrabienski if it receives a file with EXIF orientation value 3, the re-orientation function in Backdrop's core/includes/file.inc will rotate the image by 180 degrees (- code on line 1952 and following). I would be interested to hear anyone else's reports of problems. I feel there must be some other reason for this experience.

olafgrabienski commented 4 years ago

if it receives a file with EXIF orientation value 3, the re-orientation function in Backdrop's core/includes/file.inc will rotate the image by 180 degrees

Then it worked in my case, as the image has been rotated (i.e. it turned upside down). When I said in https://github.com/backdrop/backdrop-issues/issues/4185#issuecomment-549131624 that it "didn't appear with the correct orientation", I didn't know of the orientation value 3. (On my phone and on my computer it's displayed 'upside up' regardless of value 3.)

Graham-72 commented 4 years ago

Thanks for this news. I wonder whether users sometimes have problems because they do not realise that the re-orientation function has to be set by admin? Personally I think it should be on by default.

cellear commented 4 years ago

I've tested uploading images in CKEditor and via the post image field on iOS.

  • iPhone SE, iOS 12.4.1, Chrome and Safari: works √
  • iPad Pro 11, iOS 12.4, Chrome and Safari: works √

Wait...it does?

I've not been able to get it to work. I guess it's a support issue to figure out what I'm doing wrong :(

cellear commented 4 years ago

Here's what I see on in the interface on my vanity site. No "use camera" or similar option that I can see:

alt text alt text
cellear commented 4 years ago

As compared to this “take photo” option on this very Github interface that we're chatting on. Is Backdrop showing you guys this kind of interface?

alt text

...which pulls up the camera. I think this is a killer feature if we really have it, which I've been told we do :)

alt text
olafgrabienski commented 4 years ago

Is Backdrop showing you guys this kind of interface?

Yes, it is. Tested on a vanilla demo site with iPhone SE, iOS 12.4.1, Safari:

Graham-72 commented 4 years ago

And I can confirm that re-orientation is working when using an Android phone and Chrome and the option to take a photo.

@klonos we seem unable to re-create this problem.

laryn commented 4 years ago

Total spitball -- is it possible a secure page is required for the camera to connect to the field directly?

jenlampton commented 4 years ago

Aha, I thought this issue had been raised before, but all my searches turned up nothing. :( Sorry for the dupe over in https://github.com/backdrop/backdrop-issues/issues/4300. I've closed that issue in favor of this one since this one is older.

I have just tested this on BrowserStack...

I don't think BrowserStack is a suitable way to test this problem since we need to access the camera on an iPhone device. I'm not sure that an file access on an emulator would work exactly the same way as file access on a device.

Total spitball -- is it possible a secure page is required for the camera to connect to the field directly?

No, because applying the change in the PR fixes the issue for iPhone users that are having the problem.

klonos commented 4 years ago

@jenlampton BrowserStack has been using real devices for some time now:

https://www.browserstack.com/real-device-cloud

What is a Real Device Cloud?

Imagine that you have access to thousands of real desktop and mobile devices, each of which can be used for testing websites and apps in real-time, under real-user conditions. These devices are hosted on cloud-based servers, thus making them accessible online at all times. Such a testing infrastructure is called a real device cloud.

jenlampton commented 4 years ago

Woah.

quicksketch commented 4 years ago

The PR at https://github.com/backdrop/backdrop/pull/3064 seems straight forward to me. We already have this bit in file.module:

    // Add support for mobile camera uploads if jpg is a valid extension.
    // Browser support for HTML5 extension validation is still weak at this
    // point, add more extensive extension support in the future.
    if (in_array('jpg', $extension_array)) {
      $element['upload']['#attributes']['accept'] = '*/*,capture=camera';
    }

Would it work to add '*/*,image/*,capture=camera' as the accept attribute so this would work on file fields as well?

In any case, the current approach seems fine as-is anyway. Have we confirmed it solves the problem in the demo sandbox?

olafgrabienski commented 4 years ago

Have we confirmed it solves the problem in the demo sandbox?

I've tested the sandbox, and for me it works on iPhone and iPad. However, both worked for me already before this PR -- see https://github.com/backdrop/backdrop-issues/issues/4185#issuecomment-549131624 --, so I guess more testing is needed.

Retsell commented 4 years ago

I’m confused. Not a programmer; just need to be able to upload photos to my sites from my iPad. Apparently it works for some but not others. Certainly doesn’t work for me. I have backdrop sites on 3 different servers and none of them allow image uploads. The “Take Photo/Photo Library/Browse” options don’t show, for image fields, or Ckeditor insert image. All works fine on a PC, but not on the iPad or iPhone, either in safari or Firefox for iOS. If there is a fix, I’d sure like to know what it is. This thread seemed to go off on a tangent about an image orientation issue that sounds to me like a separate issue. Thanks for any help clearing this up!

Edit: Mine is an iPad Air 2 on iOS 13. I see the testing noted above was with iOS 12.

olafgrabienski commented 4 years ago

@retsell Thanks for your input! It would be interesting to see if the suggested PR (pull request) fixes the issue for you. Could you give the PR a try?

To test the PR easily, go to https://github.com/backdrop/backdrop/pull/3064, scroll down a bit and look for the Website link http://3064.backdrop.backdrop.qa.backdropcms.org. This is a sandbox site which includes the changes of the PR. You'll find the login credentials directly below the website link.

Retsell commented 4 years ago

@olafgrabienski - Thanks for that helpful information. I tested on that site and find that the problem was resolved for uploading to an image field. However, it did not fix the problem for uploading images into the body text field via the editor’s image uploader. Can the same fix be applied to the Ckeditor??

Thanks!

olafgrabienski commented 4 years ago

Thank you, @Retsell, for testing! I confirm that the PR works for the image field but not in the CKEditor image dialog. (Tested on iPhone, iOS 13.4.) @jenlampton Can you extend your PR for images to be inserted via CKEditor?

klonos commented 4 years ago

@jenlampton it seems that the default for '#type' => 'managed_file' when the extension is jpg (the file type that i-devices seem to be using for images taken with the camera) is to set the accept attribute to */*,capture=camera which doesn't seem to work. It needs to be image/*,capture=camera instead.

Here's a PR against your PR/branch that should fix things for CKEditor + other instances where '#type' => 'managed_file' is used 😉

https://github.com/jenlampton/backdrop/pull/11

klonos commented 4 years ago

Hey @Retsell 👋 ...can you please try testing the PR sandbox again? Things should now also work with the "Insert image" dialog in CKEditor. Please confirm.

olafgrabienski commented 4 years ago

For me it works in CKEditor now, using PR https://github.com/backdrop/backdrop/pull/3064! (iPhone, iOS 13.4)

Btw, the sandbox website has a lot of available DB updates, didn't run them.

klonos commented 4 years ago

So OK, it seems that there's more to this 🤔 ...although this may work now, the implementation seems to not be correct, or at least not 100% according to the current standards. This means that we may face issues down the road.

There were a couple of things that have triggered my curiosity:

Anyway, my research has indicated that adding capture= within the accept attribute is not accepted/allowed in the current standard (see https://www.w3.org/TR/html51/sec-forms.html#element-attrdef-input-accept) ...it may have started like that sometime back in 2010, but it was later dropped in favor of a separate capture attribute (see https://w3c.github.io/html-media-capture).

So instead of this:

  $element['upload']['#attributes']['accept'] = 'image/*,capture=camera';

...the proper, standards-compliant way to do things would be something like:

  $element['upload']['#attributes']['accept'] = 'image/*';
  $element['upload']['#attributes']['capture'] = 'camera';

Furthermore, instead of "blindly" doing something like this in file.module:

if (in_array('jpg', $extension_array)) {
  $element['upload']['#attributes']['accept'] = 'image/*,capture=camera';
}

...we should be doing something along these lines:

// Prepend all extensions with a dot, as it's the expected format for the
// "accept" HTML attribute.
$accept_extensions = preg_filter('/^/', '.', $extension_array);
// Prepare the "accept" string.
$accept_extensions = implode(",", $accept_extensions);
// Add allowed extensions to any already existing values.
$element['upload']['#attributes']['accept'] += $accept_extensions;
// If .jpg is among the allowed extensions for the field, then allow using the
// device camera to capture images.
if (in_array('jpg', $extension_array)) {
  $element['upload']['#attributes']['capture'] += 'camera';
}

What will need further testing with the above:

klonos commented 4 years ago

...I should also mention that all this is not just my OCD 😅 ...the accept attribute defines what file types are shown to the user when they click the upload button:

...although this does not do validation (which we do anyway on submit), it is a UX improvement, as users are not presented with disallowed extensions/types.

klonos commented 4 years ago

There are various resources out there, but here are the most helpful I've found:

https://www.w3schools.com/tags/att_input_accept.asp https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/accept https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/capture https://w3c.github.io/html-media-capture/ https://www.w3.org/TR/html51/sec-forms.html#element-attrdef-input-accept

Special kudos: https://blog.addpipe.com/correct-syntax-html-media-capture

jenlampton commented 4 years ago

Let's try a version using the accept attribute and see if that works on iOS.

klonos commented 4 years ago

@jenlampton I've filed a more comprehensive PR against your branch (also accounts for any previously set/overridden "accept" and "capture" attributes). Have a look and let me know what you think.

herbdool commented 4 years ago

Marking this as needs work since klonos has suggested some changes

jenlampton commented 4 years ago

I've merged @klonos's changes into my PR. Does anyone have an iPhone to test the updated PR?

olafgrabienski commented 4 years ago

I have, can test in about 2 hours.

olafgrabienski commented 4 years ago

@jenlampton I've had a look at the new sandbox site of the PR. When I try to add an image with an iPhone (iOS 13.5.x), the camera is starting immediately. I can't choose between taking a photo with the camera and other options like browsing the photo library of the phone. This happens both in CKEditor and using the image field.

klonos commented 4 years ago

🤦 ...the spec for the capture attribute seems to have changed to accept only the following:

<input type="file" accept="image/*" >
<input type="file" accept="image/*" capture>
<input type="file" accept="image/*" capture="user" >
<input type="file" accept="image/*" capture="environment" >

...where "user" means the user-facing camera on mobile, and "environment" means the front-facing camera. From what I've read so far, specifying just capture w/o any value works to allow both file uploads + camera, with a prompt for the user to select from these options.

The problem is that this:

$element['upload']['#attributes']['capture'] = '';

...adds the attribute like so:

<input type="file" accept="image/*" capture="">

Which won't work. I'll need to test some more and see if $element['upload']['#attributes']['capture'] = NULL; works 🤔

Retsell commented 3 years ago

@jenlampton I've had a look at the new sandbox site of the PR. When I try to add an image with an iPhone (iOS 13.5.x), the camera is starting immediately. I can't choose between taking a photo with the camera and other options like browsing the photo library of the phone. This happens both in CKEditor and using the image field.

For me this is a big issue. The ability to upload images from an iPad (or iPhone) is critical. Maintaining a site while on the road, is severely hampered without any way to upload images. Uploading directly from the camera is much less important. I have a D7 site ready to convert to Backdrop, but can’t do it until this is fixed!

jenlampton commented 3 years ago

..adds the attribute like so: <input type="file" accept="image/*" capture=""> ... Which won't work

@klonos Are you sure? That's valid HTML, it should work exactly the same way as <input type="file" accept="image/*" capture> From @olafgrabienski 's latest test it sounds like it's working like capture="user"

@Retsell thanks so much for weighing in. We agree this is a huge issue and that's why we got it working back in 2016, unfortunately iPhones have changed since then :) As you can see from the comments above, we've got a fix ready, and are in process of testing!

olafgrabienski commented 3 years ago

From @olafgrabienski 's latest test it sounds like it's

Not sure what that means. To be clear, during my latest test I wasn't able to choose between taking a photo with the camera and browsing the phone's photo library, instead the camera started right away.

jenlampton commented 3 years ago

Sorry, stopped in the middle of a sentence there. I Edited the comment and Added the end :)

Retsell commented 3 years ago

Well here’s an interesting tidbit I discovered about this problem: The upload dialog works as it should if you remove ‘jpg’ from the list of acceptable file types in the content type > image field setting!

‘jpeg’ is ok, as are ‘gif’ and ‘png’, but as soon as you add ‘jpg’ into the mix, the whole upload dialog box changes to the un-working version.

So a good work-around is to remove ‘jpg’ as a choice in the settings for an image field, and rename .jpg files to .jpeg before uploading. Then the files can be uploaded as usual.

Hopefully this information will also help someone to troubleshoot the issue.

findlabnet commented 3 years ago

Are iphone users is 80% of our users? If so, you can change the default file extension for them, but I'm not sure about this number.

Retsell commented 3 years ago

No, changing the default file extensions is not a solution. It’s a bug that needs to be fixed. But in the meantime, if anyone needs to be able to get image uploads working on an iPad, just remove ‘jpg’ from the allowed extensions list and it will work. Any .jpg files can be changed to .jpeg, and then they will upload.

Also, the same problem exists in file uploads — but they don’t normally include .jpg extensions. If you ad them though, it will break the uploader when used on an iOS device.

indigoxela commented 3 years ago

just remove ‘jpg’ from the allowed extensions list and it will work. Any .jpg files can be changed to .jpeg, and then they will upload.

@Retsell many thanks for figuring this out! Not sure yet, how we could fix that in Backdrop, though, as it looks like an iOS bug, actually. Tricky.

Retsell commented 3 years ago

Not sure yet, how we could fix that in Backdrop, though, as it looks like an iOS bug, actually. Tricky.

@indigoxela: yes, it seems like maybe an iOS bug, but then the same file upload routine in Drupal 7 works fine with the iPad. File uploading in other systems or programs also work just fine. The only place I’ve found this bug is in Backdrop.

Retsell commented 3 years ago

More about this issue: the original posting was about failure to be able to upload images from iPhone cameras. From what I discovered in testing, per my notes above:

  1. When attempting to upload an image on an iOS device, the proper dialog box, which includes three options (Take Photo or Video, Photo Library, or Browse) does not appear if ‘jpg’ is included in the allowable extensions list for the image field. Instead you get a browser box with all entries grayed out.
  2. Removing ‘jpg’ from the allowable extensions list causes the proper dialog box to appear again, giving the option to take a photo to upload (as well as the other two options.)
  3. However, the iOS photos taken are assigned the ‘jpg’ extension. So although the proper dialog box is there, and it allows you to take a photo for uploading, it throws an error when you accept the photo, since it doesn’t have an acceptable file name extension.

Again, these functions work fine in Drupal 7, but for some reason are messed up in Backdrop.

But wait, there’s more! What about inserting images in CKeditor? The same problem exists there, i.e., it brings up the browser box with grayed out images instead of the three options box. So it’s probably the same issue. But you go to the CKEditor settings for the editor format of choice, and there is no setting for allowable extensions for images. Immediately following the Image Uploading section is a File Uploading section, which DOES include settings for allowable file extensions. Clearly there should be a field for specifying allowable extensions for images. I think this is really a separate (but related) issue to the 'jpg' problem. Perhaps the lack of a setting for allowable file types should be posted as a bug in CKEditor?