Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not open external links in new tabs #496

Closed
1 task
obulat opened this issue Sep 22, 2022 · 8 comments · Fixed by #4446
Closed
1 task

Do not open external links in new tabs #496

obulat opened this issue Sep 22, 2022 · 8 comments · Fixed by #4446
Assignees
@obulat
Labels
♿️ aspect: a11y Concerns related to the project's accessibility ✨ goal: improvement Improvement to an existing user-facing feature help wanted Open to participation from the community 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: frontend Related to the Nuxt frontend

Comments

@obulat
Copy link
Contributor

obulat commented Sep 22, 2022 •
edited by sarayourfriend
Loading

This issue was modified as a result of the discussion. Refer to the edit history for previous versions and context on the discussion up until #496 (comment).

Problem

Get this audio link opens in a new tab, but does not disclose this to screen readers.

Description

As discussed, we will no longer open links in new tabs. Keep the external site icon, but remove the new tab behaviour from an links that currently do this. Review the behaviour of VLink and any usage of target="_blank" in the frontend.

Update or add e2e tests that evaluate this behaviour to ensure new tabs are not opened from external links in practice. It might be a good opportunity to add a functionality test for VLink in Storybook, if that appropriately reflects an aspect of how this is solved in the code.

Additional context

This was originally reported for the Get audio external link, by Joe Dolson (https://make.wordpress.org/openverse/2022/09/07/frontend-release-v3-4-8-and-a-call-for-a11y-testing/#comment-95)

Implementation

  • 🙋 I would be interested in implementing this feature.
@obulat obulat added 🟨 priority: medium Not blocking but should be addressed soon ✨ goal: improvement Improvement to an existing user-facing feature ♿️ aspect: a11y Concerns related to the project's accessibility labels Sep 22, 2022
@obulat obulat added good first issue New-contributor friendly help wanted Open to participation from the community labels Nov 12, 2022
@AlexVCS AlexVCS mentioned this issue Nov 18, 2022
Closed
7 tasks
@obulat obulat transferred this issue from WordPress/openverse-frontend Feb 22, 2023
@obulat obulat added the 🧱 stack: frontend Related to the Nuxt frontend label Feb 22, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Openverse Backlog Feb 23, 2023
@obulat obulat removed their assignment Feb 24, 2023
dhruvkb pushed a commit that referenced this issue Apr 14, 2023
@stacimc
Merge popularity calculations and data refresh into a single DAG (#496)
3ef7138 
* First pass at combining popularity calc, matview refresh, and data refresh DAGs

* Remove the old popularity refresh DAGs

* Use the data_refresh timeout from the configuration in the DAG

* Only force a single pool slot for the wait_for_data_refresh task

* Refactor TaskGroups out into factories and update docs

* Update docs, fix conditional

* Add config option to force metrics refresh when not the first of the month

* Fix log message

* Remove deleted DAG tests

* Use timeout from data refresh config

* Document the force_refresh_metrics option

* Add link to this issue in the docs for reference

* Safely get options from config, fix timeout

* Consider dagrun first of the month if previous runs failed

* Don't refresh metrics is option is explicitly configured to False

If `force_refresh_metrics` is explictly configured to False (rather
than omitted), then do not refresh the popularity metrics even if
this is the first successful run of the month.

This option could be helpful if, for example, the first dagrun of the
month succeeds during the popularity steps but fails during the data
refresh. When we manually re-run the DAG, we can save time by skipping
this step.

* Use current dagrun start_date instead of datetime.now()

* Test the month_check operator

* Fix dates in tests, only clean up DagRuns associated to the test Dag

* Handle case where there isn't a successful previous run

* Make task names more explicit

* Clean up unused param

* Remove unused param in tests as well

* Fix type, pull out constant

* Clean up queries and operators

* Add docs to tasks

* Add type for media_type

* Inline docs and CamelCase type

* Remove MediaType type for now

* Clarify flow of data through the data refresh in comments

* Fix type string

* Update timeouts for popularity refresh tasks
@obulat obulat mentioned this issue May 17, 2023
Closed
@dhruvkb
Copy link
Member

dhruvkb commented Apr 1, 2024

Can we reconsider if we should be opening new tabs at all instead?

The W3C guidelines recommend not opening links in new tabs unless it is absolutely necessary (with some examples about what counts as necessary). They also recommend clearly stating if a link opens a new tab, which I feel is not adequately conveyed by just the external site icon.

Our external links could open in the same tab and let users who want new tabs do so via the context menu or holding down Ctrl or Cmd.

@kvdotkirti
Copy link

Is this issue still open? I would like to attempt it

@AetherUnbound
Copy link
Collaborator

It is @kvdotkirti! Thank you for your interest in contributing to Openverse! I've assigned this issue to you. If you have any questions, you may leave them here.

Please check out our welcome and quickstart documentation pages for getting started with setting up your local environment.

@openverse-bot openverse-bot moved this from 📋 Backlog to 📅 To Do in Openverse Backlog Apr 29, 2024
@obulat obulat removed good first issue New-contributor friendly help wanted Open to participation from the community labels Jun 4, 2024
@obulat
Copy link
Contributor Author

obulat commented Jun 4, 2024

Can we reconsider if we should be opening new tabs at all instead?

Thank you for pointing this out. I think we should stop opening external links in new tabs.
Should we still have an external link icon next to links that lead to external websites? That is the behavior on WordPress.org: the external links have an icon, but open in the same tab.
@zackkrida, @fcoveram, @sarayourfriend, what do you think?

I removed the "help wanted" labels until this discussion is finalized.

@obulat obulat moved this from 📅 To Do to 📋 Backlog in Openverse Backlog Jun 4, 2024
@fcoveram
Copy link
Contributor

fcoveram commented Jun 4, 2024

Agree with @dhruvkb. Let's not open new tabs

@obulat
Copy link
Contributor Author

obulat commented Jun 4, 2024

Agree with @dhruvkb. Let's not open new tabs

Do you think we should still have the "External link" icon, @fcoveram?

@fcoveram
Copy link
Contributor

fcoveram commented Jun 4, 2024

Oh yes. Sorry for not being clear: Keep the external action (leaving the site from a link with the external icon) but in the same tab, not in a new one.

@sarayourfriend sarayourfriend changed the title The external link should disclose to the screenreaders that it opens in a new tab Do not open external links in new tabs Jun 5, 2024
@sarayourfriend
Copy link
Collaborator

I've updated the issue to reflect the decision. I won't add good first issue because it will require a bit of searching around the app to make sure it is handled in all cases as well as probably updating or add e2e tests to reflect the change.

@sarayourfriend sarayourfriend added the help wanted Open to participation from the community label Jun 5, 2024
@obulat obulat mentioned this issue Jun 6, 2024
Merged
8 tasks
@openverse-bot openverse-bot moved this from 📋 Backlog to 🏗 In Progress in Openverse Backlog Jun 6, 2024
@openverse-bot openverse-bot moved this from 🏗 In Progress to ✅ Done in Openverse Backlog Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@obulat obulat

Labels
♿️ aspect: a11y Concerns related to the project's accessibility ✨ goal: improvement Improvement to an existing user-facing feature help wanted Open to participation from the community 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: frontend Related to the Nuxt frontend
Projects
Openverse Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stop opening links in a new tab WordPress/openverse
Screenreaders new tab disclosure
6 participants
@fcoveram @AetherUnbound @obulat @dhruvkb @sarayourfriend @kvdotkirti