feat(cdk/interactivity-checker) add config parameter to isFocusable

[vanessanschmitt]

Adds config object to isFocusable so that it can optionally return true
even when an element is invisible. Example use case: call isFocusable
with ignoreVisibility=true to find a focusable element within a
collapsed element. If isFocusable returns true, expand the collapsed
element and focus the focusable element.

Fixes #6468.

[devversion]

LGTM. When you get a chance, could you please add the concrete use-case example to the commit message? Thanks!

[jelbourn]

I generally try to avoid public APIs that use a boolean flag argument:
https://martinfowler.com/bliki/FlagArgument.html

I would prefer to make isPotentiallyFocusable with a more descriptive name, though I'm having a hard time coming up with one. Maybe isPotentiallyFocusable really is the best thing with just good JsDoc.

cc @devversion @crisbeto @andrewseguin @mmalerba ideas?

If we don't have any alternates, I'm good with just making isPotentiallyFocusable public.

[devversion]

I'd need to think more about other good names, but if we can't come up with one: I'd personally be hesitant in naming this isPotentiallyFocusable as it seems to signify an even worse API design than using a flag argument here. The use of potentially is extremely ambiguous and it's never clear what it is actually referring to, unless someone dives into the description of the function or adds a good comment at call position.

Personally, I'd prefer option parameters here, as those avoid the ambiguity of flag arguments (which drive Martin Fowler's paper), and also avoid the ambiguity of potentially. e.g.

checker.isFocusable(element, {ignoreDisabled: true});

To be clear: If we could find a good name, then I'd also prefer this over the option parameters (as I agree with the Fowler's post about flag arguments), but if not, then I'd consider the above as a more convenient/ and clear API than the exposure of isPotentiallyFocusable.

[vschmitttest]

I asked my team for name suggestions for isPotentiallyFocusable. Someone suggested "isFocusableElementType", which seemed nice and descriptive, but overlooks the fact that certain attributes (contenteditable, tabindex) can also make a non-focusable type potentially focusable. Maybe "hasFocusableTypeOrAttributes"?

The option parameters sound like a good backup!

[mmalerba]

Do we actually care about all of the combinations here? ((true,true), (true,false), (false,true), (false,false)) or just the (true,true) and (false,false)?

I actually prefer the flagbag if we care about all of the combinations, otherwise maybe:

• isCurrentlyFocusable
• isFocusableWhenNotDisabled
• isFocusableWhenVisible
• isFoucsableWhenVisibleAndNotDisabled
  ^-- bleh, so wordy

[vschmitttest]

We care about the default (false, false), and for my use case I care about (false, true) because I want to include invisible elements but not disabled ones. I just added the includeDisabled flag to be consistent, but I don't actually need that flag, nor can I think of any cases off the top of my head where it would be needed.

[jelbourn]

Slept on it, some fresh ideas:

• hasFocusableIdentity
• hasFocusableQualities
• supportsFocus
• supportsInteraction
• isInteractive
• readyToMakeAStageDebut

[mmalerba]

I find those kind of vague compared to the ones I suggested, I wouldn't know what they do just by looking at them. And I also still like the flagbag, Martin Fowler aside

[andrewseguin]

I agree that adding "potentially" is too vague. I think using an optional object parameter containing booleans would be my preference, since it makes it more explicit from the caller code as well as making it easier to modify later

[jelbourn]

Ack- I think the config object with the options is the consensus (though I did prefer supportsFocus myself).

So it has been decided!

[vanessanschmitt]

Updated with the crowd favorite config object!

[jelbourn]

LGTM

[devversion]

Awesome. LGTM!

[vanessanschmitt]

@jelbourn Do you think this one could be merged soon? It seems like we can be fairly confident it won't break google3 since it just added an optional parameter, and I was hoping to use it for a small a11y feature. I saw it got tagged with "commit message fixup". Let me know if there's anything I can do to help clean that up!

[mmalerba]

I marked it P2 to bump it up the caretaker's priority queue. The commit message fixup thing is just a note to the caretaker to change the message when they go to merge it, nothing you need to address.

[vanessanschmitt]

Thank you!!

[angular-automatic-lock-bot]

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.}