bug(core): LDML processor handling "Enter" frame keys etc does not reset/invalidate the context for non-compliant apps

[rc-swag]

Edit: Update after further investigation.
When a Enter is pressed with any app non-compliant or compliant, the KMX process will reset(invalidate) the context. What happens on the following keystroke for a compliant app is the context is read again which includes the newline marker and then is set in the core with km_core_state_context_set_if_needed api call.

With the LDML processor, the context is not reset. There is no LF character inserted into the context either, however, that would be incorrect, why because not being context away the enter could have moved the cursor to a new text field.

LDML processor after
aEnterc
Process_Event_Core: context cache after: |ac| (len: 2) [ U+0061 U+0063 ]
KMX processor after
aEnterc
Process_Event_Core: context cache after: |c| (len: 1) [ U+0063 ]

KMX Processor Log for non-compliant
aEnter

kmtip: _KeymanProcessKeystroke (d 1c0001   ex=0)
130	TIPProcessKey: Enter VirtualKey=['Enter' 0xd] lParam=1c0001   IsUp=0 Extended=0 Updateable=0 Preserved=0
164	Key pressed: ['Enter' 0xd] Context '|a| (len: 1) [ U+0061 ]'
47 kmtip: KeymanGetContext: Exit: Context does not support manipulation.  Using legacy interaction
287	AITIP::ReadContext: transitory context, so use buffered context [Updateable=0]
108	ProcessEvent: vkey[13] ShiftState[0] isDown[1]
121	Process_Event_Core: context app before:   |a| (len: 1) [ U+0061 ]
122	Process_Event_Core: context app after:    || (len: 0) [ ]
123	Process_Event_Core: context cache before: |a| (len: 1) [ U+0061 ]
124	Process_Event_Core: context cache after:  || (len: 0) [ ]
149	ProcessActionsNonUpdatableParse EMIT_KEYSTROKE
214	TIPProcessKey: Success, res=0

d

164	Key pressed: ['B' 0x42] Context '|| (len: 0) [ ]'
47	kmtip: KeymanGetContext: Exit: Context does not support manipulation.  Using legacy interaction
287	AITIP::ReadContext: transitory context, so use buffered context [Updateable=0]
108	ProcessEvent: vkey[66] ShiftState[0] isDown[1]
121	Process_Event_Core: context app before:   || (len: 0) [ ]
122	Process_Event_Core: context app after:    |b| (len: 1) [ U+0062 ]
123	Process_Event_Core: context cache before: || (len: 0) [ ]
124	Process_Event_Core: context cache after:  |b| (len: 1) [ U+0062 ]
214	TIPProcessKey: Success, res=1

WordPad Compliant app
aEnter

kmtip: GetLeftOfSelection(63) = U+0061  [1 fetched]
         282	AITIP::ReadContext: full context [Updateable=1] U+0061 
	100	Process_Event_Core: set_if_needed result: 0
	108	ProcessEvent: vkey[13] ShiftState[0] isDown[1]
	121	Process_Event_Core: context app before:   |a| (len: 1) [ U+0061 ]
	122	Process_Event_Core: context app after:    || (len: 0) [ ]
	123	Process_Event_Core: context cache before: |a| (len: 1) [ U+0061 ]
	124	Process_Event_Core: context cache after:  || (len: 0) [ ]

[mcdurdin]

The reason for this, I guess, is that the KMX processor treats the Enter key as a 'frame key' and passes the raw key event to the app, which causes a context reset?

[srl295]

🙀

[mcdurdin]

Assigning to you @srl295 in next sprint, as this probably should be addressed before we leave beta

[rc-swag]

The reason for this, I guess, is that the KMX processor treats the Enter key as a 'frame key' and passes the raw key event to the app, which causes a context reset?

This question made me go look at the logs again because what you said didn't seem correct. I found that the KMX processor resets the context regardless of compliant or not. I have updated the issue to match. The KMX processor makes the decision to invalidate the context so it is reset in the core in the process_event call before the EMIT keystroke to the app has occurred.

[rc-swag]

@srl295 I was testing for #10227 and discovered that it is not just Enter that it is not resetting the context. Looking at the history I now see why. We moved to the ldml processor just pushing an action struct to the core, and we lost the invalidated context or the state->actions().push_invalidate_context(); On the OS side, the engine did not need to know about invalidate context but on the core side it does. Well at least in the non-compliant app stage.
I need to think more about the best solution to this. It may also include bringing the VKReset struct back. #10227 and this PR are now very much related.

[srl295]

So maybe we need a flag in the struct for context reset ?

[mcdurdin]

I think we need a discussion on this. The decision on what is a frame key is probably OS-dependent. So some of this logic may belong in Engine, some in Core.

[srl295]

Just to recap the current state (heh) of the ldml_processor.. when a key is 'not found' in the kmap, on key-down ldml_processor::process_key_down() will call:

void ldml_event_state::emit_passthrough_keystroke() {
  // assert we haven't already requested a keystroke
  assert(actions.emit_keystroke != KM_CORE_TRUE);
  actions.emit_keystroke = KM_CORE_TRUE;
}

The processor definitely doesn't (and, i'd think ideally wouldn't) know whether a VK is a frame key or not. Should the above be sufficient, and engine/core take it from there? Or should there be a hint set which explicitly says, "the processor did not find the requested key"?

Looking at KM_CORE_STATUS_* it doesn't seem like it would be a good fit to return a 'key not processed' status there. Just to rule that out.

[rc-swag]

Just to recap the current state (heh) of the ldml_processor.. when a key is 'not found' in the kmap, on key-down ...

Yes, this is the spot I was originally thinking you would do a lookup into the VKResetContext data structure at this point unless we can push into the core logic to make the decision. It is almost the equivalent spot in the kmx_processor [ except it has already determined whether it is a character, note and it doesn't map exactly to the old VKResetContext data structure ].

We know the information need for the decision it is:

if actions.emit_keystroke && ((key_pressed is a platform_frame_key || key_pressed == backspace && actions.code_points_to_delete ==0)) {
state->context().clear();
}

This could be a call made in km_core_process_event after the process_event call to the relevant processor
Then we just need the frame keys as an env variable passed through on keyboard creation.
We will have a meeting about this next week.

[rc-swag]

rough psudeo code could be small private function.

if actions.emit_keystroke && ((key_pressed is a platform_frame_key || key_pressed == backspace && actions.code_points_to_delete ==0)) {
state->context().clear();
}

where platform_frame_key == the VKResetArray

This could be a call made in km_core_process_event after the process_event call to the relevant processor

For the future we could add support for different frame keys on different OS's
Have the vkey arrays defined for each OS in a header file and then the environment variable would just be used to choose the correct vkey array.
items[4].scope = KM_CORE_OPT_ENVIRONMENT;
items[4].key = KM_CORE_VKEY_ENV_PLATFORM;
items[4].value = WINDOWS_PLATFORM_ENV;