aurelia / dialog

A dialog plugin for Aurelia.
MIT License
106 stars 115 forks source link

Should be based on web standards HTML <dialog> element #335

Closed timfish closed 5 years ago

timfish commented 6 years ago

Aurelia is forward looking and supports web standards so shouldn't aurelia-dialog be built using the HTML dialog element, be based around that API and use the relevant polyfils to aid with older browsers?

Obviously the API is not as nice as the aurelia-dialog API (ie. no async) so it might be worth wrapping it but in Chrome 64 testing these examples showModal() appears to fix #1 (focus issues) and will no doubt be more accessible to screen readers than the current implementation.

timfish commented 6 years ago

The HTML 5.2 specification mentions an autofocus attribute which works along with the dialog spec to set the focus in the dialog. This would replace the aurelia-dialog attach-focus which does the same thing.

zewa666 commented 6 years ago

Great idea, now lets hope all the browsers and electron catch up soon enough

timfish commented 6 years ago

It was added to chrome 37 so we don't have to worry about electron and the polyfill seems to work well as it appears to fix #1 on Safari.

StrahilKazlachev commented 6 years ago

So lets see if this can be cleanly added in a Renderer.

RomkeVdMeulen commented 6 years ago

As for accessibility: in my experience most screenreaders are still somewhat lagging in HTML5 support, but hopefully they'll catch up over time. I don't know how well the <dialog> is / can be made backwards compatible using ARIA. I'll do some testing and come up with some recommendations.

timfish commented 6 years ago

For backwards compatibility I think we'd probably have to include:

<dialog role="dialog" aria-modal="true">
</dialog>

aria-modal was introduced in ARIA 1.1. To support ARIA 1.0 screenreaders, anything outside the dialog should have aria-hidden="true" added which should have a similar effect.

RomkeVdMeulen commented 6 years ago

Some quick testing tells me support for the <dialog> tag is surprisingly good on Windows. Chrome seems to support the tag natively: when using NVDA 2017.4 and Chrome 63, the dialog is announced. NVDA with IE 11 or Firefox doesn't have native support, but the polyfill already sets role="dialog" automatically and with that, the dialog is announced on NVDA as well.

VoiceOver on Mac didn't announce that a dialog was opened in either Chrome or Safari, even with role="dialog" aria-modal="true" set manually.

timfish commented 6 years ago

I managed to find some time this evening to hack about with a modified version of DialogRenderer which simply inserts <dialog> rather than <ux-dialog-container> calls showDialog(), close() methods and removes some redundant code.

It looks like it will simplify quite a few things:

But would mean some breaking changes:

Oh and the default CSS for <dialog> is awful. It has thick black border, solid white background and hefty padding so we'd probably override it all to hide it 😄

image

RomkeVdMeulen commented 5 years ago

@EisenbergEffect I'm confused: this feature was merged in over a year ago. The native renderer source file is even included in the 1.1.0 tag. But when I look at the 1.1.0 dist files or the 2.0.0 branch source files the native renderer isn't present, and when I npm install aurelia-dialog@1.1.0 it's not there either. Was this feature not rolled out?

EisenbergEffect commented 5 years ago

Apologies @RomkeVdMeulen .... I can't remember 😢 @StrahilKazlachev Can you comment on the status of this?

StrahilKazlachev commented 5 years ago

Not sure why it's in the change log, I've probably messed up something in the commits. It's in master, with other unreleased changes, some breaking.

EisenbergEffect commented 5 years ago

@StrahilKazlachev Are we ready for a release then?

StrahilKazlachev commented 5 years ago

@EisenbergEffect can't say that, the build setup is out of sync, there are a couple commits in v1/v2 that must get in master, the community didn't give much feedback on the adoption of import() in v2(was hoping for some to decide whether to drop it or not when publishing the other breaking changes).

EisenbergEffect commented 5 years ago

@StrahilKazlachev A couple of questions for you:

RomkeVdMeulen commented 5 years ago

@StrahilKazlachev Did you loose track of this conversation?

StrahilKazlachev commented 5 years ago

@EisenbergEffect no idea for alternative to import() itself. @RomkeVdMeulen yes, and no.

EisenbergEffect commented 5 years ago

@bigopon Do you think you could help with getting this library over the finish line?

bigopon commented 5 years ago

Ill have a look with @Strahilkazlachev

bigopon commented 5 years ago

To me, it's not obvious what needs to be done, beside adjusting the build scripts of v1. About release version, it's a tough one. I think it does not disturb existing bundlers, probably we can go for an increment on rc-x

About import + alternative renderer, it seems the only way to me. If we agree on that, will probably just need to make every work properly and released correctly.

EisenbergEffect commented 5 years ago

I think we should move forward with v2, keeping import as the mechanism for dynamic import. We can merge fixes from master into v2, and then move v2 to master. Then fix up CI with our new develop/master flow and move forward with the new release that uses Animator. Any objections?

RomkeVdMeulen commented 5 years ago

...so? Any progress?

bigopon commented 5 years ago

@RomkeVdMeulen ill start this later today or tomorrow

EisenbergEffect commented 5 years ago

Thank you @bigopon and thanks @RomkeVdMeulen for the gentle nudge. I apologize for lack of communication on this, I was traveling the other week and then managed to throw my back out over the weekend, so I'm a bit behind... @bigopon has been a champion of improving Aurelia's plugins (and core) lately, so I'm confident we'll get this squared away.

RomkeVdMeulen commented 5 years ago

No problem! Thank you all for your efforts!

RomkeVdMeulen commented 5 years ago

Looks like the changes have been merged. Can we expect 2.0.0-rc6?

EisenbergEffect commented 5 years ago

Yes. There's a collection of releases going out this week.

RomkeVdMeulen commented 5 years ago

I've been trying it out and it works like a charm!

Thanks everyone for the hard work, especially @timfish.

AFAIK this bug can be closed.