Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avatars don't work on iOS 14, except in message list #4365

Closed
titanous opened this issue Dec 30, 2020 · 15 comments · Fixed by #4420
Closed

Avatars don't work on iOS 14, except in message list #4365

titanous opened this issue Dec 30, 2020 · 15 comments · Fixed by #4420
Labels
a-avatar Avatar URLs, caching, sizes, styles, etc. a-iOS P1 high-priority

Comments

@titanous
Copy link
Contributor

The profile picture in the bottom tab bar is missing on local builds targeting the current version of iOS (see screenshot below). Additionally, the margin for the "home swipe bar" appears to be missing. I have tested this in a few different configurations, and here's what I found:

Working

  • Release version (v27.157) from the App Store on an iPhone 12 with iOS 14.3
  • master (05048c7) built with Xcode 12.3 running in Simulator 13.7

Broken

  • master built with Xcode 12.3 on an iPhone 12 with iOS 14.3
  • master built with Xcode 12.3 running in Simulator 14.3
  • master built with Xcode 12.3 running in Simulator 14.0
  • v27.157 built with Xcode 12.3 running in Simulator 14.3

Simulator Screen Shot - iPhone 12 - 2020-12-29 at 21 29 32

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Dec 30, 2020

Thanks for the detailed report, @titanous!

@gnprice, you mentioned here the possibility of upgrading your test device's iOS version to 14. Did you end up doing that, or would it be best to keep it on its current version while plenty of people are still using that version? 🙂 (The latter has been my strategy for my iPhone lately; it's on 13.7, which I don't mind.)

@gnprice
Copy link
Member

gnprice commented Dec 30, 2020

Thanks @titanous for the detailed report!

I just tried this in an iOS 14 test device, and it's working. That's:

  • v27.157 from the App Store (well, TestFlight) on an iPhone XR with iOS 14.2

I see the profile picture there, and there's a margin below those five bottom icons.

One other variable which could affect this behavior is the type of avatar -- Zulip avatars can be hosted in a few different ways and places, and we've had bugs in the past which affected one or another type differently. In particular there's:

  • A Gravatar, the default for a new user;
  • An avatar the user uploaded, and which the Zulip server hosts locally (as chat.zulip.org and most other Zulip servers do);
  • An avatar the user uploaded, and which the Zulip server stores in S3 and serves a redirect to there (as zulipchat.com does);
  • An avatar that's ultimately one of the above, but where the Zulip server doesn't tell the client the URL up front, to save data on startup (significant when there's tons of inactive users, as on chat.zulip.org); this is new since v27.157, and only applies to users that have been inactive for weeks, which probably means it can't be involved for your own avatar.

I just tried the first two of these cases, and both worked.

Studying the set of configurations you tested:

  • From the results on master in the simulator, it seems like something changed with iOS 14 that broke this.
  • Then there's also the peculiar result that it works fine on v27.157 from the App Store, on a physical device, but breaks with the version you built from source at the same version, in the simulator. That is puzzling.

One test that would help narrow things down is to try it built from source at v27.157, but on the same physical device where the version from the App Store works. That will isolate whether it's the source vs. App Store, or physical vs. simulated device, that's making that difference. (Either of which would be puzzling and call for further investigation.)

Additionally, the margin for the "home swipe bar" appears to be missing.

This sounds like an unrelated bug. Does it have the exact same pattern of working vs. not working (in those configurations you tested) as the missing-avatar bug? That'd be very interesting, just because it sounds so unrelated.

On its own, that symptom sounds like a case of #3066, which is an old bug that I have to admit I'm a bit embarrassed we haven't fixed yet.

@gnprice gnprice added a-avatar Avatar URLs, caching, sizes, styles, etc. a-iOS labels Dec 30, 2020
@titanous
Copy link
Contributor Author

I saw the same thing for both of these variants:

  • A Gravatar (in this case one of the random defaults)
  • An avatar the user uploaded, and which the Zulip server hosts locally (testing on chat.zulip.org)

One test that would help narrow things down is to try it built from source at v27.157, but on the same physical device where the version from the App Store works. That will isolate whether it's the source vs. App Store, or physical vs. simulated device, that's making that difference.

I did this test, and v27.157 built locally is broken on the same physical device where the App Store version is working.

I wonder if perhaps the App Store version was built against an older SDK version or with an older version of Xcode and so there's a compatibility layer that's being inserted?

This sounds like an unrelated bug. Does it have the exact same pattern of working vs. not working (in those configurations you tested) as the missing-avatar bug? That'd be very interesting, just because it sounds so unrelated.

Indeed, after further testing, it is an unrelated bug. It only reproduces in the simulator for the 12/12 Pro/12 Pro Max and on my physical 12. The simulator 12 mini, and various models of the 11 have the correct margin. This is another case where the App Store version has the correct margin on my physical 12, but all local builds are broken.

@gnprice
Copy link
Member

gnprice commented Dec 30, 2020

I did this test, and v27.157 built locally is broken on the same physical device where the App Store version is working.

Fascinating.

I wonder if perhaps the App Store version was built against an older SDK version or with an older version of Xcode and so there's a compatibility layer that's being inserted?

Yeah, quite plausible.

On the machine I use for App Store builds, I have:

$ xcodebuild -version
Xcode 11.7
Build version 11E801a

so that's consistent with that theory. (I believe I haven't upgraded Xcode since building the v27.157 release; certainly I haven't downgraded it.)

I guess the next step is I should see if I can reproduce with that Xcode 11.7.

@gnprice
Copy link
Member

gnprice commented Dec 30, 2020

Indeed, after further testing, it is an unrelated bug.

Cool. Filed that one as #4369.

@gnprice
Copy link
Member

gnprice commented Dec 31, 2020

I guess the next step is I should see if I can reproduce with that Xcode 11.7.

I think the answer is... that I can't run that test! When I try to run the app from source on that device, I get an error saying "The current device configuration is unsupported. This iPhone XR is running iOS 14.3 (18C66), which is not supported by Xcode 11.7. To run on this device, please update to a version of Xcode that supports iOS 14.3."

Awkward because it appears that once I upgrade to Xcode 12, the next release I build will bring this bug to all users with affected devices... so I'd like to stay on Xcode 11 until this issue is resolved. (I only have the one Mac.)

I just took a look through the iOS 14 release notes, and didn't find any change there that I could recognize as likely to be related to this issue.

I think the next step is to take a configuration where it's broken, and do some debugging there. Some debugging questions that might be helpful to answer to narrow things down:

  • Are avatars showing up just fine in other places in the app? E.g.:
    • in the list of PM conversations (the "people" icon in that main bottom nav)
    • the giant version of your own avatar, at the "profile" screen that that lower-right icon leads to
      • Or is that button not even working? I'd guess that it works and just isn't rendering its image.
    • in the message list when you go look at some messages
  • Is the OwnAvatar component (which supplies the avatar in that spot in the main bottom nav) winding up with the right avatar URL?
  • If so, is something going wrong in the request to fetch that image?
    • This might be easiest to debug from the server side: using an uploaded avatar on a development Zulip server, or another server where you can inspect the server logs.

@gnprice
Copy link
Member

gnprice commented Dec 31, 2020

(I believe I haven't upgraded Xcode since building the v27.157 release; certainly I haven't downgraded it.)

Oh, and a correction on this part: I actually did upgrade Xcode (chat thread) the week after building that release. Previously it was Xcode 11.3.

@titanous
Copy link
Contributor Author

  • Are avatars showing up just fine in other places in the app? E.g.:
    • in the list of PM conversations (the "people" icon in that main bottom nav)

Missing here.

  • the giant version of your own avatar, at the "profile" screen that that lower-right icon leads to

Missing here.

  • Or is that button not even working? I'd guess that it works and just isn't rendering its image.

The button working.

  • in the message list when you go look at some messages

Works here.

I'll dig further on this after I put together the PR for another fix I'm working on.

@gnprice
Copy link
Member

gnprice commented Dec 31, 2020

I'll dig further on this after I put together the PR for another fix I'm working on.

Sounds great!

Interpreting a bit that pattern of observations:

  • it's failing everywhere that we try to show an avatar directly from the React Native part of the app;
  • it's working where the avatar instead appears inside the webview for the message list.

In the first category, all those spots in the app ultimately use our UserAvatar component; and that just uses RN's built-in ImageBackground component, passing it an appropriate source URL for the avatar image.

In the second category, the RN code that's putting together the HTML supplies an avatar URL (see e.g. messageAsHtml.js); and then inside the webview, some other code goes and rewrites the src attributes, when they point to specific URL routes on the Zulip server, to include the user's API key in the query part.

See also #4367 -- that edits UserAvatar to fix a bug with some very similar symptoms. But that seems to be a different bug -- it was happening on iOS 13 and on Android, and on the other hand only for certain types of avatars (the fourth of the four categories I mentioned at #4365 (comment) .)

At this point it seems possible that the bug we're looking at is in RN, in the implementation of ImageBackground. Certainly it looks likely that further debugging will soon lead into doing some debugging in that implementation.

@gnprice gnprice changed the title Profile picture in bottom tab bar is missing on iOS 14 Avatars don't work on iOS 14, except in message list Dec 31, 2020
@titanous
Copy link
Contributor Author

titanous commented Jan 1, 2021

@titanous
Copy link
Contributor Author

titanous commented Jan 1, 2021

This is blocked by #4245.

@gnprice gnprice added the blocked on other work To come back to after another related PR, or some other task. label Jan 4, 2021
@gnprice
Copy link
Member

gnprice commented Jan 4, 2021

@titanous Great, thanks for tracking that down!

This makes a good reason for us to take the time soon to complete that RN upgrade. As you can see on #4245, Chris has marked it now as a priority issue.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jan 21, 2021
In this commit:

- Update "react-native", "react", and "flow-bin" in package.json.
  Run `pod update Folly --no-repo-update` to move past an error on
  the `pod install` step of the `postinstall` script [1]. After this
  step, the Podfile.lock file is much changed, as expected. There is
  also a change in the project.pbxproj file.

- Update .flowconfig with the new Flow version, and address one new
  warning by removing a `$FlowFixMe` that we shouldn't have needed
  in the first place. (Usually this part is much more complicated!)

- Update "jest-expo" in package.json, to oblige a peer-dependency
  constraint on "react".

- Make a change to the Podfile, according to
  facebook/react-native@a35efb940.

  - Instead of spelling out all the Pods from RN (all the
    non-Flipper-related Pods, that is -- we'll handle the Flipper
    ones in a later commit), call a function defined by React
    Native, `use_react_native!`, newly exposed in RN v0.63.

  - Since we're still using React-RCTPushNotification (pending the
    resolution of zulip#4163), take care to keep
    React-RCTPushNotification. It apparently hasn't yet been removed
    from React Native, but it understandably isn't handled by
    `use_react_native!`.

See the PR thread [2] for the list of RN commits affecting the
template app and how we've chosen to propagate the template-app
changes into our project.

See also the RN v0.62 -> v0.63 upgrade guide [3], which gives a
visual representation of the changes to the template app. It mostly
matches the changes we've made; important deviations should have
been explained in the commit list in the PR thread [4].

We know of one bug that's directly solved by taking this upgrade:
issue zulip#4365. I've just tested, and I do indeed start to see avatars
on a simulator running iOS 14, where I didn't see them before.

[1] See
    zulip#4420 (comment)
    for more details on this.

[2] zulip#4420 (comment)

[3] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4

[4] As always, the guide does show some changes that don't appear in
    the template app. I think this noise is an effect of how the
    guide is generated (with react-native-community/rn-diff-purge)
    and can safely be ignored. It's a diff between a fresh app
    created with `react-native init --version=$CURRENT` and a fresh
    app created with `react-native init --version=$NEXT`, for the
    selected values of `$CURRENT` and `$NEXT`. In particular:

    - I believe that some dependency version range changes in
      package.json, including for @babel/core and @babel/runtime,
      might be caused by different versions for those dependencies
      being available when the `react-native init` commands are run.

    - Some changes in ordering and unique ID values always seem to
      show up in the project.pbxproj file.

Fixes: zulip#4245
Fixes: zulip#4365
@gnprice gnprice removed blocked on other work To come back to after another related PR, or some other task. help wanted labels Jan 26, 2021
@gnprice
Copy link
Member

gnprice commented Jan 26, 2021

Hmm, well -- we landed that RN upgrade last week (to v0.63.4), which we expected would fix this. But with #4431, we have a report of what sound like the same symptoms still appearing.

@titanous Would you try this again with a current version from master, and confirm whether it still reproduces for you?

If it does, then I wonder if something went wrong with that bugfix in RN upstream, or if we're seeing some other bug that happens to have overlapping symptoms.

@gnprice
Copy link
Member

gnprice commented Jan 26, 2021

Also reopening this issue -- given that report, seems safest to regard it as open until we have some confirmation that the fix seemed to work 🙂

@gnprice gnprice reopened this Jan 26, 2021
Gautam-Arora24 pushed a commit to Gautam-Arora24/zulip-mobile that referenced this issue Feb 2, 2021
In this commit:

- Update "react-native", "react", and "flow-bin" in package.json.
  Run `pod update Folly --no-repo-update` to move past an error on
  the `pod install` step of the `postinstall` script [1]. After this
  step, the Podfile.lock file is much changed, as expected. There is
  also a change in the project.pbxproj file.

- Update .flowconfig with the new Flow version, and address one new
  warning by removing a `$FlowFixMe` that we shouldn't have needed
  in the first place. (Usually this part is much more complicated!)

- Update "jest-expo" in package.json, to oblige a peer-dependency
  constraint on "react".

- Make a change to the Podfile, according to
  facebook/react-native@a35efb940.

  - Instead of spelling out all the Pods from RN (all the
    non-Flipper-related Pods, that is -- we'll handle the Flipper
    ones in a later commit), call a function defined by React
    Native, `use_react_native!`, newly exposed in RN v0.63.

  - Since we're still using React-RCTPushNotification (pending the
    resolution of zulip#4163), take care to keep
    React-RCTPushNotification. It apparently hasn't yet been removed
    from React Native, but it understandably isn't handled by
    `use_react_native!`.

See the PR thread [2] for the list of RN commits affecting the
template app and how we've chosen to propagate the template-app
changes into our project.

See also the RN v0.62 -> v0.63 upgrade guide [3], which gives a
visual representation of the changes to the template app. It mostly
matches the changes we've made; important deviations should have
been explained in the commit list in the PR thread [4].

We know of one bug that's directly solved by taking this upgrade:
issue zulip#4365. I've just tested, and I do indeed start to see avatars
on a simulator running iOS 14, where I didn't see them before.

[1] See
    zulip#4420 (comment)
    for more details on this.

[2] zulip#4420 (comment)

[3] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4

[4] As always, the guide does show some changes that don't appear in
    the template app. I think this noise is an effect of how the
    guide is generated (with react-native-community/rn-diff-purge)
    and can safely be ignored. It's a diff between a fresh app
    created with `react-native init --version=$CURRENT` and a fresh
    app created with `react-native init --version=$NEXT`, for the
    selected values of `$CURRENT` and `$NEXT`. In particular:

    - I believe that some dependency version range changes in
      package.json, including for @babel/core and @babel/runtime,
      might be caused by different versions for those dependencies
      being available when the `react-native init` commands are run.

    - Some changes in ordering and unique ID values always seem to
      show up in the project.pbxproj file.

Fixes: zulip#4245
Fixes: zulip#4365
@chrisbobbe
Copy link
Contributor

chrisbobbe commented Apr 14, 2021

seems safest to regard it as open until we have some confirmation that the fix seemed to work 🙂

We may have it. @Gautam-Arora24, the author of #4431, said,

I think building the app from scratch on iPhone solved my problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-avatar Avatar URLs, caching, sizes, styles, etc. a-iOS P1 high-priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants