Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[ios, macos] Adapt Mapbox Streets–sourced layers for Dynamic Type. #9734

Conversation

fabian-guerra
Copy link
Contributor

Fixes #7030
WIP

@fabian-guerra
Copy link
Contributor Author

fabian-guerra commented Aug 8, 2017

One requirement I didn't account initially was the fact that custom styles have different font types and scaling fonts have different results according to the type face.

This is a 30% scale in the default style (left) and in my custom style. The labels have different typefaces with different results for the same scale increase. In this implementation I need to consider the typeface otherwise I will break the UX for the user.
stylecomparison 2

/cc @boundsj @1ec5

Copy link
Contributor

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that we could implement this more generically in Core. There are a few issues with the approach in this PR:

  • Doesn't work for layers that use a data-driven size, a zoom-level dependent size, or an expression
  • Alters the stylesheet which makes it impossible to revert to the original size
  • Implementation logic is specific to iOS/macOS, but Android/Qt/Node could benefit from this too.

@kkaefer kkaefer added the iOS Mapbox Maps SDK for iOS label Aug 16, 2017
@felixLam
Copy link

Our implementation keeps a reference to all existing values before manipulating it which allows us to revert to the original value and to listen for changes. It also works for zoom level driven styles and should be easy enough to extend to support data driven style values too:

https://github.com/iosphere/ISHMapboxDynamicFontObserver

@felixLam
Copy link

I would also make this behavior opt-in as this may look odd in apps that otherwise do not support dynamic type.

@fabian-guerra fabian-guerra self-assigned this Aug 21, 2017
@fabian-guerra
Copy link
Contributor Author

fabian-guerra commented Aug 21, 2017

@kkaefer

Doesn't work for layers that use a data-driven size, a zoom-level dependent size, or an expression

The scope for this PR was to try only in Mapbox streets layers.

Alters the stylesheet which makes it impossible to revert to the original size

Right we need to implement this mechanism.

Implementation logic is specific to iOS/macOS, but Android/Qt/Node could benefit from this too.

Agree, I need to research how this is handled on each platform.

For this feature I discussed with @boundsj that we are not going to consider the customized font types. If you look at the image same weight applied to different font types have a different results thus implementing this as an opt-in.

@felixLam
Copy link

@fabian-guerra could you elaborate why "custom fonts" are an issue? We are using the scaling provided in our repository without any issue using a custom glyph atlas.

@fabian-guerra
Copy link
Contributor Author

@felixLam sure. Just in case when I say font type I refer to typeface.
In the first picture circled and squared are labels with the typeface Open Sans, on the right Frutiger Next. I applied the same increment on both. The result visually is different and is related to the typeface itself. So to account for something similar on both we would have to create a Typeface / weight table. Since we support customization this will become impossible to achieve. That's why I am saying we will make this opt-in and we will consider visually only the typeface in the default style.

@felixLam
Copy link

One option would be to allow developers to provide their own increments, like we do here:
https://github.com/iosphere/ISHMapboxDynamicFontObserver/blob/master/ISHMapboxDynamicFontObserver.m#L148

@fabian-guerra
Copy link
Contributor Author

@felixLam thank you. I will take a look.

@1ec5
Copy link
Contributor

1ec5 commented Aug 22, 2017

Implementation logic is specific to iOS/macOS, but Android/Qt/Node could benefit from this too.

Are accessible type settings uniform across these platforms? We should avoid a situation where we’re limited to a lowest common denominator of accessibility settings because of a desire to implement them all in core GL.

@kkaefer
Copy link
Contributor

kkaefer commented Aug 23, 2017

What I'm proposing is to add this feature of scaling fonts to core, rather than implementing it for every platform. This would also allow us to add the scaling operation after function/expression evaluation for text-size.

@kkaefer
Copy link
Contributor

kkaefer commented Aug 23, 2017

...sort of like pixelRatio. If a platform supports adjustable type, or if a developer decides to add it to just their app, the platform SDK has to make a call to this API, rather than having to go through all layers, adjust the setting manually with runtime styling and having to manage state about what layers it already touched, and needing to monitor new layers being added so that it can apply the setting as well.

@1ec5
Copy link
Contributor

1ec5 commented Aug 23, 2017

Less work on the SDK’s part sounds great. 😄 What would the SDK tell core GL exactly? A universal scale factor would be simpler to implement, but the intent behind Dynamic Type is that labels would be scaled up based on their semantic roles (content size categories). For example, if we bake in Mapbox Streets source layer names, we could associate a country label with UIFontTextStyleTitle1, which UIKit could adjust at a different rate than UIFontTextStyleHeadline or UIFontTextStyleBody. (Technically a text style can specify a font face, weight, etc., but let’s ignore that for now.)

Would it be feasible to provide core GL with a table mapping source layer names to font sizes, rather than a universal scale factor?

@kkaefer
Copy link
Contributor

kkaefer commented Aug 23, 2017

Would it be feasible to provide core GL with a table mapping source layer names to font sizes, rather than a universal scale factor?

It would... however that leaves us with the same problem: When the user adds a custom layer, the SDK doesn't know about it and we don't scale that layer, unless we implement some sort of fallback scale value. Overall, I'm not a big fan of hardcoding logic that is geared to specific stylesheets. Adjusting individual labels at a different rate for maps seems like overengineering to me.

@fabian-guerra
Copy link
Contributor Author

When the user adds a custom layer, the SDK doesn't know about it and we don't scale that layer, unless we implement some sort of fallback scale value.

We could provide an API delegate that takes care of this.

Adjusting individual labels at a different rate for maps seems like overengineering to me.

It may look but when you see an example it makes sense. I recommend watch this video and if you have an iPhone change the text size and see how Apple Maps reflects the changes.

@1ec5
Copy link
Contributor

1ec5 commented Aug 23, 2017

It would... however that leaves us with the same problem: When the user adds a custom layer, the SDK doesn't know about it and we don't scale that layer, unless we implement some sort of fallback scale value.

To clarify, I was referring to source layers like the ones documented here, not style layers. The SDK already has some logic specifically intended for the Mapbox Streets source as of #9582.


if ([layer.textFontSize isKindOfClass:[MGLConstantStyleValue class]]) {
NSNumber *textFontSize = [(MGLConstantStyleValue<NSNumber *> *)layer.textFontSize rawValue];
textFontSize = [NSNumber numberWithFloat:(textFontSize.floatValue * [self sizeForContentSizeCategory:preferredContentSize])];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could genericize the expression transformation in #11651 so that it could also be used to scale fonts according to the style layer’s content size category.

@stale
Copy link

stale bot commented Oct 24, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the archived Archived because of inactivity label Oct 24, 2018
@stale
Copy link

stale bot commented Oct 27, 2018

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this Oct 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived Archived because of inactivity iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants