element-hq / element-ios

A glossy Matrix collaboration client for iOS
https://element.io
GNU Affero General Public License v3.0
1.73k stars 492 forks source link

Nbuquet/respect system setting font scale #7701

Closed NicolasBuquet closed 11 months ago

NicolasBuquet commented 11 months ago

Pull Request Checklist

image

pixlwave commented 11 months ago

Hi @NicolasBuquet thanks for this PR. Its exciting to see someone working on accessibility in old Element.

I'm afraid we can't accept the PR in its current state however, for a quite a few reasons. To implement this properly will be a lot of work, especially on the timeline. The initial things that jump out at me are listed below:

  1. This changes the message font size from 15pt to 17pt which but none of the other user interface elements are updated to match. You would probably need to use subheadline as the base font to fix this:
develop PR
IMG_0707 Simulator Screenshot - iPhone 14 - 2023-11-02 at 16 54 59
  1. When comparing those two screenshots, notice how the layout is incorrect on this PR, missing messages from the sender (the view is scrolled right to the bottom). The whole timeline is driven by fixed font sizes, so for example the logic to detect the tapped message is now wrong and tapping on 3 to react to it does this:

https://github.com/vector-im/element-ios/assets/6060466/3195695c-02c9-4448-8d18-9f026ff1d396

Whilst it looks like the UI was awkward and it succeeded, it did not:

develop
IMG_0708@2x
  1. Setting the font after setting an attributed string wipes out all formatting on that string. So for example the message below is now incorrect:
develop PR
IMG_0710 Simulator Screenshot - iPhone 14 - 2023-11-02 at 17 07 13

I'm very much in favour for making our apps more accessible (and we're putting a lot of effort into Element X to check for things like this). However, if we're going to accept changes to support Dynamic Type in this app, we would likely be looking for the overwhelming majority of the screen to be updated to support it, and not just the tabs on the home screen and the message text in the timeline. It would need to be done screen by screen (with a branch based cleanly off develop), and I even then I'm not sure that we as a team have the time to dedicate to reviewing this given Element iOS is not meant to be receiving anything more than P1 bugfixes.

manuroe commented 11 months ago

I am closing this PR because getting dynamic font size on element-ios will be a huge work for an app that is in a maintenance mode. @pixlwave showed some failure cases but there are probably more. Element X iOS already support it.

NicolasBuquet commented 11 months ago

Thank you guys to take time to review this.

I don't see the exposed behavior of wrong hitTest to react to a particular message on my real device.

I understand that Element iOS is in maintenance mode and won't receive this kind of ambitious update.

I will see if I can update the fix to not destroy attributed string when changing font size. But that's only on my side now.

🙏

pixlwave commented 11 months ago

I don't see the exposed behavior of wrong hitTest to react to a particular message on my real device.

From your screenshots it looks like you have bubbles enabled. The timeline behaves very differently in this case, I would suggest testing any changes like this with them disabled as the plain mode merges multiple messages into a single text view.

NicolasBuquet commented 10 months ago

So I progress on my side :

But I will stop here for this.