LeoNatan / LNPopupController

A framework for presenting view controllers as popups of other view controllers, much like the Apple Music and Podcasts apps.
MIT License
3.04k stars 342 forks source link

Larger text sizes / content categories look bad #545

Closed LeoNatan closed 1 year ago

LeoNatan commented 1 year ago

IMG_5881

LeoNatan commented 1 year ago

@iDevelopper I remember you said the toolbar items were not centered for you. Do you, by chance, have larger text size on your iPhone? I see there is a bug with UIToolbar does not center items properly when larger text sizes are used. 🤦‍♂️

iDevelopper commented 1 year ago

No, I tested with no larger text size.

In safeSystemImages.m, if you return:

    return [[[UIImage systemImageNamed:named withConfiguration:config] imageWithAlignmentRectInsets:UIEdgeInsetsZero] imageWithoutBaseline];

They are better centered, but there is still a small gap of 0.33 (height of a shadow line, but which is not present).

10,67 + 22,33 + 10,67 = 43,67 (10,67 + 22,33 + 11 = 44)

Capture d’écran 2023-10-06 à 10 10 07
LeoNatan commented 1 year ago

That’s something that the toolbar manages internally. 0.3333pt is 1px in modern iPhones, so it’s not very noticeable. But with larger text sizes, it goes quite off-center.

iDevelopper commented 1 year ago

It shouldn't! The problem is that you calculate the frame of the labels based on the size of the font.

        if(_resolvedStyle == LNPopupBarStyleCompact)
        {
            titleLabelFrame.origin.y -= _titleLabel.font.lineHeight / 2 - 1;
            subtitleLabelFrame.origin.y += _subtitleLabel.font.lineHeight / 2 + 1;
        }
        else
        {
            titleLabelFrame.origin.y -= _titleLabel.font.lineHeight / 2.1;
            subtitleLabelFrame.origin.y += _subtitleLabel.font.lineHeight / 1.5;
        }
LeoNatan commented 1 year ago

What has that code anything to do with toolbar?

iDevelopper commented 1 year ago

This is your code to calculate the frame of the labels and with larger text sizes, the labels are out of bounds (y = - 21...)

LeoNatan commented 1 year ago

The labels don’t fit, because the bar is too short. If it was higher, the layout would have been correct. I just need to make the height of the bar increase beyond certain text size category, especially the accessibility sizes.

iDevelopper commented 1 year ago

This is not the behavior of the bars of navigation controller or tab bar controllers. We could also limit the text size to a maximum value so that the text is adjusted.

LeoNatan commented 1 year ago

Yes, that is also an option. I will see how taller bar looks like and decide.

LeoNatan commented 1 year ago

I think I will offer both options. By default, the bar will increase height for larger content categories. I will add a property that will allow users to limit the content category support to such that will not increase the bar height.

LeoNatan commented 1 year ago

There already exists https://developer.apple.com/documentation/uikit/uiview/3750923-maximumcontentsizecategory which I will use.

iDevelopper commented 1 year ago

Yes, I couldn't get this to work correctly!

LeoNatan commented 1 year ago

It just works for me, no code needed. Just set for example XXL as the max category, and it didn’t grow above.

iDevelopper commented 1 year ago

On titleLabel for example?

    _titleLabel.textColor = self._titleColor;
    _titleLabel.font = self._titleFont;
        if (@available(iOS 15.0, *)) {
             _titleLabel.maximumContentSizeCategory = UIContentSizeCategoryExtraSmall;
         }

It doesn't change anything for me, I must be missing something!

LeoNatan commented 1 year ago

No, set it directly on the LNPopupBar.

iDevelopper commented 1 year ago

Ok, not working with Marquee Labels enabled.

    if (@available(iOS 15.0, *)) {
        targetVC.popupBar.maximumContentSizeCategory = UIContentSizeCategorySmall;
    }
iDevelopper commented 1 year ago

With marqueeScrollEnabled = NO, the bar is too big or poorly placed if you go to settings and come back for example.

- (void)viewDidLoad
{
    [super viewDidLoad];

#if LNPOPUP
    _demoVC = [IntroWebViewController new];

    self.navigationController.popupBar.barStyle = LNPopupBarStyleFloating;
    self.navigationController.popupBar.standardAppearance.marqueeScrollDelay = 0.0;
    self.navigationController.popupBar.standardAppearance.marqueeScrollEnabled = NO;
#endif
}

- (void)viewDidAppear:(BOOL)animated
{
    [super viewDidAppear:animated];

#if LNPOPUP
    self.navigationController.view.tintColor = self.navigationController.navigationBar.tintColor;
    [self.navigationController presentPopupBarWithContentViewController:_demoVC animated:YES completion:nil];
    if (@available(iOS 15.0, *)) {
        self.navigationController.popupBar.maximumContentSizeCategory = UIContentSizeCategorySmall;
    }

    [self.navigationController.popupBar addInteraction:[LNPopupDemoContextMenuInteraction new]];
#endif
}

Capture d’écran   2023-10-08 à 10 22 24

Capture d’écran   2023-10-08 à 10 35 24

With marqueeScrollEnabled = YES, maximumContentSizeCategory does not work.

Capture d’écran   2023-10-08 à 10 41 23

LeoNatan commented 1 year ago

Try setting it before the bar is presented.

iDevelopper commented 1 year ago

Already done!

LeoNatan commented 1 year ago

And you still see issues?

iDevelopper commented 1 year ago

Yes, I also tried with a new basic project with only MarqueeLabel and maximumContentSizeCategory does not work.

LeoNatan commented 1 year ago

Regarding MarqueeLabel, it could be an issue with that framework. I’ll test and see.

LeoNatan commented 1 year ago

LOL It doesn't work in the demo project because I override the fonts in the controller: https://github.com/LeoNatan/LNPopupController/blob/42ae30d719aa20e0f6b013730ec54b0f849639a2/LNPopupControllerExample/LNPopupControllerExample/TestingScene/IntroWebViewController.m#L23

iDevelopper commented 1 year ago

No,

https://github.com/cbpowell/MarqueeLabel/issues/295

LeoNatan commented 1 year ago

I’ll take a closer look. My repo contains a fork of the framework. Without looking, I’m guessing they are looking at the device content size category, not the local one.

iDevelopper commented 1 year ago

I found that MarqueeLabel should have a setter for adjustsFontForContentSizeCategory which is missing:

In MarqueeLabel.m

- (void)setAdjustsFontForContentSizeCategory:(BOOL)adjustsFontForContentSizeCategory {
    [super setAdjustsFontForContentSizeCategory:adjustsFontForContentSizeCategory];
    self.subLabel.adjustsFontForContentSizeCategory = adjustsFontForContentSizeCategory;
}

In MarqueeLabel.swift

    open override var adjustsFontForContentSizeCategory: Bool {
        didSet {
            sublabel.adjustsFontForContentSizeCategory = self.adjustsFontForContentSizeCategory
        }
    }
LeoNatan commented 1 year ago

You are correct, but it works without that somehow. I don't understand why. Maybe adjustsFontForContentSizeCategory is YES by default, so it works.

LeoNatan commented 1 year ago

Somehow, the sublabel gets an incorrect trait collection:

UICTContentSizeCategoryAccessibilityXXXL
UICTContentSizeCategoryAccessibilityXXXL
UICTContentSizeCategoryAccessibilityXXXL
UICTContentSizeCategoryAccessibilityXXXL
UICTContentSizeCategoryAccessibilityXXXL
UICTContentSizeCategoryAccessibilityXXXL
UICTContentSizeCategoryAccessibilityXXXL
UICTContentSizeCategoryAccessibilityXXXL
UICTContentSizeCategoryXL
UICTContentSizeCategoryXL
UICTContentSizeCategoryAccessibilityXXXL
UICTContentSizeCategoryAccessibilityXXXL

Notice how, after I set it to XL as the maximum, it gets called with XL, but then somehow it gets AXXXL, while the MarqueeLabel itself doesn't (it only gets XL).

iDevelopper commented 1 year ago

Actually I am testing on a basic project without LNPopupController.

LeoNatan commented 1 year ago

Yes, if the issue happens in my project, it will happen in an empty project.

LeoNatan commented 1 year ago

@iDevelopper Adding setAdjustsFontForContentSizeCategory: fixes the marquee label. You should submit a PR for the Swift version of the library.

LeoNatan commented 1 year ago

I will also fix the issue where the popup bar looks bad if the max/min category is set while it's presented.