1987232 - Date/Time widget's internal scripts trigger redundant/reentrant calls to focus-handler

Status: NEW

(Core :: DOM: Forms, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Spinning this off from bug 1889803 and bug 1983702; this is part of the problem in those bugs.

STR:

1. Load attached testcase.

EXPECTED RESULTS:

• The widget should have a lime outline (indicating that it's focused).
• The page should show logging to indicate that the focus handler was called exactly once ("didFocus call 0")

ACTUAL RESULTS:

• input widget has no lime outline (it's not focused).
• Logging indicates that the focus handler was called 3 times. (with call #1 happening during call #0!)

Comment 1

[Daniel Holbert [:dholbert]]

Based on a pernosco analysis in bug 1889803, I think what's happening here is basically all coming from this internal JS:
https://searchfox.org/firefox-main/rev/5ea04164ef9f7e0d2baf2afc34edac30e79f78bd/toolkit/content/widgets/datetimebox.js#120-140

rebuildEditFieldsIfNeeded() {
  if (
    this.shouldShowSecondField() == !!this.mSecondField &&
    this.shouldShowMillisecField() == !!this.mMillisecField
  ) {
    return;
  }

  let focused = this.mInputElement.matches(":focus");

  this.removeEditFields();
  this.buildEditFields();

  if (focused) {
    this._focusFirstField();
  }
}

_focusFirstField() {
  this.shadowRoot.querySelector(".datetime-edit-field")?.focus();
}

When the testcase changes the field type (in our focus handler), we synchronously call rebuildEditFieldsIfNeeded. That function rebuilds the pieces of the field, and then synchronously calls focus() on one of the freshly-rebuilt inner components. This is a new focus event, which triggers an additional call to the focus handler (while we're still inside of the first call to the focus handler!!). The inner call changes the type of the field again, but fortunately we don't get another reentrant call (I'm not sure why offhand; perhaps because we haven't finished updating the value for the new type, or something).

Then after both of those focus-handlers complete, we get one more call to the focus-handler (for reasons I haven't yet dug into).

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (adjusted to repro the issue in older builds too)

Here's a version of the testcase that uses time instead of datetime-local and plain JS strings instead of backtick strings -- this reproduces the issue in Nightly and its syntax is all recognized further back than testcase 1's syntax, which makes it useful for mozregression.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

EXPECTED RESULTS:

• The widget should have a lime outline (indicating that it's focused).

This doesn't happen in any old builds that I've tried.

• The page should show logging to indicate that the focus handler was called exactly once ("didFocus call 0")

This part did used to work (but possibly only by accident). This is where testcase 2 starts showing logging for multiple calls to didFocus at pageload:

Last good revision: 2bd46b2573e6e52f396483be1822021b475deb70 (2021-09-07)
First bad revision: a72096bff057d469bf02132d723af1091f19f728 (2021-09-08)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2bd46b2573e6e52f396483be1822021b475deb70&tochange=a72096bff057d469bf02132d723af1091f19f728

In that range, I'm guessing this would have been from bug 1413836, which is where we started letting component-parts (e.g. the thing that we focus at the end of my code-quote in comment 1) transfer their focused state to their component (e.g. the <input> element). (Before that point, probably we just dropped some focus state on the floor, which just-so-happened to make us produce the expected-result here, but not for good reasons.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (not exactly testing this bug, just exploring how we handle focus when type dynamically changes from 'time' to 'date')

Here's a testcase to try to see if that code-snippet from comment 1 actually restores focus in simpler cases, as it seems to be trying to do.

STR for this testcase: Shift+reload this testcase, and then see what happens to the lime outline (after 1s when the widget type changes).

If we preserved the focus state, it'd remain lime; but it doesn't. So in this case at least, it doesn't restore focus like it's trying to. (We used to preserve the lime outline before bug 1815526, but that was actually a mirage -- we styled this testcase's final widget as focused before that point, but it wasn't actually focused, as can be seen by e.g. pressing Spacebar or Tab [which revealed that focus had in fact been reset])

Comment 5

[Daniel Holbert [:dholbert]]

Note for prioritization purposes... This is part of the root cause of a fuzzer-crash (triggering a beta/nightly-only diagnostic-assert) that fuzzers are rediscovering in different bugs, in bug 1889803 and (duplicate) bug 1983702.

The crash signature has been one of our topcrashes for beta at various points in time, but I don't think this specific way-of-hitting-it (with the datetime widget) is the way in which users in the wild are tripping over it. Still: if we could fix this, that would probably help fuzzers generate new testcases that hit the same crash signature in ways that are closer to how users are actually crashing (without those testcases being discarded as instances of a known crash).