Fix dialog initialization process

[jotaen4tinypilot]

https://github.com/tiny-pilot/tinypilot/pull/1764 introduced a regression: the paste dialog’s <textarea> doesn’t receive focus anymore when opening the dialog. This is due to us switching the dialog initialization order in order to fix another problem, which is that you may see undesired UI animation artifacts while opening the dialog.

The root cause is that calling show() after initialize() renders the this._elements.pasteArea.focus(); call irrelevant, because show also implicitly sets focus via calling showModal().

So right now, we’d have to decide between two problems:

1. Call .show() before .initialize(): users may see the animation artifact
2. Call .initialize() before .show(): the <textarea> doesn’t receive focus

Note: show() is inherited from the overlay panel, whereas initialize() is a method from the respective dialog, e.g. in <paste-dialog>.

I’d see two options:

• (1) We could revert the offending commit, because I think the focus problem is worse than the animation artifact problem. We’d have to leave an additional comment, however, since this issue is too subtle otherwise.
• (2) We take this opportunity and refactor the initialization process of dialogs in general, so that it is unified across all dialogs, and robust against such kind of issues.

Considerations on general refactoring of dialog initialization

The initialization process is basically the same in all our dialogs:

1. We trigger one operation to initialize the dialog. We don’t always have a method called .initialize(), but that’s just an accidental inconsistency.
2. We call .show()

It makes sense for these two things to occur in the stated order.

Considering the amount of dialogs we have, and that they all essentially work in the same way, we could consolidate the flow, and encode a unified initialization process in the code. For example (just to illustrate the idea):

• show() could dispatch two custom events into the dialog component:
  • dialog-requested: the dialog may listen to that event, to carry out it’s general init procedure (if applicable)
  • dialog-shown: the dialog may listen to that event and do final adjustments, e.g. for setting focus (if applicable)

[mtlynch]

I’d see two options:

1. We could revert the offending commit, because I think the focus problem is worse than the animation artifact problem. We’d have to leave an additional comment, however, since this issue is too subtle otherwise.
2. We take this opportunity and refactor the initialization process of dialogs in general, so that it is unified across all dialogs, and robust against such kind of issues.

Can you share the cost estimate (in t-shirt sizing) of (2)?

[jotaen4tinypilot]

Can you share the cost estimate (in t-shirt sizing) of (2)?

I’d say somewhere between “small” and “medium”. I don’t think it’s super complex, but we’d have to break it down into a few separate PRs, and also do testing in both Community and Pro separately.

I believe the work items are these:

• Dispatch dialog-requested and dialog-shown events from <overlay-panel> component
  • We may want to declare these events in events.js
• Set up respective event listeners in all dialogs (if applicable), and remove the initialize() et al calls from app.js
  • Attaching the dialog-shown listener in <paste-dialog> would resolve this issue.
• Document initialization flow, either in <overlay-panel> component, or in events.js
• Take care of edge cases (though after briefly scanning the code, there only appears to be one):
  • Move gkParams logic into dialog component itself

[jotaen4tinypilot]

That being said, we can also do (1) first, and then push (2) down the road.

[jdeanwallace]

Nice catch @jotaen4tinypilot! To make option (1) more appealing, since we've persisted the last masked state the original animation artifact will no longer be present/noticeable. Yay? 🥲

[jotaen4tinypilot]

The persisted toggle makes the situation better indeed, though unfortunately the animation artifact is still visible when you open the paste dialog for the first time after loading the page (if the toggle was persisted as “on”).

I think the “proper” fix would still be worthwhile, because I’d find the call order of show() and then initialize() quite unintuitive. When you pointed out the issue in the PR, I actually thought the existing order had been incorrect in the first place. By the way, we have the same issue in the <change-hostname-dialog>.

[mtlynch]

Let's do (1) now and hold off on (2).

[jotaen4tinypilot]

Okay, I’ve opened https://github.com/tiny-pilot/tinypilot/pull/1772 to fix, and extracted https://github.com/tiny-pilot/tinypilot/issues/1773 for the general refactoring.