Fix issue where audioCurrent highlight disappears (BL-6654)

[jsubloom]

1. try to ensure .ui-audioCurrent highlight doesn't get overwritten
2. Prevent an easily reproducible case where Talking book navigation can get stuck
3. Reduce visual flash from elements getting disabled/enabled rapidly

---

This change is 

[andrew-polk]

1. Is this a new issue with the record by box feature?
2. If we can figure out why there is a second call to updateMarkupAndControlsToCurrentText happening, does that make all this unnecessary?
3. I haven't seen the flashing, so I don't know the context of the problem. The only hesitation I have there, is it seems safer to disable everything initially then enable as we know what state we are in. That prevents the user from clicking something he shouldn't.

Reviewed 3 of 3 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @JohnThomson)

[jsubloom]

1. TBD
2. if we figure out why there is a second call to CKEditor's onload(), then yes.
3. well, I was going to say theoretically you end up with the same result, but i guess now there's maybe a split second where you stay in the old state. IDK, maybe we should be .off() them initially and .on() them when they get enabled?

[jsubloom]

Regarding #1: Not repro in 4.3 and earlier versions of 4.4. Not obvious how, but somehow the new record by text box feature could be triggering a 2nd onload()? Maybe it happens when we set attributes or classes on the bloom-editable?

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @JohnThomson)

[JohnThomson]

It is definitely puzzling that this is a new issue. Is it possible that what CkEditor does only messes with the class of the root element it is applied to? Then it would not affect child spans, but now we put ui-audioSentence on the root element. However, I think what Jeffrey caught it doing was to reset the HTML of the whole element. (But maybe what it resets it to includes the whole previous content?)
I don't think disabling initially helps. We can't predict how long it might be before we can safely turn it on; times up to a half second seem to have been needed. We don't want to wait that long to turn on the highlight if we can help it.
It's a pain, but I can't think of a better workaround, so approving this (with a suggestion for one minor change that might be worth a try).

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sujeffreyl)

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts, line 812 at r1 (raw file):

            delayInMilliseconds *= 2;
        }

One possible small improvement: call setCurrentAudioElement... immediately; have it set a timer to invoke itself again after a short interval. Even 20ms might be harmless. You could count iterations or measure time to stop it after a second or two. Since it's already programmed to do nothing if there is a current ui-audioCurrent, it's harmless to call it many times.
Advantages: you can stop repeating it if you detect isEarlyAbortEnabled; this might possibly reduce any risk of problems from timeout callbacks after the page has unloaded.
Calling rapidly may make the flash less noticeable, or even hide it altogether much or all of the time.
Up to you whether you think this is worth trying.

[jsubloom]

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts, line 812 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

One possible small improvement: call setCurrentAudioElement... immediately; have it set a timer to invoke itself again after a short interval. Even 20ms might be harmless. You could count iterations or measure time to stop it after a second or two. Since it's already programmed to do nothing if there is a current ui-audioCurrent, it's harmless to call it many times.
Advantages: you can stop repeating it if you detect isEarlyAbortEnabled; this might possibly reduce any risk of problems from timeout callbacks after the page has unloaded.
Calling rapidly may make the flash less noticeable, or even hide it altogether much or all of the time.
Up to you whether you think this is worth trying.

Ok... so if I understand, the suggestion is basically to have a similar amount of total callbacks, but to do so in a chained fashion instead of a parallel one. This would reduce the amount of async calls that need to be processed in certain situations*

• I think this situations is less than thought. I think it makes sense to stop the chain for isEarlyAbortEnabled && !this.isShowing. (And that the chain method makes more sense than the parallel method).
  However, I don't think it's safe to stop the chain if audioCurrentList.Length >= 1. The issue is that the onload() could wipe out our changes let's say hypothetically say 50ms in. In this case, the callbacks at 20 ms or 40ms would abort early, but they shouldn't stop the chain because we need the callback at 60ms or 80ms to fix the UI

Regarding the timeout delay, I was worried if it was bad to have too many setTimeouts going so I used the exponential method but I don't have any problem going with a linear method if we don't mind handling a lot of callbacks.

[andrew-polk]

JT's brain works much better than mine with these things, so if he doesn't see a problem with the code, I think we can move forward.
I do think it is still worth an hour or so to see if we can determine what, specifically, is causing different behavior from before.
Note that Jeffrey shared the sequence of events with me yesterday which shows that 4.3 does actually call onload() twice just like 4.4. But the difference is when a settimeout callback occurs. @jeffrey, it may be worth adding that data to this PR so JT (and anyone else who cares) can see it.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson)

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts, line 812 at r1 (raw file):

Previously, sujeffreyl wrote…

Ok... so if I understand, the suggestion is basically to have a similar amount of total callbacks, but to do so in a chained fashion instead of a parallel one. This would reduce the amount of async calls that need to be processed in certain situations*

