layercake

layercake copied to clipboard

Hello, I was wondering if it might be possible to add attribute-level controls on the <svg> and <canvas> elements (so Svg, ScaledSvg, Canvas, and Webgl). My most common use-case for this comes in wanting to add descriptions for svgs. In order to do this in HTML, I would write something along the lines of

<svg role="img" aria-labelledby="my-label">
    <title id="my-label">This is a label.</title>
</svg>

Currently, I should be able to add the role and aria-labelledby attributes on the Svg component by running setAttribute() on the element bindings, but that feels a little bit clunky, especially for something I use fairly regularly. I'd prefer being able to write something along the lines of

<Svg role="img" aria-labelledby="my-label">...</Svg>

or

<Svg extraAttrs={{role: "img", "aria-labelledby": "my-label"}}>...</Svg>

If you'd be interested in this, let me know; I'd be happy to submit a pull request. Thanks!

Hey @maxblee Yea good idea. I like the second approach since it covers future use cases. And probably just attrs as the prop name works. Could you start a PR? Would this not apply to the HTML layout component too? In your PR, if you could also add info in the docs with examples that would be great. It would go in this file.

Here's an example. Seems to work: https://svelte.dev/repl/6bb9adafd7f545f181335b680e3b0409?version=3.48.0

@mhkeller Sorry, I meant to respond/get to this sooner but got lost in other things. I've had a chance to take an initial stab at this. Similar to you, my first thought was to use spread props. But for some reason, they seem to create some sort of conflict with the scaleCanvas utility. I saw this when I built the static SvelteKit site and viewed the Voronoi example. When I didn't include the spread attributes {...attrs}, scaleCanvas worked properly and created a canvas with a fixed width and height, like

<canvas class="layercake-layout-canvas" style="width: 770px; height: 170px; position: absolute; inset: 10px 5px 20px 25px;" width="1540" height="340"></canvas>

But with the spread attributes, the width and height stayed at 100%, even though scaleCanvas seemed to run .

<canvas class="layercake-layout-canvas" style="width: 100%; height: 100%; position: absolute; inset: 10px 5px 20px 25px;" width="1540" height="340"></canvas>

I think the underlying issue has to do something with how Svelte tries to handle spread attributes — there's a seemingly similar open issue on Svelte — but I'm not familiar enough with the internals of Svelte to know for sure.

The workaround I wound up going with was to use the element bindings:

export let attrs = {};
$: if (element) {
    Object.entries(attrs).forEach(d => element.setAttribute(...d));
}

This doesn't break any of the existing visuals and seems to work on a minimal example. I'm adding a pull request now using that syntax. Not sure if it's the best way to go, but hopefully it's helpful.

Thanks for doing the research. Maybe set ‘attrs’ to null at first and add ‘attrs’ in the if statement so that doesn’t run if unset and also maybe gives svelte more of a hint to run that block when ‘attrs’ changes? It may also be good to add a check to see if the attributes value differs from what exists. That way if you change the prop, it won’t have the chance to trigger any unnecessary repaints. I don’t have a specific use case but i feel like those bugs are subtle and this may be a good way to avoid unnecessary code execution.

Makes sense. So something like this?:

export let attrs = null;

$: if (element && attrs) {
    Object.entries(attrs)
// hasAttribute check may be necessary because getAttribute can return an empty string in some browsers
// and some attributes (e.g. hidden) may be set to empty strings 
// https://developer.mozilla.org/en-US/docs/Web/API/Element/getAttribute#non-existing_attributes
        .filter(d => !element.hasAttribute(d[0]) || element.getAttribute(d[0]) !== d[1])
        .forEach(d => element.setAttribute(...d))
}

I would avoid the extra filter loop and include an if statement. I would also use a three-part for loop instead of .forEach since I believe the accepted wisdom is those are faster than .forEach. It probably won't make a big different for this use case but I feel like a library should try to be as considerate as possible

That sounds good. I've added a pull request with these changes. Let me know if you need any changes.

I might have overlooked the reason, but why not pass $$restProps to the underlying component instead of the attrs prop container?

@techniq would that fix this issue?

@mhkeller It does not.

I have not ran into this issue myself (surprisingly) and have used {...$$restProps} quite a lot with LayerChart and svelte-ux.

The example REPL uses $$restProps (not just $$props) so it would definitely been an issue, although I see it's just any spreading so this fails...

<img src="//unsplash.it/600/400" width="100%" alt="Alt text" {...{}} >

Sorry for the noise and I see why attrs is used. Thanks for the clarity (and I've subscribed to that issue).

Hi! I'm wondering, what is the status on this issue?

I am also running up against a case where I want to add aria-label/ role/tabindex attributes to my SVG, and also a title (this goes inside the SVG) for screen reader accessibility. (Would you recommend creating a fork of the repo to do this in the meantime?)

I think you don't have to go full-fork actually. You should be able to grab Svg.svelte and copy that as a local component inside your project. And instead of doing...

import { Svg } from 'layercake';

...you can simply import it like a normal component

import Svg from './Svg.svelte';

In terms of supporting this inside of the library, we have this PR but there was concern that allowing arbitrary props would break some of Svelte's optimizations.

This use case is really helpful to know, though, and I think doing a PR for the most common attributes would be a good replacement.

You're using these?

1. aria-label
2. role
3. tabindex
4. title is this also on the <svg> element or on the <g>?

@maxblee what do you think about scaling down that PR to just the common accessibility labels instead of allowing arbitrary ones?

(Perfect! That's what I ended up doing, but I wasn't sure if that was the best way.)

Yes, exactly. And title on the SVG element too, to describe the overall chart.

I see from the original comment there is aria-labelledby and it has <title> as a separate element. Is there any difference to doing it that way versus a prop like <svg title="The chart shows etc.">

I'm not sure it's possible to have title as a prop. I spoke too soon in my last message; my use case would be the same as the original comment (we can replace aria-label with aria-labelledby and a title).

My last message was a little unclear, though-- I meant <title> should be a direct child element of <svg> (and the docs here say it should be the first child).

Got it. Okay that should be easy, we'll do it as a separate slot like defs