Skip to content

Conversation

@devknoll
Copy link
Contributor

When collecting nodes from the DOM for hydration, we need to treat nodes with paths (e.g. content projection) as the new root for all subsequent elements, not just the next one.

Additionally, when using content projection it's possible for translated content to become disconnected, e.g. when it doesn't match a selector and there isn't a default. We need to handle such cases by manipulating the disconnected node data associated with hydration as usual.

Fixes #56176

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #56176

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@devknoll thanks for the fix 👍 The code looks great, just left a couple minor comments.

@devknoll devknoll force-pushed the x-improve-i18n-content-projection branch from 27861d2 to 4dbf906 Compare June 4, 2024 20:02
@devknoll devknoll force-pushed the x-improve-i18n-content-projection branch 2 times, most recently from ce91198 to 00b2c30 Compare June 5, 2024 17:11
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@devknoll looks great, thanks for the fix! I've left a few minor comments, but I think this PR is ready for final review (please switch it to the "Ready for review" state when you get a chance).

@devknoll devknoll force-pushed the x-improve-i18n-content-projection branch from 00b2c30 to 3e80491 Compare June 6, 2024 18:13
@devknoll
Copy link
Contributor Author

devknoll commented Jun 6, 2024

@AndrewKushnir The previous iteration using isICUChild ended up feeling a bit hacky, as it was unable to do any checking to actually validate that ICU child nodes were never actually disjoint.

The new iteration adds a getFirstNativeNodeForI18nNode, which allows all of the isDisjointNode stuff to work unconditionally, and allows a (future) failure of the assertion to correctly be revealed and addressed.

@devknoll devknoll marked this pull request as ready for review June 6, 2024 18:33
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: i18n Issues related to localization and internationalization target: patch This PR is targeted for the next patch release core: hydration labels Jun 6, 2024
@ngbot ngbot bot added this to the Backlog milestone Jun 6, 2024
@devknoll devknoll force-pushed the x-improve-i18n-content-projection branch from 3e80491 to b456f4a Compare June 14, 2024 16:28
@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Jun 14, 2024
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@devknoll thanks for addressing the feedback 👍

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 14, 2024
When collecting nodes from the DOM for hydration, we need to treat nodes with paths (e.g. content projection) as the new root for all subsequent elements, not just the next one.

Additionally, when using content projection it's possible for translated content to become disconnected, e.g. when it doesn't match a selector and there isn't a default. We need to handle such cases by manipulating the disconnected node data associated with hydration as usual.
@devknoll devknoll force-pushed the x-improve-i18n-content-projection branch from b456f4a to 29fdd6e Compare June 14, 2024 18:32
@angular-robot angular-robot bot requested a review from AndrewKushnir June 14, 2024 18:32
@AndrewKushnir
Copy link
Contributor

@devknoll would it be ok to proceed with the next steps for this PR (presubmit and merge) or do you plan to perform some additional changes?

@devknoll
Copy link
Contributor Author

@AndrewKushnir No additional changes planned at the moment 👍

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Jun 20, 2024
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: presubmit The PR is in need of a google3 presubmit labels Jun 20, 2024
@AndrewKushnir
Copy link
Contributor

Caretaker note: presubmit is "green", this PR is ready for merge.

@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit 29ca6d1.

The changes were merged into the following branches: main, 18.0.x

AndrewKushnir pushed a commit that referenced this pull request Jun 20, 2024
…56192)

When collecting nodes from the DOM for hydration, we need to treat nodes with paths (e.g. content projection) as the new root for all subsequent elements, not just the next one.

Additionally, when using content projection it's possible for translated content to become disconnected, e.g. when it doesn't match a selector and there isn't a default. We need to handle such cases by manipulating the disconnected node data associated with hydration as usual.

PR Close #56192
@devknoll devknoll deleted the x-improve-i18n-content-projection branch June 21, 2024 16:41
henry-alakazhang pushed a commit to freelancer/angular that referenced this pull request Jun 26, 2024
…ngular#56192)

When collecting nodes from the DOM for hydration, we need to treat nodes with paths (e.g. content projection) as the new root for all subsequent elements, not just the next one.

Additionally, when using content projection it's possible for translated content to become disconnected, e.g. when it doesn't match a selector and there isn't a default. We need to handle such cases by manipulating the disconnected node data associated with hydration as usual.

PR Close angular#56192
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 22, 2024
henry-alakazhang pushed a commit to freelancer/angular that referenced this pull request Aug 30, 2024
…ngular#56192)

When collecting nodes from the DOM for hydration, we need to treat nodes with paths (e.g. content projection) as the new root for all subsequent elements, not just the next one.

Additionally, when using content projection it's possible for translated content to become disconnected, e.g. when it doesn't match a selector and there isn't a default. We need to handle such cases by manipulating the disconnected node data associated with hydration as usual.

PR Close angular#56192
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime area: i18n Issues related to localization and internationalization core: hydration merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

i18n hydration fails when multiple elements are inside an i18n tag

2 participants