Repository navigation
widgets: give the home-screen vitals footer the app's own HR/HRV icons - #1798
Merged
Merged
Conversation
The footer under the score rings paired heart rate with waveform.path.ecg and HRV with heart.fill - the inverse of the pairing every other surface uses. TodayView renders the two as adjacent rows the other way round, and DashboardCards, TodayCustomizationMetadata, MetricCatalog and this extension's own Live Activity all agree with TodayView. The footer has been the lone outlier since #1022. It matters more here than a mismatched glyph usually would, because the footer shows no text labels at all: the icon is the only thing naming each number. The inversion was an understandable one, and the doc comment now names it so it does not get "corrected" back. waveform.path.ecg IS the right symbol for Live HR as a feature - RootView, RootTabView and HomeScreenQuickActions all use it for the Live screen. As a metric icon it is HRV's, and this footer is the one place that draws both metrics side by side. Also gives the three vitals accessibility labels and values. They were bare numbers with no unit and no name, so VoiceOver read "58", "64", "84 percent" - three anonymous figures. This is the same gap #1715 fixed for the lock-screen accessory ("an icon says nothing to VoiceOver"); the footer predates that change and never got it. Literals are plain rather than String(localized:), for the reason already documented on accessoryScore: the extension does not carry the app's string catalog. Reported in #1795 by @Sneheth.
Re-review: vital() set accessibilityLabel/accessibilityValue without first calling accessibilityElement(children: .ignore), making it the only labelled graphic in this file that does not. accessoryScore and WidgetScoreRing both collapse first. It is not cosmetic. Label under .titleAndIcon renders an image beside a text, so without the collapse the bare number remains its own accessibility element and whether the label wins is SwiftUI's container semantics rather than something this file decides - and a widget cannot be checked for that here. Matching the two sites that already work is strictly safer than relying on how Label happens to decompose.
statCell renders the value, then the unit as a separate view, then an abbreviated caption. Read as-is that is "64", "ms", "HRV" - three fragments in value-before-label order, with ms and bpm spelled out letter by letter. Less severe than the footer, because the caption at least names the metric somewhere, but still not a reading anyone would choose. Collapses the cell like the other three labelled graphics in this file and gives it a spoken name and unit, so it reads "Heart rate variability, 64 milliseconds". spoken falls back to the rendered value rather than to "No data", so a caller that omits it degrades to the old reading instead of asserting something false.
Also to add to this PR, I also feel the lock screen widgets are slightly misaligned if you observe the central effort component. And I also feel that HR on lock screen widget is pointless since it is not realtime anyway and offsets the entire widget look |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported in #1795 by @Sneheth, who is right: in the home-screen widget footer the HR and HRV
values read backwards against their icons.
The bug
vitalsFooterpairedsnap.bpmwithwaveform.path.ecgandsnap.hrvwithheart.fill—the inverse of the pairing used everywhere else in the app:
TodayView:2816-2820(adjacent rows)waveform.path.ecgheart.fillTodayCustomizationMetadata:44-45waveform.path.ecgheart.fillDashboardCards:88-89waveform.path.ecgheart.fillMetricCatalog:134waveform.path.ecgNOOPLiveActivity:39(same extension)heart.fillNOOPWidget:278-281(this footer)heart.fillwaveform.path.ecgIt matters more than a mismatched glyph normally would: the footer renders no text labels, so
the icon is the only thing naming each number.
Not a regression — the footer has been this way since #1022, and it is untouched by the 11.0.0
diff. Unrelated to the other widget report going around.
Why it happened, recorded so it is not "corrected" back
waveform.path.ecgIS the right symbol for Live HR as a feature —RootView:126,RootTabView:640andHomeScreenQuickActions:32all use it for the Live screen. As a metricicon it belongs to HRV, and this footer is the single place that draws both metrics side by
side, which is where the collision becomes the whole problem. The doc comment says this.
Second defect, same four lines
The three vitals were bare numbers with no unit and no name, so VoiceOver read "58", "64",
"84 percent" — three anonymous figures. Added
accessibilityLabel/accessibilityValue,carrying
spokenseparately from the rendered text so the unit reaches a reader who cannotsee the glyph.
This is exactly the gap #1715 fixed for the lock-screen accessory ("an icon says nothing to
VoiceOver"). The footer predates that change and never got the treatment. Literals stay plain
rather than
String(localized:), for the reason already documented onaccessoryScore: thisextension does not carry the app's string catalog, so
String(localized:)would look localisedand render English regardless.
Verification
swiftc -parseclean; brace balance checked on the touched region.Tools/doc_comment_lint.pyexit 0,Tools/i18n_audit.py --ciexit 0..labelStyle(.titleAndIcon)still propagates through thehelper, the
–placeholder and the!compactgate on HRV are preserved, and the diffcontains nothing beyond the icon swap and the accessibility modifiers.
StrandiOSWidgets, whichswift-packages.ymldoes not build andapp-build.ymlis disabled by default. Needs anapp-build dispatch before merge.
StrandTests, which no default CI runs,so a pin on the icon pairing would rot silently. The doc comment carries the reasoning
instead. Happy to add one if you would rather have it.
systemSmallandsystemMediumonly.