Skip to content

feat(router): support custom elements for RouterLink#59567

Closed
jeripeierSBB wants to merge 1 commit into
angular:mainfrom
jeripeierSBB:routerlink-for-custom-elements
Closed

feat(router): support custom elements for RouterLink#59567
jeripeierSBB wants to merge 1 commit into
angular:mainfrom
jeripeierSBB:routerlink-for-custom-elements

Conversation

@jeripeierSBB

Copy link
Copy Markdown
Contributor

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.dev application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Relates to #28345

We as Web Component library authors have the problem that routerLink does not properly work with custom element anchor tags (native anchor tag wrapped in Shadow DOM). This is due to the lack of automatic href updates for elements besides <a>/ <area> and the internal logic of adding a tabindex="0" attribute for non <a>/ <area> elements.

What is the new behavior?

This PR adds a multi provider CUSTOM_ELEMENT_ANCHOR_TAG_NAMES for adding RouterLink compatible custom elements.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@pullapprove pullapprove Bot requested a review from crisbeto January 16, 2025 13:32
@angular-robot angular-robot Bot added detected: feature PR contains a feature commit area: router labels Jan 16, 2025
@ngbot ngbot Bot added this to the Backlog milestone Jan 16, 2025
jeripeierSBB added a commit to sbb-design-systems/lyne-angular that referenced this pull request Jan 21, 2025
@JeanMeche JeanMeche requested a review from atscott March 4, 2025 17:07
@atscott

atscott commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

Thanks for giving this a shot. This is an interesting solution. After discussions with the team I think we'd want to evaluate other, more direct options.

Some options we discussed, some of which could be worth further exploration, others certainly not:

  • always apply a/area behavior for all elements (obviously doesn't work because some need to be made focusable)
  • add a DI token to configure for element types
  • always apply a/area behavior for custom elements only
  • always apply a/area behavior for custom elements with 'href' properties
  • add an input(s) or marker attribute(s) to the directive that explicitly enables the a/area and/or behavior

Unfortunately, we don't have the capacity right now to take time determining the best path forward.

@atscott atscott closed this Mar 7, 2025
@kyubisation

kyubisation commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

Hello @atscott
That is very disappointing, as this is a problem right now, as described in #28345.
This will also probably get worse with the upcoming Reference Target proposal, which is being worked on right now.

We are more than willing to put in the effort, if you give us a direction.

always apply a/area behavior for all elements (obviously doesn't work because some need to be made focusable)

add a DI token to configure for element types
How does this differ from our current proposal?

always apply a/area behavior for custom elements only
That seems to be risky, as there is no guarantee for this to properly work.

always apply a/area behavior for custom elements with 'href' properties
That seems reasonable and not that far off from our proposal. Although there is a possibility that this might conflict with custom element upgrade timing.

add an input(s) or marker attribute(s) to the directive that explicitly enables the a/area and/or behavior
That seems questionable from a developer experience perspective? Or what do you have in mind?

Does this in any way relate to what was said by @mgechev here? https://www.youtube.com/watch?v=CMZHUPSmZx4&t=624s

@atscott

atscott commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

add a DI token to configure for element types

How does this differ from our current proposal?

Noted because we aren't entirely throwing this option out as a possibility.

@atscott

atscott commented Mar 8, 2025

Copy link
Copy Markdown
Contributor

always apply a/area behavior for custom elements with 'href' properties

That seems reasonable and not that far off from our proposal. Although there is a possibility that this might conflict with custom element upgrade timing

Would you be able to do some testing here? IIUC, the observedAttributes are available even before upgrade is called.

@kyubisation

Copy link
Copy Markdown
Contributor

I will create a few test cases. 👍
Just to clarify; The approach would be something like 'href' in el.nativeElement or what did you have in mind?

@atscott

atscott commented Mar 8, 2025

Copy link
Copy Markdown
Contributor

The approach would be something like 'href' in el.nativeElement or what did you have in mind?

Looking for the element in the registry and reading its observed attributes would be more effective, right?

@kyubisation

kyubisation commented Mar 8, 2025

Copy link
Copy Markdown
Contributor

Yes, although that might need to be revisited when Scoped Custom Element Registries lands. Although from my understanding it would just be an additional call to get the declaration (e.g. customElements.get(...) ?? el.nativeElement.customElements?.get(...).

@kyubisation

Copy link
Copy Markdown
Contributor

@atscott I have created #60290

@angular-automatic-lock-bot

Copy link
Copy Markdown

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 Apr 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: router detected: feature PR contains a feature commit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants