I can't see anything wrong with the JS - looks great. But what does it bring to the table? What problem does this solve?

There's something about loading JS to inject content that just leaves me thinking of IE6 and Javascript based rounded corner solutions and thus leaves me a bit uncomfortable. The Spidey sense is going off, but I can't quite say why. Maybe it's the idea of loading this on every page unnecessarily? Or maybe this is a solution looking for a problem, or not the right approach for the problem. Honestly I don't have enough background on what the desired future state is to fully back up the Spidey sense, but it's usually worth listening to.

Hi @anthonylindsay

The problem is described here: #563

The reason I am proposing we do this with JS is because the alerts are wysiwyg styles so we don't have a template for them in Twig. If we did, I'd put this into a twig file instead. Since these alerts can appear anywhere a wysiwyg is used (body field, some teasers, quote blocks, media with text, etc) we need to do this dynamically via JS.

If JS fails to load, a screenreader user gets the exact same experience we currently have. If JS loads, they will get the announcement.

Thanks to Big Blue Door for sponsoring my time to work on this.

does a ckeditor plugin not sort of fit better, if we're talking editor styles?
Like, do it at source, and save it once, rather than at runtime, each time?

That sounds like an interesting approach Anthony. If you have a PR we can consider it.

In the meantime, perhaps we can merge this, and then remove it if a better solution comes along.

Well it's a very different approach, isn't it?

If you do it at runtime, you're not creating anything: it's cosmetic. If you then remove it if a better solution comes along, you lose that cosmetic change.

If you do it as an editor plugin, you're effectively producing HTML, which gets saved.

Thus, if you do it at runtime and replace it later, you've a headache down the line and a tricky, destructive script to run to modify HTML in content so that nobody loses the thing that they had.

However, if you do it as a plugin, the end result is HTML. So even if you later remove the plugin, the HTML is persistent.
Does that make sense?

I'd like to say, yes that makes sense, but my brain's a little hurting.

Are you saying if you create a ckeditor plugin, we'd need to get editors to resave every node that has an alert on it to get the new markup?

my brain's a little hurting.

lol!

Yeah. In the scenario where the runtime solution is removed, then the editors would be starting from scratch again. Thus, they would have to edit a node, highlight the thing they want, push the new button, save.

I don't think that is feasible for our set up given the amount of live sites we are dealing that we know about and the ones we don't know about.

Using JS means it gets a nice quick fix now and will work for all instances without editor action.

Going back to first principles, and issue 563, we've got some cosmetic styles for some content, and the content should tell someone who cannot see the style what the content is about.

You might have a person who cannot see and uses a screenreader. A screenreader can see a visually hidden thing, so that can make sense.

However, a colourblind person is left unable to see the visually hidden announcement or the colour. They get nothing.

Really this is a content problem, and should have a content solution. Even a ckeditor plugin only simplifies the job the editor has. The editor still has to make it accessible. Even with your Javascript solution the problem persists for the colourblind usecase and the editor still has to make changes.

If your Javascript solution is to actually solve it, you'd need to add in something that can be seen, as well as or instead of the visually hidden thing.

We have a separate issue for the "add in something that can be seen" request - #562. This is about adding context for screenreaders.

Also, with this JS fix, it doesn't preclude content editors from adding more context. This is supplementary.

I don't want to get to a position with this issue where we have the opportunity to make things better than they currently are by merging this (and it's easy to remove if we come up with a better solution) but we miss out on that opportunity while we search for the perfect solution.

Thanks to Big Blue Door for sponsoring my time to work on this.

Fair enough. When the 'add in something that can be seen' bit is done it might make this redundant anyway.

@anthonylindsay are you okay if we mark this as "approved"? it would be good to get it merged at tomorrow's Merge Tuesday if it's approved before then.

yeah. Let's keep things moving. Thanks for engaging on it: was a worthwhile discussion methinks.

More generally I'd like to find out how these are being used in the wild. Whether the green one does get used for "Success" messages. Once we know that we can label them with more confidence.

Great idea though.

Let's chat about this a bit more at Merge Tuesday or at the Tech Group Drop-in.

It seems like an issue worth getting solved and merged.

@msayoung We should chat about this one. It would be good to get merged or closed if we are not going to merge it.

More generally I'd like to find out how these are being used in the wild.

I've no idea how we'll get data on that given the amount of live sites we have. I'm going to guess lots of editors use them as we expect and lots of editors use them because they happen to like the colours.

I wonder is it worth forcing our fix on people so those that are using them correctly will not be using them even more correctly and those that are using them just because they like background colours will still be using them not as designed.

Perhaps just "Alert: Warning" , or "Alert: Success"

I've made this change for us in the PR.

Thanks to Big Blue Door for sponsoring my time to work on this.