• I think this situations is less than thought. I think it makes sense to stop the chain for isEarlyAbortEnabled && !this.isShowing. (And that the chain method makes more sense than the parallel method).
  However, I don't think it's safe to stop the chain if audioCurrentList.Length >= 1. The issue is that the onload() could wipe out our changes let's say hypothetically say 50ms in. In this case, the callbacks at 20 ms or 40ms would abort early, but they shouldn't stop the chain because we need the callback at 60ms or 80ms to fix the UI

Regarding the timeout delay, I was worried if it was bad to have too many setTimeouts going so I used the exponential method but I don't have any problem going with a linear method if we don't mind handling a lot of callbacks.

So let me make sure I understand.
John is suggesting we just call rapidly until we have ui-audioCurrent set, but Jeffrey is saying that doesn't work because we could have it early, then it could go away, then it will eventually come back. And if we just stop as soon as we see it, that might be before it goes away?

[jsubloom]

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts, line 812 at r1 (raw file):

Previously, andrew-polk wrote…

So let me make sure I understand.
John is suggesting we just call rapidly until we have ui-audioCurrent set, but Jeffrey is saying that doesn't work because we could have it early, then it could go away, then it will eventually come back. And if we just stop as soon as we see it, that might be before it goes away?

Right, exactly... I estimate 30% of the time you can observe the Yellow Highlight on, a brief flash off, and then see Yellow Highlight re-enabled (with the new code). This illustrates the "have it early, then it could go away, then it will eventually come back"

[jsubloom]

4.3 (this sequence seems to happen consistently but I am puzzled why):
Bootstrap()
Onload assigned #1
Settimeout callback #1
Onload assigned #2
Setttimeout callback #2
Onload assigned #3
Settimeout callback #3
CKEditor done
Update markup
Set current span

4.4 record-by-text box (Unlucky):
Editable::Bootstrap
Onload is assigned
Settimeout callback #1
Onload is assigned #2
Settimeout callback #2
Onload assigned #3
CKEditor done
UpdateMarkup()
SetCurrent()
Settimeout callback #3

What I see is in 4.3, onload() is assigned 3 times...
There is DoWhenCKEditorDone(), which sounds to me like it should wait until the onload assignments finish (but from looking at the code, it doesn't look like it will properly wait until the onload setTimeout callback's finish.)
4.3 looks equally vulnerable to race conditions from what I can see, but it never seems to happen. Is it just getting ridiculously lucky??? hard to believe. But I am not sure what is causing it to happen to always execute in the right sequence.
And I am not sure what the new code is doing to ruin its chances. I mean, my changes are all below DoWhenCkEditorDone(). And the problem is that the 3rd onload SetTimeout completes after CKEditorDone()... however, if I only modify descendants of CkEditorDone, how can I be negatively affecting CKEditorDone() or things that happen before CKEditorDone()? That part greatly puzzles me too.

Directions:

1. Investigate if there is a bug/interaction introduced by new code which is causing the timing issue to show up more? This is still not really fixing the root cause IMO, just treating the symptoms at a different layer. Because as far as I can currently tell, the root issue is in 4.3 I just can't trigger a repro to prove it.
2. Should I just modify CKEditorDone() not to have SetTimeout? (Sounds like an even bigger can of worms to test)
3. Should I try to fix DoWhenCkEditorDone()? It seems........ extremely difficult to know when all the SetTimeout callbacks are finished. Maybe more plausible if I modify the CkEditor code itself, but it is minified JSON so great care needed.
4. is there a problem in my understanding of the issue and we really did introduce a new root cause?

[andrew-polk]

Ok, with JT's review, I am going to merge this so we can get it tested. We all agree this isn't the most ideal fix but should work.
We can continue to research more after JH has a chance to weigh in.
I don't think we're going to push 4.4 on a Friday, anyway, and with JH returning on Monday.
So, this gives us the chance to test this change and make sure it didn't introduce more problems so we are hopefully ready to deploy Monday morning.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson)

[JohnThomson]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrew-polk)

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts, line 812 at r1 (raw file):

Previously, sujeffreyl wrote…

Right, exactly... I estimate 30% of the time you can observe the Yellow Highlight on, a brief flash off, and then see Yellow Highlight re-enabled (with the new code). This illustrates the "have it early, then it could go away, then it will eventually come back"

I didn't suggest stopping the sequence of timeouts if ui-audioCurrent is already present, just (as now) abort the CURRENT attempt to set it on the first possible element (so we don't move it back if the user has quickly pressed Next). I agree we need to keep doing timeouts since we don't know when CkEditor might mess things up. Maybe stop after a second or so as experiment seems to indicate that is safe. And of course we can stop if we don't need the highlight any more (e.g., page closed).

[jsubloom]

---

src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts, line 812 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I didn't suggest stopping the sequence of timeouts if ui-audioCurrent is already present, just (as now) abort the CURRENT attempt to set it on the first possible element (so we don't move it back if the user has quickly pressed Next). I agree we need to keep doing timeouts since we don't know when CkEditor might mess things up. Maybe stop after a second or so as experiment seems to indicate that is safe. And of course we can stop if we don't need the highlight any more (e.g., page closed).

got it, makes sense, sounds like we were all on the same page