components

components copied to clipboard

Passing in null to the FocusMonitor.focusVia method is currently supported, but it doesn't make sense. These changes make it impossible via typings.

It seems really strange to me to call a method that sounds like its supposed to focus the element and instead it blurs it. Why not just default the origin to 'program' if null is passed? This will yield the same end result that people are seeing today, and in my mind it makes more sense (since I am in fact focusing it programmatically, 'program' makes sense if I fail to specify an explicit origin)

I agree that it seems weird, but I think that the ideal solution would be not to allow null to begin with. I think that defaulting it to program is weird as well, because I see focusVia as "doing the opposite" of monitor, but just in this one case (null) it doesn't.

I see it more as a version of the native focus method that allows the user to override the origin. I think just switching the implementation to the behavior you're suggesting is super confusing. If we want to switch it we should come up with a better name (e.g. maybe setFocusState), and deprecate focusVia

It might be easier to talk through it and this change isn't particularly urgent. I'll add it to agenda for tomorrow's meeting.

I agree with what Miles said. I also consider the focusVia method as a simple replacement to HTMLElement.focus with the ability to overwrite the focus origin. My thinking is that null should just fall back to the default (which is program at the moment). I agree that null probably shouldn't have been added in the past, but changing it's meaning rather causes confusion/ a breaking change IMO.

What's the use-case of providing a method for blur in the FocusMonitor?

I'm not arguing that it's a valid usage, but based on the way we've typed it looks like it's valid so we might as well treat it that way.

I see what you are trying to achieve. You are saying that it looks like it's a valid scenario. That seems to depend on how people think of it though? or do you think it's made clear somewhere?

For me personally, since the method is called focus, I'd never expect it to be able to blur an element. Also, null as as part of the FocusOrigin type does not necessarily sound like blur. Agreed that we can just chat about it in the meeting.

@mmalerba @devversion I've reworked it based on our discussion.

I've addressed the feedback @mmalerba. I also had to change the signature for a handful of components where we were accepting a FocusOrigin and passing it along to focusVia directly.

poke @mmalerba

Closing this given how out of date it is and that it'll be a breaking change.

This issue has been automatically locked due to inactivity. Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}