-
Notifications
You must be signed in to change notification settings - Fork 6
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
Automatically include accessibility stats #59
base: main
Are you sure you want to change the base?
Conversation
@kkostov Looks good in general, but I'm worried about some fields mismatching the existing naming and therefore creating a need to adjust insights or create new ones. And some need to be removed due to sensitivity. For example, the following two sound like the same thing for me:
They are both related to font weight. Could you not merge them into one? E.g. by using
This sounds very similar to
Yeah, that's my bad for not giving a more generic name. But please note that we received feedback from affected users in the community and therefore removed this option anyways, alongside switch control. See TelemetryDeck/SwiftSDK#222.
I'm not sure what the accessibility button is, but it sounds similar to "AssistiveTouch" on iOS. Please note that I had considered adding if this setting is enabled but decided against it, because it has no influence on development. There's nothing in UI/UX design a developer needs to consider differently when they have many users having this feature enabled. Users can place it anywhere and might use it for a wide variety of things. From my point of view, there's nothing useful we can learn from it, so we should follow the minimal data principle and not collect it. If this is true for your value as well, consider removing it.
I totally missed this, but we have it on iOS as well:
Like I said above, this too is sensitive information, please remove it. |
Thank you for the fast reply, @Jeehut here are some comments on this: TelemetryDeck.Accessibility.fontWeightAdjustment
To be clear, TelemetryDeck.Accessibility.fontScale
I'm not sure a 1:1 mapping produces the expected results, as there is no direct equivalent to content size categories. It's usually a device vendor who chooses the density, resolution, type face and font parameters of the system. If we try to approximate the value to a linear-ish scale, we could get:
Of course, the caveat is that this is not guaranteed to be always accurate (e.g. on my lab devices, I have a device which defaults to "extraLarge" and another on "medium"). TelemetryDeck.Accessibility.isVoiceOverEnabled, TelemetryDeck.Device.isAccessibilityButtonSupported, TelemetryDeck.Accessibility.isAudioDescriptionRequested, TelemetryDeck.Accessibility.isSwitchControlEnabledGood catch, will have it removed! |
@kkostov As we just chatted on Slack, for future reference, here's Claude's summary of my inputs in the chat:
|
@Jeehut Thanks, but I think I can do better than Claude ;) In summary, we agreed to send the actual font scale factor as a separate value in addition to a mapping compatible with iOS. The mapping to iOS categories will be done so that "large" corresponds to scale 1.0 in order to align with iOS defaults.
The following is the default font scale (1.0) on a recent One UI Samsung device, it will be mapped to "large". The remaining 5 scale factors will be spread over extraLarge, extraExtraLarge and extraExtraExtraLarge. The 2 smaller factors will be mapped to medium and small. extraSmall would be unused and reserved only for devices which offer an even smaller factor. |
@kkostov There are, in fact, even larger sizes than |
@Jeehut sounds great, let's do this 🚀 |
@kkostov I just checked and we are actually already reporting accessibilities values as well. They report as That was not an intended name, but if that works for you, we might want to keep it? Maybe an explanation is needed for developers who don't know that An alternative name that keeps the mapping could be |
@Jeehut Could you break this down for me using the actual values to be sent? For example, based on the above, So your proposal is to rename Regarding the |
@kkostov I do not understand the question, to be honest. But because these names are system given (S to XXXL, and AccessibilityM to AccessibilityXXXL) I think we need to stick to those values so developers can make the connection to the system naming on iOS. If we start mapping say AccessibilityXL to XXXXXL I'm not sure if that's very helpful, then we both need to provide a table. All that really matters to me is that the default is the same group across platforms, the mapping is not confusing, and the group names are consistent. So, apart from that, what was your question again? |
@Jeehut I must admit I also don't fully grasp your comment, so let's try to bridge the gap. Putting aside the OS specifics for a moment. Your request was to create a string enumeration instead of sending
In the comment above, you also want to add AccessibilityM, AccessibilityL, AccessibilityXL, AccessibilityXXL, and AccessibilityXXL. I think if you can fill the table with the corresponding |
I can't rate those fontScale numbers. I would expect that you try what values you get with some common devices and than map like this, while using "L" as the default value: Other than that, the names sent via parameter should not be long as in The full list: I don't think you need "unspecified", given the mapping in the above image, you could simply use I hope it is somewhat clear now. |
@Jeehut Thank you - all I needed was |
Fixes #42
TelemetryDeck.Accessibility.isBoldTextEnabled
- is available on API 31 and aboveTelemetryDeck.Accessibility.fontWeightAdjustment
- is Android only, the value equals 0 if no adjustment is appliedTelemetryDeck.Accessibility.isDarkerSystemColorsEnabled
TelemetryDeck.Accessibility.fontScale
- is Android onlyTelemetryDeck.Accessibility.isInvertColorsEnabled
TelemetryDeck.Accessibility.isVoiceOverEnabled
- I kept the name for consistency, but clearly on Android the default service is called TalkBack, alternatives are also possible. The flag returns true if any voice feedback service is enabled.TelemetryDeck.Accessibility.isReduceMotionEnabled
TelemetryDeck.Device.isAccessibilityButtonSupported
- Android only, true if the accessibility button within the system navigation area is supported.TelemetryDeck.Accessibility.isAudioDescriptionRequested
- Android only, true if users want to select soundtrack with audio description by defaultTelemetryDeck.UserPreference.colorScheme
- N/A as devices support various themes and styles. With this PR,isDarkerSystemColorsEnabled
returnstrue
if the current theme has dark-mode like features, but the opposite is not necessarily true.TelemetryDeck.Accessibility.isReduceTransparencyEnabled
TelemetryDeck.Accessibility.isSwitchControlEnabled
TelemetryDeck.Accessibility.preferredContentSizeCategory
- N/A as devices support different font styles.TelemetryDeck.Accessibility.fontScale
andTelemetryDeck.Accessibility.fontWeightAdjustment
have been added to reflect the exact font metrics.TelemetryDeck.Accessibility.shouldDifferentiateWithoutColor
- based onaccessibility_display_daltonizer_enabled
TelemetryDeck.UserPreference.layoutDirection
The README has been updated to include the list above.
@Jeehut @winsmith Please feel free to have a look. Thoughts and comments are always welcome.