1944189 - Caret sometimes blinks away immediately after I've typed a character (rather than resetting its paint duration with each keypress)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (just a textarea)

STR:

1. Load attached testcase.
2. Click into the textarea.
3. Get a feel for the caret blink frequency, i.e. how long the caret paints before it blinks out of existence. (It paints for roughly 1 second and the disappears for roughly 1 second, I think.)
4. Now: type "a" repeatedly, at a higher rate than the caret blink frequency (so maybe press "a" every 0.6 seconds, say).

EXPECTED RESULTS:
Caret should never disappear (because each character that I type forces it to paint, and each time it paints, it should get a full duration of its paint timer; and I'm typing another character before that timer gets used up).

ACTUAL RESULTS:
Caret periodically disappears as I'm typing, sometimes immediately after I've typed a character. (This feels a bit disconcerting, sort of like the last character that I typed just disappeared, since the caret is the last visible thing at the leading edge of the text that I'm typing, so it's disconcerting when it disappears out from under me as I'm typing.)

Chrome and my text editor both give EXPECTED RESULTS.
Firefox gives ACTUAL RESULTS.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screencast showing how this can results in caret being mostly-not-painted

If you time things "just right", this bug can end up meaning that we hardly ever paint the caret.

Here's a screencast to demonstrate. Note that the caret only appears very briefly after each keypress, and then we trip whatever timer had been queued up to change the caret showing/hiding state, and the caret immediately disappears.

Here's a profile that I captured during this screencast:
https://share.firefox.dev/4hwCwhb

If you filter the marker chart for caret, you can see that that runnable is firing on a consistent frequency regardless of my typing; and when a typed character happens to fall just before the caret runnable (as pretty much all of them do in this example), the caret only gets a chance to show up very briefly.

Comment 2

[Daniel Holbert [:dholbert]]

Here are some relevant code snippets here:
https://searchfox.org/mozilla-central/rev/a965e3c683ecc035dee1de72bd33a8d91b1203ed/layout/base/nsCaret.cpp#603,618-619,644-648

void nsCaret::ResetBlinking() {
...
  auto blinkRate =
      LookAndFeel::GetInt(IntID::CaretBlinkTime, kDefaultBlinkRate);
...
  if (blinkRate > 0) {
    mBlinkTimer->InitWithNamedFuncCallback(CaretBlinkCallback, this, blinkRate,
                                           nsITimer::TYPE_REPEATING_SLACK,
                                           "nsCaret::CaretBlinkCallback_timer");
  }

This^ looks up some platform-dependent blink rate and kicks off a runnable callback at that frequency, "CaretBlinkCallback". That callback then has this relevant line...

https://searchfox.org/mozilla-central/rev/a965e3c683ecc035dee1de72bd33a8d91b1203ed/layout/base/nsCaret.cpp#720,725

void nsCaret::CaretBlinkCallback(nsITimer* aTimer, void* aClosure) {
...
  theCaret->mIsBlinkOn = !theCaret->mIsBlinkOn;

...which simply changes the state of the caret (if it was on, then this callback turns it off).

Meanwhile, every time I press a character, something turns theCaret->mIsBlinkOn on to helpfully insta-paint the caret after my newly entered character, but then CaretBlinkCallback fires and insta-hides the caret if my timing is unlucky.

Comment 3

[Daniel Holbert [:dholbert]]

Ideally, what I would expect here is that each keypress should cancel and reinitialize mBlinkTimer, as part of the work that each keypress already does to force the caret to be shown as the new character appears (wherever that happens). That way, the caret remains visible as I'm rapidly typing, instead of intermittently blinking out of existence right after I type a character (and sometimes staying almost-forever-hidden in pathological cases like comment 1).

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, this is a Layout issue.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #4)

Well, this is a Layout issue.

Fair enough. :)

OK, I poked in pernosco to see why keypresses don't reinitialize the timer that I quoted at the start of comment 2. It's because we take this early-return:
https://searchfox.org/mozilla-central/rev/a965e3c683ecc035dee1de72bd33a8d91b1203ed/layout/base/nsCaret.cpp#628-631

if (mBlinkRate == blinkRate) {
  // If the rate hasn't changed, then there is nothing else to do.
  return;
}

That early-return was added in bug 1372761 and I think it's precisely what's causing the problem here. I'm tentatively flagging this as regression from that bug, though I haven't actually tested to confirm that (as a regression range) yet.

Conceptually, we do want to reset the timer every time we call ResetBlinking to force the caret to paint (e.g. from a keypress), so that after you press a key, you're guaranteed that the caret will remain visible for its full blink-duration (instead of insta-disappearing).

The obvious way to achieve what I'm looking for would be to just remove that early-return, but that would mean potentially kicking off a new timer (and canceling the previous one) on every keypress, and that's not too efficient -- it looks like that's what bug 1372761 was about.

Here's one idea for a more efficient way to address this (in terms of what I'll call the targetTimestamp)

1. Extend the above-quoted early-return with some code that just computes the targetTimestamp, which is the time at which we actually want to have the caret disappear (e.g. now + 1 second if we're blinking at a 1-second frequency).
2. If we get multiple keypresses in rapid succession before CaretBlinkCallback fires, each of those keypresses will just update (increase) that targetTimestamp, but none of them need to allocate a timer or anything.
3. When our existing timer fires and invokes CaretBlinkCallback: we'll now check if the caret is currently showing AND targetTimestamp is set AND it's substantially in the future from now (say >50ms away or something like that). If that combined boolean expression evaluates to true, then we opt to not hide the caret yet -- we leave it showing, and we cancel our repeating timer, and replace it with a one-shot timer that will call CaretBlinkCallback again at the appropriate time which is targetTimestamp - now. (And we reset targetTimestamp to 0 so it doesn't cause trouble if our callback fires a bit late/early.)
4. If nothing happens before that new timer fires, then we simply hide the caret as-normal at that point, and we kick off a repeating timer
5. OR, if we get more keypresses, we do the same thing discussed above (reinitializing targetTimeStamp) so that we'll continue declining-to-hide-the-caret (and periodically getting a timer callback ~once per second) for as long as the user is rapidly typing.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1372761

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1944189 - Reset blink timer even if blink rate doesn't change. r=dholbert

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/583e7a27040e
Reset blink timer even if blink rate doesn't change. r=dholbert

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/583e7a27040e