Add fontFeatureSettings and resolve font features in shared C++ - #58266
Add fontFeatureSettings and resolve font features in shared C++#58266Abbondanzo wants to merge 2 commits into
Conversation
Summary: `hash_combine` mixes each field into the previous seed, so it forms a dependency chain the CPU cannot overlap and an unset optional still costs a full link. `hash_combine_optionals` folds a run of optionals into one presence-mask link plus the engaged values, so a further optional costs a bit in the mask rather than a link. The mask is what keeps it collision-free: skipping disengaged fields alone would make the same value in two different slots hash identically. Equality moves from `std::tie` to a short-circuit chain ordered cheapest first, with the string and vector fields last, because the dominant caller is a successful cache lookup where the keys are equal and every field has to be examined. Changelog: [Internal] - Hash optional `TextAttributes` fields via a presence mask and compare them with a short-circuit chain Differential Revision: D115621401
Summary: Adds the `fontFeatureSettings` style prop, and moves feature-list composition out of the two per-platform tables into one function in shared C++ so both renderers resolve precedence identically. Contributions are emitted least- to most-specific, so a tag named more than once resolves last-wins and no separate conflict-resolution step has to be kept in sync. Also fixes a pre-existing bug: the Android span-creation guards did not test `fontFeatureSettings`, so a fragment whose only font prop was `fontVariant` produced no span at all and dropped its features. Changelog: [General][Added] - Add `fontFeatureSettings` style prop to `<Text>` [General][Fixed] - A text run whose only font prop was `fontVariant` produced no styled span on Android and silently dropped its features Differential Revision: D115621392
meta-cla
Bot
added
the
CLA Signed
|
@Abbondanzo has exported this pull request. If you are a Meta employee, you can view the originating Diff in D115621392. |
|
Warning JavaScript API change detected This PR commits an update to
This change was flagged as: |
|
/review |
🤖 AI code reviewDecision: Ready for human review (with comments) Overall PR risk: Medium. The change modifies existing behavior for text attribute parsing, fontVariant resolution, equality and hash, and Android and iOS text spans, so blast radius is text rendering on both platforms with direct rollback by revert. The most plausible break is platform divergence where Android applies fontFeatureSettings while iOS drops it, plus loss of slant for angled oblique values. Coverage was reduced because the contracts-api-codegen review ran out of time, so findings are incomplete and absence of other findings does not mean the change is clean.
🟡 Warning (2)
This review is advisory — it never blocks a merge and never auto-approves. |
|
/review |
3 similar comments
|
/review |
|
/review |
|
/review |
1 participant
Summary:
Adds the
fontFeatureSettingsstyle prop, and moves feature-list composition out of the twoper-platform tables into one function in shared C++ so both renderers resolve precedence
identically. Contributions are emitted least- to most-specific, so a tag named more than once
resolves last-wins and no separate conflict-resolution step has to be kept in sync.
Also fixes a pre-existing bug: the Android span-creation guards did not test
fontFeatureSettings,so a fragment whose only font prop was
fontVariantproduced no span at all and dropped itsfeatures.
Changelog:
[General][Added] - Add
fontFeatureSettingsstyle prop to<Text>[General][Fixed] - A text run whose only font prop was
fontVariantproduced no styled span on Android and silently dropped its featuresDifferential Revision: D115621392