iOS: Fix image display issue in Lightbox.

[gokcan]

iOS app could not be able to display images having non-ASCII characters
(e.g. 你好世界, алуй, مرحبا) in their filenames.

This PR fixes #2113.

[zulipbot]

Hello @gokcan, it seems like you have referenced an issue in your pull request, but you have not referenced that issue in your commit message(s). When you reference an issue in a commit message, it automatically closes the corresponding issue when the commit is merged.

Please run git commit --amend in your command line client to amend your commit message description with "Fixes #{issue number}”.

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51

Thank you for your contributions to Zulip!

[borisyankov]

Is original a good name to represent that part of the string?
Is it maybe path?

[gokcan]

Yeah, path sounds good. :)

[borisyankov]

Adding a test for this function will prove that it works correctly.

[gokcan]

Added the tests now. Thank you.

[gnprice]

Thanks @gokcan for digging into this!

This solution looks brittle to me -- I worry that there may still be a bug in this path, and also that we're at high risk of creating another bug like this here later. I think with a little more work, we can make a good solid solution that makes this code much more likely to stay robust.

The key thing I'd like to see in a fix is that when looking at a piece of this code, for example handle_image, it should be easy to answer questions like: what is src supposed to be? Is it a URL that the image can be loaded from? Is it some other kind of encoding of something related to a URL, and if so, what? And similar questions for both the parameter of encodeImageUri and its return value, and for the src prop of LightboxScreen (which is where that first argument to navigateToLightbox is sent).

I think the root cause of this bug is basically that we weren't clear on the answers to those questions, and so the answers ended up not matching up between different parts of the code. And looking at this version, I'm not quite sure what the answers are either.

One great way to resolve this would be for the answers to all the questions like this to be "it's a valid URL which you can load the image from." It seems like that's not yet the case for the src in handleImage. Why is that? Can we make it be the case there?

[gokcan]

Hi @gnprice,

Actually, we are able to display those images with special characters in streams (if we don't enter the LightboxScreen). After further debugging, it becomes obvious that the problem is totally related to library we use for displaying images in LightboxContainer. Here is the last apperance of the image uri, before passing it to the external library component PhotoView:

zulip-mobile/src/lightbox/LightboxContainer.js

Line 108 in 333ff3a

source={resource}

Also, we don't have any issues on Android side, right? This is because that RN library(photo-view) uses different native libraries for different platforms (obviously). Specifically, the iOS part relies on MWPhotoBrowser.

What I found is: The RN library(photo-view) uses NSURL without encoding the NSString to UTF-8. (Solution to this in 3rd bullet point below ↓).

After that, I dug deeper.. and found that the core logic of MWPhotoBrowser for async download operation, uses another library called SDWebImage.

Not surprisingly, there are open issues also about this problem on their repos 1 2.

In the end, we have 3 options:

• Escaping the special characters in our inside image URI logic (RN). (We are already doing this in this PR). URL Escaping also reduces the risks related to security (valid statement for both option 1 and 3). -- Remember effective power? :)
• Completely replacing the PhotoView component with another useful library. (If we can find such a library).
• Escaping the special characters in the library source (iOS). Replace this line in the lib source with:
  NSURL *imageURL = [NSURL URLWithString:[uri stringByAddingPercentEscapesUsingEncoding: NSUTF8StringEncoding]]; (I have already tested this option too, it works well!)
  UPDATE: I've also opened a PR on their repo:
  iOS: Fix special character issue in image URL. alwx/react-native-photo-view#157

On the other hand, src is something like this before entering the encodeImageUri(). It is a URI (actually URN) that the image can be loaded from:

https://chat.zulip.org/user_uploads/2/5f/_QwEkmtWog1ZlhI4A6dET8ap/你好世界.jpg

and after, like this:

https://chat.zulip.org/user_uploads/2/5f/_QwEkmtWog1ZlhI4A6dET8ap/%E4%BD%A0%E5%A5%BD%E4%B8%96%E7%95%8C.jpg

I choose to use encodeImageUri() inside the handleImage, because handleImage is the common way to reach LightboxScreen. So that we can intercept the url in there, (before url is passed into the props of LightboxScreen), make the escaping (by encoding) and pass it to the props of

navActions ->> LightboxScreen -> LightboxContainer -> External libraries

Looking forward for your suggestions on this @gnprice!
Thank you.

[gnprice]

Thanks for this investigation! These resources are helpful.

One bit of terminology: looking at RFC 3986, which is the main RFC describing URIs (*) or URLs, we find that

A URI is a sequence of characters from a very limited set: the letters of the basic Latin alphabet, digits, and a few special characters.

and in particular that every character in a proper URI is a character from ASCII. There is no such thing as a URI nor a URL, according to the official definitions, that contains non-ASCII characters.

I've added a comment on the PR you opened in react-native-photo-view. Perhaps that will get fixed there, and in that case it's no longer a live problem for us.

More thoughts coming in another comment.

(*) And although the RFCs say "URI", I usually prefer to say "URL" because the distinction is basically dead -- even in the formal terminology of the RFCs, virtually every URI that anyone cares about is in fact a URL -- and "URL" is better understood.

PS - the URLs we're talking about are not in fact URNs. :-) Few URLs, or URIs, are. Fortunately, as RFC 3986 says, you can basically ignore the concept of a URN, and I recommend doing so.

[gnprice]

So, whether the issue gets fixed in react-native-photo-view or not, I'd still like to sort out having clear answers to what type of thing each prop or parameter in our code, like src, is supposed to be.

• It seems like RN's Image accepts either a real URI, or a sort of proto-URI where the non-ASCII characters can be percent-encoded to make a URI.
• Moreover, that behavior comes (at least on iOS) from RCTConvert, which is a very general function that RN uses internally all over to convert JS objects to native objects like NSURL. So this is basically the semantics that almost anything on RN that needs a URI/URL will have.
• PhotoView on Android seems to do the same thing.
• PhotoView on iOS apparently requires a genuine URI.

Given how broadly this either/or semantics is present in RN, I'm OK with the answer to "what is this supposed to be" being "a React Native uri", meaning either a real URI or something that can be converted this way into one.

Then it's just a matter of converting that to what PhotoView needs, which on iOS is a genuine URI. Fortunately, a genuine URI works fine on Android too.

So here's the logic I propose:

• We pass around "React Native URLs" everywhere, right up until we're about to call PhotoView.
• Then, we convert to a real URL.
• Doing so means we (a) encode as bytes with UTF-8, then (b) percent-encode each byte that's not ASCII.
  • This is a no-op if the whole string fits in ASCII already, so we might check and skip doing any work in that case.
• When we call that conversion function, right above the use of PhotoView, we have a comment explaining clearly why we're doing this. 😉

Want to try doing that?

[gnprice]

Thanks @gokcan !

Quick workflow bits:

• Because the later commits in this branch are fixing up earlier commits, please squash them into one commit (and give that one a commit message that describes the whole change.) That's part of how we produce clear and coherent commits.
• GitHub has a bit of a gotcha where it doesn't make it easy for a reviewer to tell when you push a new version of your code to a PR. So when you push a new version that's ready for review again, it's a good idea to make a comment on the PR saying so.

Inside this code:

• I'm still not a fan of this logic that tries to specifically identify "image" URLs and then operate just on a specific part of the URL. It's making a lot of assumptions about what the URLs will look like, and feels pretty brittle to me.

• What I'd like is to mirror as closely as possible this logic where RN does its own conversion from a string that may be either a real URL or a sort of proto-URL, to a real URL:
  https://github.com/facebook/react-native/blob/f8d6b97140cffe8d18b2558f94570c8d1b410d5c/React/Base/RCTConvert.m#L83-L97

  Then the function can have a name like "encodeRnUrl", and a comment explaining that it's mirroring that logic. And then when we're about to pass the URL to PhotoView, like here, we just unconditionally call that function.

  We can skip the parts of the logic under "Assume that it's a local path", if they turn out to be at all tricky, as they shouldn't matter for now; if so, just leave a comment there saying so.

Would you try that approach?

(BTW, I appreciate these tests!)

[gnprice]

A note, as I close a tab I had open:

The function encodeURI may do exactly what we need to convert a "React Native URI" to a real URI. I haven't checked that carefully, though -- another look is needed.

[gokcan]

@gnprice I think it mostly does what we want. However, it does not take into account the 'double-encoding' case.

We can use the encodeUri inside the encodeImageUri's condition-check.

[zulipbot]

Heads up @gokcan, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

[gnprice]

Closing this PR as it's gone stale. Thanks @gokcan for your work on it!