search

aurelia / dialog

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

feat(renderers): add restoreFocus setting #378

Closed RomkeVdMeulen closed 4 years ago

RomkeVdMeulen commented 5 years ago

This controls where focus should be restored when a dialog closes. By default focus is restored to the last element that was active when the dialog opened.

Unfortunately the unit tests seem to broken at the moment (timeouts). I've added a test based on existing ones but haven't seen it run succesfully.

Closes #375.

RomkeVdMeulen commented 5 years ago

@StrahilKazlachev what do you think of this?

bigopon commented 5 years ago

@RomkeVdMeulen this looks like a breaking change to me, maybe add it with a flag?

RomkeVdMeulen commented 5 years ago

@bigopon @StrahilKazlachev As far as I understand it, there shouldn't be a compatibility problem. If restoreFocus is overriden to a falsy value, including undefined, the behaviour will simply be skipped (e.g. src/renderers/native-dialog-renderer.ts:98). If people haven't overriden their dialog config, then by default the focus behaviour will be active. I don't believe that to be a compatibility problem: new releases can introduce additional behaviour. I think this behaviour should be active by default: it improves accessibility and usability for consumers and gives no downsides that I can think of.

If I'm missing something though, please let me know. I don't mind marking this for a major release if necessary.

StrahilKazlachev commented 5 years ago

I can't speak for @bigopon, but my concern for turning on by default this new feature is what if someone has already implemented his own restore logic. Wouldn't this mess up his project? Also given the default you are suggesting, it shouldn't be such a burden to add it when configuring the plugin.

RomkeVdMeulen commented 5 years ago

But the new user must also be considered. I consider this a disirable core behaviour. It would be strange if every new aurelia-dialog user from now on would need to explicitly opt-in to this behaviour.

I can't say with certainty how many people will already have implemented their own focus behaviour. I don't imagine there will be many of those though. And even then: in the worst case their implementation will race the default one, and the outcome may vary. But it won't result in error conditions.

Given all that, I believe that a note with the release notes for this change should suffice. It could tell people who prefer their own focus implementation (if there are any) that they need to disable the new default after updating.

RomkeVdMeulen commented 5 years ago

@EisenbergEffect What is your take on this? Does this constitute a breaking change?

EisenbergEffect commented 5 years ago

I'd be willing to merge this in as a minor release. It is technically breaking, but I'd guess that most people using this module aren't handling that scenario at all. Just to be sure, if it does break someone, they can easily set a custom config to turn it off, yes?

RomkeVdMeulen commented 4 years ago

Yes. E.g.:

aurelia.use.plugin("aurelia-dialog", (dialog: DialogConfiguration) => {
  // [...]
  dialog.settings.restoreFocus = undefined;
});
RomkeVdMeulen commented 4 years ago

@EisenbergEffect I had expected this to be released with the september releases. Did it fall off the radar?

EisenbergEffect commented 4 years ago

We had an issue with the dialog branching. We're in the process of fixing that. I just merged a fix PR and will be testing it out today. If it's all working, I'll get the release out today or tomorrow.

EisenbergEffect commented 4 years ago

@RomkeVdMeulen I've made the release. However, after reviewing this PR again and the latest source code, while the restore focus implementation is present and tests pass, for some reason the code that adds the interface member for restoreFocus is missing. I'm not sure how that happened. Can you take a look at the latest release and if you see that as an issue, would you mind submitting a PR to add the desired signature? I can turn around a release with that fix within 24 hours without an issue now that we've addressed the branch problem.

RomkeVdMeulen commented 4 years ago

Sorry, I missed this. I'll take a look at it now.

RomkeVdMeulen commented 4 years ago

Ah, I see @StrahilKazlachev already fixed this. Thank you!

RomkeVdMeulen commented 4 years ago

@EisenbergEffect AFAIK the PR has been properly merged. Could you roll out a new release?

EisenbergEffect commented 4 years ago

@RomkeVdMeulen Are you not seeing it in 2.0.0-rc.8 ?

RomkeVdMeulen commented 4 years ago

My apologies, I misunderstood. It is present in rc.8. Thanks!