fix(embedded-sdk): add accessible title to iframe #27017

bhaugeea · 2024-02-05T18:54:53Z

SUMMARY

Screen reader users may rely on the (missing) title attribute to know that the iframe is where the dashboard content is. This PR adds a title to the iframe with the generic value "Embedded Dashboard".

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue: Fixes Embedded dashboard iframe missing title (accessibility) #27016
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

rusackas · 2024-02-05T18:57:22Z

superset-embedded-sdk/src/index.ts

@@ -154,6 +154,7 @@ export async function embedDashboard({
      });

      iframe.src = `${supersetDomain}/embedded/${id}${dashboardConfig}${filterConfigUrlParams}`;
+      iframe.title = 'Embedded Dashboard';


This works in a pinch, but does the dashboard config (or something else) happen to provide the actual title of the dashboard? That would be the ideal thing to put here.

No not that I'm aware of, and the SDK doesn't fetch it either. It looks like the SDK doesn't load anything from Superset API at all — just gets a token using the opaque function it's given, and passes it to the iframe content.

It seems the sdk should expose the option to set this as maybe an iframeTitle property of the dashboardConfig object (?), and fall back on some default like this one "Embedded Dashboard". Note that for extra points you could i18n that string as well.

Moved it to the config. Didn't provide a fallback for now because I'd probably butcher the i18n. Would a fallback be required for this, or okay without?

Ok without AFAIC, the i18n is easy though, just FYI ->

import { t } from '@superset-ui/core'; const translatableString = t("Embedded Dashboard");

What happens then is the i18n framework scans through the code base and inventorize all those strings on the backend and frontend, where other people can then translate then in various languages.

github-actions

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

rusackas · 2024-02-06T20:46:34Z

Thanks for following up... running CI 🤞

bhaugeea · 2024-02-06T23:52:33Z

I believe the whole ui/core package would need to be added as a dependency here. I haven't tried to open the can of worms of actually running this repo so not confident in being able to get that working.

mistercrunch · 2024-02-07T22:30:36Z

superset-embedded-sdk/src/index.ts

@@ -77,6 +80,7 @@ export async function embedDashboard({
  id,
  supersetDomain,
  mountPoint,
+  iframeTitle = t("Embedded Dashboard"),


Hey this look like a public interface, so this new [optional] param should be the last one in the function call for backwards compatibility, otherwise we'l have off-by-one param matching on the function call!

mistercrunch · 2024-02-07T22:32:51Z

superset-embedded-sdk/src/index.ts

@@ -154,6 +158,9 @@ export async function embedDashboard({
      });

      iframe.src = `${supersetDomain}/embedded/${id}${dashboardConfig}${filterConfigUrlParams}`;
+      if(iframeTitle) {


I think given the default value set in the function header, this will always have a value, so no if required. Actually I think if someone actually wanted an empty string and pass iframeTitle = '' or iframeTitle = null in the function call, it would prevent them from overriding the Embedded Dashboard default :/

mistercrunch · 2024-02-07T22:35:07Z

I believe the whole ui/core package would need to be added as a dependency here. I haven't tried to open the can of worms of actually running this repo so not confident in being able to get that working.

Oh you're right about that! Let's actually not do this then. Not worth adding the dependency for the sake of a i18n that one string!

rusackas · 2024-06-03T16:15:58Z

@mistercrunch @bhaugeea Curious if you see a way forward with this, so we can close out the PR and Issue. If there's no intent to rebase this in the near future and get it across the finish line, I'll convert this to a Draft, and the issue may closed as stale in time.

Addressing my comments

mistercrunch · 2024-06-03T20:47:17Z

@rusackas I just made the changes I suggested and removed superset-ui/core dependency that was reference, but wasn't added as a dependancy of the sdk since it could have implications.

nikola-arcadis · 2024-06-21T06:01:38Z

I don't see this change is published on npmjs registry with version 0.1.0-alpha.11. How can we get the new package with this change?

rusackas · 2024-06-26T18:30:01Z

Sorry... we need to update a bunch of npm packages. We'll try to get that effort underway soon.

mistercrunch · 2024-06-27T00:02:38Z

@rusackas is the process to do that documented someplace? Oh nevermind found an example PR -> https://github.com/apache/superset/pull/13593/files

rusackas · 2024-06-28T18:25:57Z

That brings in the newly published ones, but here are the instructions (the most current I'm aware of) to actually publish the modules to NPM:

https://github.com/apache/superset/pull/23730/files

This should be made part of the release process, but for now, I have it on my task list to go back and try it on some of the voted-in release branches/SHAs.

Co-authored-by: Maxime Beauchemin <[email protected]>

Add title to embedded dashboard iframe

765e809

pull-request-size bot added the size/XS label Feb 5, 2024

rusackas reviewed Feb 5, 2024

View reviewed changes

github-actions bot reviewed Feb 5, 2024

View reviewed changes

bhaugeea added 2 commits February 6, 2024 09:25

move iframe title to dashboard config

7a41939

Provide default iframe title with i18n

fed7303

mistercrunch requested changes Feb 7, 2024

View reviewed changes

Update index.ts

dee2eee

Addressing my comments

github-actions bot added the embedded label Jun 3, 2024

Merge branch 'master' into patch-1

7de23d2

mistercrunch approved these changes Jun 3, 2024

View reviewed changes

Update index.ts

fdefa2b

rusackas merged commit 1a52c6a into apache:master Jun 4, 2024
31 checks passed

eschutho pushed a commit that referenced this pull request Jul 24, 2024

fix(embedded-sdk): add accessible title to iframe (#27017)

8e654a2

Co-authored-by: Maxime Beauchemin <[email protected]>

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix(embedded-sdk): add accessible title to iframe (apache#27017)

b693878

Co-authored-by: Maxime Beauchemin <[email protected]>

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.1.0 labels Nov 27, 2024

Vitor-Avila mentioned this pull request Mar 19, 2025

chore: Add features from OSS preset-io/frontend-sdk#30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(embedded-sdk): add accessible title to iframe #27017

fix(embedded-sdk): add accessible title to iframe #27017

bhaugeea commented Feb 5, 2024

rusackas Feb 5, 2024

bhaugeea Feb 5, 2024

mistercrunch Feb 6, 2024 •

edited

Loading

bhaugeea Feb 6, 2024

mistercrunch Feb 6, 2024 •

edited

Loading

github-actions bot left a comment

rusackas commented Feb 6, 2024

bhaugeea commented Feb 6, 2024

mistercrunch Feb 7, 2024

mistercrunch Feb 7, 2024

mistercrunch commented Feb 7, 2024 •

edited

Loading

rusackas commented Jun 3, 2024

mistercrunch commented Jun 3, 2024

nikola-arcadis commented Jun 21, 2024

rusackas commented Jun 26, 2024

mistercrunch commented Jun 27, 2024

rusackas commented Jun 28, 2024 •

edited

Loading

fix(embedded-sdk): add accessible title to iframe #27017

fix(embedded-sdk): add accessible title to iframe #27017

Conversation

bhaugeea commented Feb 5, 2024

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

rusackas Feb 5, 2024

Choose a reason for hiding this comment

bhaugeea Feb 5, 2024

Choose a reason for hiding this comment

mistercrunch Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

bhaugeea Feb 6, 2024

Choose a reason for hiding this comment

mistercrunch Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

rusackas commented Feb 6, 2024

bhaugeea commented Feb 6, 2024

mistercrunch Feb 7, 2024

Choose a reason for hiding this comment

mistercrunch Feb 7, 2024

Choose a reason for hiding this comment

mistercrunch commented Feb 7, 2024 • edited Loading

rusackas commented Jun 3, 2024

mistercrunch commented Jun 3, 2024

nikola-arcadis commented Jun 21, 2024

rusackas commented Jun 26, 2024

mistercrunch commented Jun 27, 2024

rusackas commented Jun 28, 2024 • edited Loading

mistercrunch Feb 6, 2024 •

edited

Loading

mistercrunch Feb 6, 2024 •

edited

Loading

mistercrunch commented Feb 7, 2024 •

edited

Loading

rusackas commented Jun 28, 2024 •

edited

Loading