Closed Bug 1450219 Opened 2 years ago Closed 11 months ago

Datetimebox may lose focus when the value changes.

Categories

(Core :: Layout: Form Controls, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: Gijs, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

STR:

0. remove this block of findbar code and replace with an early return; :
https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/toolkit/content/browser-content.js#986-992

For convenience: this is a bubbling system-group keypress handler and it does the following with the event:

    let fakeEvent = {};
    for (let k in event) {
      if (typeof event[k] != "object" && typeof event[k] != "function" &&
          !(k in content.KeyboardEvent)) {
        fakeEvent[k] = event[k];
      }
    }


1. run ./mach mochitest dom/html/test/forms/test_input_time_key_events.html

ER:
test passes, why would <input type=time> care about findbar code?

AR:

The test fails, specifically this part:

    // PageDown on second field increments second by 10.
    keys: ["KEY_Tab", "KEY_Tab", "KEY_PageDown"],


Some helper code dispatches those keys and expects it to tab through hours, then minutes, then alter the seconds field. The test fails because it alters the hours field instead. I have no idea why the previous test (tab, tab, pageup) seems to consistently pass for me on my system. Probably a race condition of sorts...


The code in that findbar loop accesses every event property value. Including (potentially among other things, this is what I bisected it to, maybe there are other properties that do this) event.rangeOffset.

Fun trick: before the early return, add back: event.rangeOffset;, and rerun the test - it'll pass again.

From the UIEvent implementation ( https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/dom/events/UIEvent.cpp#215 )

  nsCOMPtr<nsIPresShell> shell = mPresContext->GetPresShell();
  if (!shell) {
    return 0;
  }

  shell->FlushPendingNotifications(FlushType::Layout);

Oh.


So, I intend to work around this in bug 1371523 by making the test flush layout (elem.clientTop;) between sending keys, which fixes things for me. Note that making the binding access event.rangeOffset in the keypress handler didn't seem to help - it has a capturing handler and presumably it's too late to update the event's original target at that point, or something. I don't think it's possible for a user to trigger this. Hopefully. But it's an interesting quirk in the input time/date implementations. I don't really know how to fix it "properly" because AFAICT the handler for the pageup/pagedown uses the key event's originalTarget to determine which field to change, and I don't really understand why that would be incorrect in this particular case.

Perhaps the sync flush in the test is enough and we should just not worry about this issue (that is, I would be surprised if there was a user-facing effect), but it was surprising enough that it seemed like it'd warrant at least a comment from people more familiar with these controls and how event.originalTarget works, as it's confusing to me that it could be "wrong" in this way. Seems Jessica, who implemented them, is inactive, so ni'ing some reviewers instead...
Flags: needinfo?(mconley)
Flags: needinfo?(dholbert)
Oh, I should have mentioned, this test relies on being executed on a 12-hour-clock (AM/PM) system, so it fails lots of stuff on my local machine - hackaround is to just make `is12HourTime` in toolkit/content/widgets/datetimebox.xml return `true` all the time.
(Canceling my needinfo -- I don't really know about event handling internals of this widget, sorry.  IIRC I mostly reviewed the frame class, nsDateTimeControlFrame.cpp, and some of the flex styling on the children.  I don't know anything about these key handlers / events & their dependencies.)
Flags: needinfo?(dholbert)
Passing ni for comment #0 from dholbert to Olli...
Flags: needinfo?(bugs)
I'm afraid this is lower-level into our event handling code than I'm familiar with. I don't think I'll be much use.

Perhaps masayuki might have some insight?
Flags: needinfo?(mconley) → needinfo?(masayuki)
I don't expect this to be about event handling, more about xbl and date/time implementation and focus handling.
I wonder if this was working better before bug 1366250
Flags: needinfo?(masayuki)
Priority: -- → P3
So I took a look to this, because with my patch for bug 1505887 one of these tests start failing consistently. I dug a bit and I see what's happening. Here's a reduced test-case:

<!doctype html>
<input type="time">
<script>
onload = () => {
  let input = document.querySelector("input");
  input.focus();
  input.value = "08:10:10";
};
</script>

Expected result is that the input is still focused. Actual result is "it's not".

This is because when the value change, the datetime widget may end up removing the focused element in rebuildEditFieldsIfNeeded(), which removes the actually focused element, and doesn't restore the focus on the newly created element.
Assignee: nobody → emilio
Blocks: 1505887
Flags: needinfo?(bugs)
Summary: Input type=time / input type=date XBL bindings manipulate the wrong field (event.originalTarget can be wrong?) which breaks dom/html/test/forms/test_input_time_key_events.html when nothing flushes layout between keypresses → Datetimebox may loose focus when the value changes.
(rebuildEditFieldsIfNeeded is called when the value attribute changes)
Summary: Datetimebox may loose focus when the value changes. → Datetimebox may lose focus when the value changes.
Removing the inner field while focused nulls out the active element, without a
blur event of course, so this means that we left the :focus state in the input
set incorrectly, plus that we actually lost focus.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/6da0db82505c
Make sure not to lose track of focus when we rebuild the inner fields. r=Gijs
Is this only affects datetime UA Widget and not datetime XBL binding? I don't see the same change made to the XBL binding.
Flags: needinfo?(emilio)
https://hg.mozilla.org/mozilla-central/rev/6da0db82505c
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
I was assuming we were not maintaining the XBL stuff anymore, but yes, this does affect the XBL implementation.
Flags: needinfo?(emilio)
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/e97253b0e95c
followup: Fix the XBL implementation as well.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)
> I was assuming we were not maintaining the XBL stuff anymore, but yes, this
> does affect the XBL implementation.

We won't need the XBL stuff if everything goes well, but we do want to keep the escape hatch intact.
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14102 for changes under testing/web-platform/tests
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.