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.

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

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

Closed
timfish opened this issue Jan 12, 2018 · 27 comments
Closed

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

timfish opened this issue Jan 12, 2018 · 27 comments
Labels

Comments

@timfish
Copy link
Contributor

timfish commented Jan 12, 2018

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
Copy link
Contributor Author

timfish commented Jan 12, 2018

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
Copy link
Member

zewa666 commented Jan 12, 2018

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

@timfish
Copy link
Contributor Author

timfish commented Jan 13, 2018

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
Copy link
Contributor

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

@RomkeVdMeulen
Copy link
Contributor

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
Copy link
Contributor Author

timfish commented Jan 15, 2018

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
Copy link
Contributor

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
Copy link
Contributor Author

timfish commented Jan 22, 2018

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:

  • No longer required to set z-index, the spec includes a top layer where dialogs are rendered
  • Overlay is no longer required. There is a dialog::backdrop pseudo element which you can style
    • It is dialog + .backdrop in the polyfil
  • showDialog() does properly fix focus considerations #1 by trapping focus while still allowing tabbing to browser chrome
    • The polyfil reports "The browser's chrome may not always be accessible via the tab key" but it appeared to work in Safari
  • The autofocus attribute just works on input elements
  • Vertical centring appears automatic but doesn't update on resize
    • This is similar to the existing implementation in aurelia-dialog but I never understood why it didn't just use CSS?

But would mean some breaking changes:

  • Escape key closes the dialog and there doesn't appear to be a way to stop it
    • Actually you can by listening for the cancel event and preventDefault()
  • The position callback would no longer be passed an overlay

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
Copy link
Contributor

RomkeVdMeulen commented Apr 2, 2019

@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 [email protected] it's not there either. Was this feature not rolled out?

@EisenbergEffect
Copy link
Contributor

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

@StrahilKazlachev
Copy link
Contributor

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
Copy link
Contributor

@StrahilKazlachev Are we ready for a release then?

@StrahilKazlachev
Copy link
Contributor

@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
Copy link
Contributor

@StrahilKazlachev A couple of questions for you:

  • What is the alternative to using import?
  • Approximately, what is the level of effort to get this to a releasable state?

@RomkeVdMeulen
Copy link
Contributor

@StrahilKazlachev Did you loose track of this conversation?

@StrahilKazlachev
Copy link
Contributor

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

  • master has at least 1 breaking change(can't remember else right now) - the transition to using the Animator, it concerns the Renderer
  • all build changes bigopon has done to v2 must be applied on master
  • there was 1 PR from Vildan that I'm not sure if and in what shape has got in master, but it is out in v2, so it should just continue work
  • some doc PRs must be reviewed if still applicable - they got in v1/v2, not sure where
  • what definitely must be cleared is will resources by dynamically imported(v2/import()) or statically(as were in v1). Again, v2 was made just so we can release a v1 which uses import()(no other changes) and see what feedback we get. I can't remember any feedback with the exception of people needing to reconfigure their loaders/bundlers to use import(), which was expected.
  • it would be great if other things also get some attention and be decided on:
  • how should this be released, breaking changes in v2-rc or v3

@EisenbergEffect
Copy link
Contributor

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

@bigopon
Copy link
Member

bigopon commented Apr 15, 2019

Ill have a look with @StrahilKazlachev

@bigopon
Copy link
Member

bigopon commented Apr 17, 2019

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
Copy link
Contributor

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
Copy link
Contributor

...so? Any progress?

@bigopon
Copy link
Member

bigopon commented May 9, 2019

@RomkeVdMeulen ill start this later today or tomorrow

@EisenbergEffect
Copy link
Contributor

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
Copy link
Contributor

No problem! Thank you all for your efforts!

@RomkeVdMeulen
Copy link
Contributor

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

@EisenbergEffect
Copy link
Contributor

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

@RomkeVdMeulen
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants