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

Reapply #1929 and fix issue where header title uses incorrect font #2040

Merged
merged 6 commits into from
Jun 21, 2024

Conversation

nodes11
Copy link
Contributor

@nodes11 nodes11 commented May 31, 2024

Platforms Impacted

  • iOS
  • visionOS
  • macOS

Description of changes

Issue:
#1929 ensured that the TableViewHeaderFooterView does not override any font or color provided as part of the AttributedString. However, an issue was observed where headers that should be headerPrimary are showing up as header and vice versa. The change was reverted in #1945.

We were also seeing an issue where the title font would change after scroll where the title goes off screen.

This change reapplies #1929 and fixes the issue where headers that should be headerPrimary are showing up as header and vice versa, as well as the issue where the title font changes after it goes off screen.

Root Cause:

  1. In TableViewHeaderFooterView should not override any font or color provided as part of the AttributedString #1929, if no attributed string was provided, the resolved resolvedTitleFont and resolvedTitleColor were only set once at inside setup(...). This call occurs before the tokenSet is updated with overrides and the resolvedTitleFont would be set to the wrong font as a result.
  2. The setup method wasn't correctly "recycling" the views when dequeueReusableHeaderFooterView was called, so resolvedTitleFont and resolvedTitleColor properties were still set in some cases and the values would be used when rendering the header for a different header with a different font.

Fix:

  • Change resolvedTitleFont and resolvedTitleColor to attributedTitleFont and attributedTitleColor as they'll only be used to hold values for the attributed string.
  • Replace usages of resolvedTitleFont and resolvedTitleColor as an r-value with attributedTitleFont ?? tokenSet[.textFont].uiFont and attributedTitleColor ?? tokenSet[.textColor].uiColor.
  • Cleanup the setup methods so they call setupBase(...) with their parameters. setupBase handles the cleanup and ensures all the properties are set in the same order for call cases.

Binary change

Total increase: 19,968 bytes
Total decrease: -808 bytes

File Before After Delta
Total 30,874,424 bytes 30,893,584 bytes ⚠️ 19,160 bytes
Full breakdown
File Before After Delta
TableViewHeaderFooterView.o 277,496 bytes 291,664 bytes ⚠️ 14,168 bytes
__.SYMDEF 4,751,744 bytes 4,755,896 bytes ⚠️ 4,152 bytes
FocusRingView.o 832,280 bytes 833,752 bytes ⚠️ 1,472 bytes
PopupMenuSectionHeaderView.o 24,504 bytes 24,680 bytes ⚠️ 176 bytes
PopupMenuController.o 376,616 bytes 375,808 bytes 🎉 -808 bytes

Verification

  • Validated that the header titles show in the correct font.
    • Verified that the font displayed matches the style listed in TableViewHeaderFooterSampleData
    • Verified that the font displayed matches the screenshots in the After category of this Revert #1929 #1945
Visual Verification

Before

upstream.main.head.mov

After

feature.head.mov

Pull request checklist

This PR has considered:

  • Light and Dark appearances
  • iOS supported versions (all major versions greater than or equal current target deployment version)
  • VoiceOver and Keyboard Accessibility
  • Internationalization and Right to Left layouts
  • Different resolutions (1x, 2x, 3x)
  • Size classes and window sizes (iPhone vs iPad, notched devices, multitasking, different window sizes, etc)
  • iPad Pointer interaction
  • SwiftUI consumption (validation or new demo scenarios needed)
  • Objective-C exposure (provide it only if needed)
Microsoft Reviewers: Open in CodeFlow

@nodes11 nodes11 requested a review from a team as a code owner May 31, 2024 00:34
@nodes11 nodes11 force-pushed the fix_tableheaderfooter_view branch from 3424f8c to 2b3d1a9 Compare June 17, 2024 22:52
owenconnolly and others added 6 commits June 21, 2024 07:50
…ded as part of the AttributedString (microsoft#1929)

* Update CommandBarButton.swift

* Update CommandBarButton.swift

* Command handled event should pass the event source in the handler

* Fix spacing

* Fix lint errors and make updates for some review comments

* Do not override font or color provided in the attributed string

* Fix spacing

* Fix update of titleView font

* Updates for PR feedback
@nodes11 nodes11 force-pushed the fix_tableheaderfooter_view branch from 2b3d1a9 to ce077c4 Compare June 21, 2024 14:50
@nodes11 nodes11 enabled auto-merge (squash) June 21, 2024 14:51
@nodes11 nodes11 merged commit 744a205 into microsoft:main Jun 21, 2024
7 checks passed
@mischreiber mischreiber mentioned this pull request Jun 24, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants