🐛 Use anchor tags outside tooltip for linking #32181
Conversation
|
Hey @gmajoulet, @newmuis, @Enriqe! These files were changed: |
|
High-level directional question: Is it possible to modify linker to be able to inspect links in shadow roots? The approach in this PR means we have to manually do this for all links in shadow DOM, and seems prone to us missing one somewhere (which, we're already missing some). |
|
From the current way of the linker handles clicks, it seems that you'd need to attach a click listener to all the shadow roots, which is not easy because there's no way to know when they are created (or easily target them with CSS). We'd need extra code in the linker and each shadow dom, so it's more complex in the end. If there are other places where this happens, I could fix those in this PR as well |
|
Info dialog, bookend (although deprecated), consent dialog, access (maybe? can't remember), interactive components, and inline page attachment come to mind |
|
Oh yeah, looks like it's a number of places where users could potentially use the linker. However, I'd say most of these cases we would (on purpose) prevent linking for privacy reasons or just lack of usefulness (or being deprecated). Enabling linking on specific scenarios might work better than just enabling them on all (and we also avoid more code complexity for now), and we can start adding this functionality where users see a benefit and where we think it's a good use-case. How does that sound? |
|
Ok, diving deeper into that list, I think we can skip bookend (because it's deprecated) and access (it doesn't actually have a shadow root, which is surprising to me, but... 🤷♂️ ) The others, it seems like we should probably do. Can you factor out the cloning into a util? Given that it's hacky, I wouldn't be surprised if we find problems with it down the line in some specific device/browser/content combination, and it'd be nice to not have to update every call site. |
|
Created a function |
I think you should be able to test with the demo page you provided, no? It doesn't work there either? |
|
Demo now works perfectly, I had to add the |
|
Enrique and Matias, since we do |
|
Amp-analytics is triggered at the same time we do this, so they don't seem to clash from what I've seen. If other click events were added to the shadow dom that wanted to catch this click, that could be a problem. But on the analytics side it shouldn't break anything since they don't have access to the shadow dom (which is why this bug occurred) |
There was a problem hiding this comment.
Enrique and Matias, since we do preventDefault() on the actual <a, can you double check the eventual amp-analytics implications? Is there anything we could be breaking?
Did a quick sanity check and it seems to be working correctly. As Matias mentioned since we're only doing event.preventDefault on the shadow DOM we should be good because the events wouldn't bubble up to light DOM normally.
|
I'll add this functionality into |
|
|
* Create tag from tooltip * CloneNode * Move functionality into utils and hide anchor * Added example and renamed util * Remove unused imports
Linker cannot access anchor tag from tooltip since it's inside the shadow dom, so we create an anchor outside the shadow root and click it when the tooltip is clicked. Creating the tag and clicking it is considered a trusted user interaction since it's called inside the original click event listener
Demo at https://stories-demos-matias.web.app/examples/amp-story/analytics.html#page=p3. Clicking on the bottom and top links adds the cid to the url
Closes #32060