Repository navigation
fix: link-external-newwindow: true not respected by default listing - #9469
Conversation
|
I'm a bit confused about the fix here; I would have expected there to be a condition that needs to be checked depending on |
|
That's the thing I did not figure out yet. The option seems to be applied at another level as I did not find anything in Edit: this is why removing this class makes the external new window option works. The option is used as part of a JavaScript script which is added to HTML when the option is true.
|
|
Just a note that this PR is exactly a revert of commit 8c61f7e So I don't know exactly why it was added this way with no-external, but it seems it was done on purpose. I'll ask Charles as he is around. |
|
Yes, I was looking into it to find what was the reason for this to be done only in the default. @dragonstyle in case you recall why you did add the |
|
Do not merge. |
|
I've tested on various website listings (6) and did not see any issues. |
|
I believe what needs to be tested if all the feature that removing From the code it seems to mean
Currently, both of this won't happen to links with After discussing with @dragonstyle last week, he does not remember why exactly but was warning us that it could have been made following some slack discussion for some of our doc website, and that it could trigger other stuff. Now, things may have changed since then, and maybe removing the class won't trigger anything bad 🤔 |
|
For example, this is how it looks if I add We got an icon everywhere - and I don't think we want this really |
|
Indeed, that might be the issue Charles tried to fix. I think the "noexternal" fix is wrong. Wrong in the sense that other listing will have the same issues. What do you think? |
|
Updating this PR with the addition of a CSS rule to get the same display behaviour (no external icon in default listing). Code
---
title: "Test"
link-external-newwindow: true
link-external-icon: true
listing:
contents:
- path: https://www.nature.com/articles/d41586-024-00692-7
title: "China–US climate collaboration concerns as Xie and Kerry step down"
author: Smriti Mallapaty
description: "Gang's quote on the importance to continue the research exchange and colloboration 'If the river freezes over, water will still flow underneath.'"
date: "2024-03-12"
categories: [Nature]
- path: https://www.scientificamerican.com/article/what-the-u-s-china-agreement-means-for-greenhouse-gas-emissions/
title: "What the U.S.-China Agreement Means for Greenhouse Gas Emissions"
author:
- Benjamin Storrow
- Sara Schonhardt
description: "The two nations announced limited steps to address climate change. But even a modest agreement could have far-reaching effects."
date: "2023-11-16"
categories: [Scientific American, E&E News]
sort: "date desc"
categories: true
sort-ui: false
filter-ui: false
page-layout: full
title-block-banner: true
--- |
|
Thanks for working on this PR again. This is all quite a bit messy and, because it's close to reverting a previous commit, I don't think I'm comfortable merging it before adding tests that exercise the expected behavior for these different options. |
|
I'll try to figure suitable tests for behaviour and icons. Switching this to draft for now. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
If |
|
@cscheid, I've reworked this so it no longer reverts the earlier fix. @cderv If you want to follow up on this. |
|
(fixing the issue with the tests) |
The data-quarto-external attribute ended up in the selectors axe builds, which changed finding signatures and broke matching against an axe baseline.
|
The smoke test Removing the marker fixes the test, and almost every decoration step is safe to repeat. Options I looked at:
For now, the PR follows the first option. |
# Conflicts: # src/resources/editor/tools/yaml/all-schema-definitions.json
There was a problem hiding this comment.
Thanks for digging into this. I agree with your analysis, the custom icon branch is the only step not safe to repeat. I would go with the sibling check (so your option 2): it is a small guard, nothing axe would see, and we don't leave a known duplicate behind. A WeakSet would skip the whole link on later runs, so a future step needing a re-run would be skipped silently.
The string form comes from the original implementation (fc1c503) and was never documented, so I don't think it is meant to be supported anymore. Removing it can be a follow-up.
The listener stacking on hashchange is pre-existing; with the sibling check it won't cause duplicates here, so we can track it separately.
I'll go through the rework itself next, unless you want to do that.
You mean, the follow-up on |
|
@mcanouil yes I meant this. |
|
I'll let you do it, but I can open an issue to ensure it's not getting lost in this PR comments. |
|
ok great. I'll finish this work and merge. Thank you again ! |
Listing updates rerun the external-link decoration on the same item elements, since list.js reuses them, so a string link-external-icon inserted another icon on every sort, filter or page change.
The span appended inside the link became part of its accessible name. Image links named only by a title were renamed "(opens in a new tab)", an empty aria-label gave " (opens in a new tab)", and an image link with no alt text got a name, hiding the link-name failure from axe. Links named by aria-labelledby got no announcement at all. Links now reference one hidden label element through aria-describedby, appended to any ids already there, so the name stays untouched in every case. A title on a text link no longer reaches the accessible description, since aria-describedby takes precedence; the tooltip stays.
Cover a text link, image links with alt text, with only a title, and with no alt text, an empty aria-label, and aria-labelledby with an existing aria-describedby. Each keeps its name and gets the announcement as its description. Also check that a string link-external-icon adds one icon per link across repeated listing page changes. The listing axe scan now skips the empty links left behind when a description containing a link splits the item's wrapping anchor. They fail link-name and already render this way in 1.10; the new-tab span used to give them a name.
|
I pushed the remaining changes, with the sibling check for the custom icon (d4d2f0f) and one change on the new-tab announcement (939951e). As the span was appended inside the link, it became part of the link accessible name, and for links without text the whole name. So I moved the announcement to
The last one has no name on main, and axe reports it again with This also showed that the listing axe test was passing thanks to the span. When a description contains a link, the item wrapper Tests cover all the cases above and the custom icon over page changes (0eeb5df), and I updated the changelog for the grid icon (b27b00f). |
…15213) Default and grid listings wrap the item description in a link to the item, so a link inside the description produced nested anchors. The HTML parser split them into empty, unnamed links, which axe reports as link-name. The delink filter flattens such links to text, but it only matches Pandoc Divs, and the item templates stopped emitting one in 1.7 when they moved to raw HTML (see #11758), so the filter no longer ran. The fenced div is restored so the filter applies again, and only the item link stays raw HTML. Description links are text again, as in 1.6, so the whole description links to the item. Making them clickable is discussed in #10814 and is not part of this change. - Restore the `delink` container in the default and grid item templates. - Remove the empty-link exclusion from #9469's listing axe test. - Remove the description link icon test from #9469 and cover both listings with a smoke-all project. - Move the custom icon pagination test from the default listing to a table listing, whose title link keeps the external icon. Fixes #15211
Register each listing's update handler once
Pages with several listings attached another list.on("updated")
handler to every listing on each URL hash change, so a single update
reran the same work several times. Each listing's inline script called
the handler on DOMContentLoaded and again from its own hashchange
listener. Wiring now runs once per listing id, and one hashchange
listener in quarto-listing.js only applies the hash.
- Wire each listing once, keyed by its id
- Move hash handling into applyListingHash()
- Replace the per-listing hashchange listeners with a single one
- Pin hash, pagination, and update-handler behavior with tests
The hashchange listener dates from #1139, and back/forward between
categories still depends on it. Found while reviewing #9469.
Fixes #15206
Description
With
link-external-newwindow: true, external listing items now open in a new tab.Fixes #9468.
The external-link script skipped listing items because they carried
no-external, which turns off both the new tab and the icon.A new class,
no-external-icon, turns off only the icon, and default and grid items use it.no-externalkeeps its meaning, and an external link inside an item description keeps its icon.The listing reruns the script when pagination, filtering or sorting shows new items.
Quarto never tells screen reader users that a link opens a new tab, which fails WCAG technique G201 (success criterion 3.2.5), and this change opens more links that way.
Each link the script opens in a new tab now ends with a visually hidden "(opens in a new tab)", from a new language key
link-external-newwindow-label(English only for now).An external item without an image also left an empty thumbnail link in the tab order, which #15099 fixed only for items with a file; it is now hidden the same way.
Playwright tests cover default, grid and table listings, pagination,
minimal: true, axe-core, and a page without the option.Table rows still open in the same tab when clicked outside the link, which I'll address separately.
Checklist
I have (if applicable):
AI-assisted PR
Note: autonomous AI agents submitting PRs without human oversight are not permitted — see the Code of Conduct.