Close search bar in Connect on blur by ravicious · Pull Request #25186 · gravitational/teleport

ravicious · 2023-04-26T11:31:04Z

Adding onBlur to the search input fixes two scenarios:

Pressing the shortcut to open a new terminal tab while the search bar is open now automatically closes the search bar.
It is now possible to tab out of the search bar.
- I don't 100% understand what the problem was before. Matter of fact is that pressing Tab while the input was focused did result on the input receiving a blur event, but the input was receiving focus back again. Since we had onFocus on the overall search bar container and not the input itself, I imagine it might have went like this:
  - You click on the search bar.
  - onFocus on the search bar container gets called.
  - open gets called, rendering a new input with autoFocus.
  - The new input gets focused.
  - Pressing Tab at this point blurs the input and shifts focus to another element within the search bar.
  - onFocus of the search bar container gets triggered again.
- I haven't verified this though.

As the comment in SearchBar explains, the focus wasn't actually achieving anything. I added it only because I was testing SearchContext in separation and depended on the input getting focused in the test. This doesn't reflect how the input is actually used so I adjusted the test.

ravicious · 2023-04-26T11:32:43Z

  isOpen: true,
  open: () => {},
  close: () => {},
-  closeAndResetInput: () => {},


In the upcoming commits I'm going to add closeWithoutRestoringFocus. I didn't want to have three different close variants and I didn't want to have close({restoreFocus: boolean, resetInput: boolean}) either. So I decided to get rid of closeAndResetInput as it was used only in a single specific situation which already also called resetInput anyway.

ravicious · 2023-04-26T11:36:50Z

+  // If that element is present though, onBlur takes precedence over onClickOutside. For example,
+  // clicking on a button outside of the search bar will trigger onBlur and will not trigger
+  // onClickOutside.
+  const closeIfAnotherElementReceivedFocus = makeEventListener(


This is an unorthodox name, but I don't like listeners being called like onBlur and handleBlur if they do something more complex because then you have no idea what's happening in them.

That's the same reason why I renamed the prop in the following piece of code from onShowDetails to showErrorsInModal:

teleport/web/packages/teleterm/src/ui/Search/pickers/ActionPicker.tsx

Lines 217 to 220 in 5bf72af

<ExtraTopComponents

status={actionPickerStatus}

getClusterName={getClusterName}

showErrorsInModal={showErrorsInModal}

What do you mean by "more complex". More complex than what? To me, handleBlur makes sense because it sounds like "when this event happens, we are going to handle it like this" and that seems very open ended in terms of complexity and viable to me, imo. I'd love to know your thoughts a bit deeper.

because then you have no idea what's happening in them

I suppose I understand this from a prop -> passed func readability (as in, the prop name is onBlur and you pass it a function that describes what it does). If I were to read it outloud as a normal sentence I could see it like "on blur, close if another element received focus" so that makes sense the way you're using it here

Hmm maybe the complexity of the handler is the wrong way to look at it. What I should've said is that if I'm writing a generic component that's going to be used in many different contexts, say a <Button> or <Input>, then onClick, onBlur accepted through props all make sense because <Button> doesn't care what the handler does.

But if I'm working on a specific component in a specific context, then IMHO giving specific names to handlers makes it easier to understand what's going on, as you described. Otherwise all I see is just onBlur or handleBlur and if I want to know what actually happens on blur, I have to go to the function and I have to read through it because there's no variable name which would summarize what the function does.

Matter of fact, the onInputValueChange function exported from SearchContext has this problem too, I'm going to rename it in the next commit.

I think it makes sense to use more specific names when possible.
If I needed to run a few unrelated actions in a handler, then probably I would just call it handleBlur.

ravicious · 2023-04-26T11:38:21Z

+  const closeWithoutRestoringFocus = useCallback(() => {
+    previouslyActive.current = undefined;
+    close();
+  }, [close]);


I wish I didn't have to implement it like this, I'd rather have close which doesn't restore focus and then another version of close which does. This way we wouldn't have to depend on some state interactions to achieve the effect we want.

But given that in all but one case we want to restore focus, I just couldn't find a way to do this without making the code unnecessarily complicated.

ravicious · 2023-04-26T11:39:28Z

  const close = useCallback(() => {
    setIsOpen(false);
    setActivePicker(actionPicker);
-    if (previouslyActive.current instanceof HTMLElement) {


Grzegorz, do you remember if this check was only to satisfy TypeScript? Or if previouslyActive can actually be some other kind of element and we want to execute this only when it's HTMLElement?

I replaced it with a simple truthiness check because I wanted to use a simple mock in tests instead.

teleport/web/packages/teleterm/src/ui/Search/SearchContext.test.tsx

Lines 185 to 187 in 058cb0a

const previouslyActive = {

focus: jest.fn(),

} as unknown as HTMLInputElement;

AFAIK previouslyActive can be for example an SVGElement that doesn't have focus method, so we want to run it only for HTMLElement. That's why I checked an instance of the element.
https://stackoverflow.com/a/47198823

I think you can still have that simple mock in a test, but only revert the instanceof check?

SVGElement extends HTMLOrSVGElement which has focus, though I've seen people mention what you said (microsoft/TypeScript#5901 (comment)), but I suppose it has changed since 2015.

I cannot use the instanceof check together with a simple mock because the instanceof check is done in runtime and the object is not an instance of HTMLElement.

MDN doesn't explain how it's possible for document.activeElement to be an Element that doesn't have focus function. I assume this is just due to the internal browser engine hierarchy of DOM objects.

For now, I'll revert previouslyActive back to just Element to adhere to the browser API. Before calling focus I'll check if previouslyActive has the focus property which is a function. I think this is better than an instanceof check because after all we're not interested in the element being an instance of a specific class, we just want to call focus on it.

I have to use a less type-safe check anyway because the current code won't pass CI until #25683 gets merged and I don't want to be blocked by it. I'll add a TODO comment to address it later.

Edit: I actually didn't change the check, I just temporarily replaced previouslyActive.current.focus() with previouslyActive.current['focus'](). We do the manualy "type check" on this property beforehand so it's safe to call it without null checks.

Edit 2: nvm, I had to change the check after all. ( .__.)

ravicious · 2023-04-26T11:43:23Z

+   * to add a listener to an element directly. By wrapping the listener in makeEventListener, you
+   * make sure that the listener will be removed when the interaction with the search bar is paused.
+   */
+  const makeEventListener = useCallback(


Technically the function has nothing to do with event listeners, it just simply does not return the passed value if isUserInteractionPaused is true.

So we could have a more generic name but since this function would be used only for event listeners, I feel like makeEventListener is a better name than a generic one.

I tried coming up with something like makePausableEventListener but I didn't think of anything that I'd like.

gzdunek

Looks good, but I think we should revert checking the instance of previouslyActive, so the app won't crash in the runtime.

gzdunek · 2023-04-28T09:50:20Z

+        !containerRef.current.contains(elementReceivingFocus);
+
+      if (isElementReceivingFocusOutsideOfSearchBar) {
+        closeWithoutRestoringFocus(); // without restoring focus


Nit: this comment just repeats the function name

Ah, good catch, I forgot to remove it when I was still prototyping since I originally called just close there. ;)

gzdunek · 2023-04-28T10:10:57Z

  const close = useCallback(() => {
    setIsOpen(false);
    setActivePicker(actionPicker);
-    if (previouslyActive.current instanceof HTMLElement) {


AFAIK previouslyActive can be for example an SVGElement that doesn't have focus method, so we want to run it only for HTMLElement. That's why I checked an instance of the element.
https://stackoverflow.com/a/47198823

I think you can still have that simple mock in a test, but only revert the instanceof check?

gzdunek · 2023-04-28T10:29:26Z

+  // If that element is present though, onBlur takes precedence over onClickOutside. For example,
+  // clicking on a button outside of the search bar will trigger onBlur and will not trigger
+  // onClickOutside.
+  const closeIfAnotherElementReceivedFocus = makeEventListener(


I think it makes sense to use more specific names when possible.
If I needed to run a few unrelated actions in a handler, then probably I would just call it handleBlur.

public-teleport-github-review-bot · 2023-05-05T11:05:33Z

@ravicious See the table below for backport results.

Branch	Result
branch/v13	Create PR

ravicious added 4 commits April 26, 2023 13:25

Remove closeAndResetInput

1abc27b

Move onFocus on input, add onBlur to input

058cb0a

Remove nesting from handling outside click

41d7d02

ravicious added the backport/branch/v13 label Apr 26, 2023

ravicious commented Apr 26, 2023

View reviewed changes

ravicious marked this pull request as ready for review April 26, 2023 11:43

github-actions Bot added size/md ui labels Apr 26, 2023

github-actions Bot requested review from rudream and ryanclark April 26, 2023 11:44

ravicious requested review from avatus and gzdunek and removed request for rudream and ryanclark April 26, 2023 11:44

avatus approved these changes Apr 26, 2023

View reviewed changes

SearchContext: Rename onInputValueChange to setInputValue

7b55bea

gzdunek approved these changes Apr 28, 2023

View reviewed changes

ravicious added 2 commits May 4, 2023 18:02

Do proper type check before calling focus on previouslyActive

4f9077e

Add a temp workaround for tsc issue

2b4773f

ravicious enabled auto-merge May 5, 2023 10:07

ryanclark approved these changes May 5, 2023

View reviewed changes

Make TypeScript 4.8.4 happy

e50de42

ravicious added this pull request to the merge queue May 5, 2023

Merged via the queue into master with commit f01e675 May 5, 2023

ravicious deleted the ravicious/search-bar-blur branch May 5, 2023 11:04

ravicious mentioned this pull request May 5, 2023

[v13] Close search bar in Connect on blur #25690

Merged

	<ExtraTopComponents
	status={actionPickerStatus}
	getClusterName={getClusterName}
	showErrorsInModal={showErrorsInModal}

	const previouslyActive = {
	focus: jest.fn(),
	} as unknown as HTMLInputElement;

Conversation

ravicious commented Apr 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravicious May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzdunek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

public-teleport-github-review-bot Bot commented May 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

avatus Apr 26, 2023 •

edited

Loading

ravicious May 5, 2023 •

edited

Loading