Skip to content

fix(web): default popup key selection, space highlight after popup#4306

Merged
jahorton merged 4 commits intobetafrom
fix/web/default-popup-key
Jan 22, 2021
Merged

fix(web): default popup key selection, space highlight after popup#4306
jahorton merged 4 commits intobetafrom
fix/web/default-popup-key

Conversation

@jahorton
Copy link
Copy Markdown
Contributor

When reviewing this PR, it may be helpful to revisit #3718:

This PR standardizes the popup keys by not pre-pending the base key in the popup array.

When we stopped prepending the base key for phone-oriented form factors... we forgot to stop auto-selecting the first subkey. Before we disabled said prepending behavior, this had been intended to indicate that the base key was still selected... just as a subkey.

Of course, we still want to preserve this functionality if the keyboard specifies a subkey matching its base key. However...

Also from #3718:

This also fixes #3092 because now Keyboard creators can control if/where the base key appears in the popup keys.

So, we're given no guarantee for where the base key lies within the subkey array.


That said, there was an interesting feature that this enabled:

Screen Shot 2021-01-21 at 12 17 10 PM

There are some cases where it may be appropriate to auto-select a subkey that doesn't match its base key. Due to the 'bug' left by #3718, this is behavior that's been in the 14.0 beta for a while, and it's pretty nifty for typing this language with a touch layout.

So, to preserve this 'nifty' behavior, I noticed and preserved a reasonable condition that developers could consider to deliberately allow this: the base key's ID matches that of the subkey, but the layer is different - the subkey highlighted above has layer = 'shift'. So, I turned it into a selection rule - if there's no perfectly-matching subkey, we'll take the first "near-match".


Interestingly, this fixes point 2 of #2990!

@jahorton jahorton added this to the B14S4 milestone Jan 21, 2021
@mcdurdin
Copy link
Copy Markdown
Member

mcdurdin commented Jan 22, 2021

So, to preserve this 'nifty' behavior, I noticed and preserved a reasonable condition that developers could consider to deliberately allow this: the base key's ID matches that of the subkey, but the layer is different - the subkey highlighted above has layer = 'shift'. So, I turned it into a selection rule - if there's no perfectly-matching subkey, we'll take the first "near-match".

I like the concept but I am concerned that it adds unwarranted complexity to the touch layouts. This kind of 'accidental' feature is difficult to maintain over time and leads to complications, if we want to add more functionality to the OSK. I prefer these kinds of features to be deliberate -- e.g. having an isDefault property on the subkeys (which can be true only for one subkey in any given subkey array). That's more flexible in design and to accommodate. So, can you disable that functionality and we can revisit that in 15.0?

Comment on lines +1733 to +1738
if(skSpec.id == baseKey.keyId && skSpec.layer == baseKey.key.layer) {
bk = skElement;
break; // Best possible match has been found.
} else if(!bk && skSpec.id == baseKey.keyId) {
bk = skElement;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(skSpec.id == baseKey.keyId && skSpec.layer == baseKey.key.layer) {
bk = skElement;
break; // Best possible match has been found.
} else if(!bk && skSpec.id == baseKey.keyId) {
bk = skElement;
}
//if(skSpec.isDefault) { TODO for 15.0
// bk = skElement;
// break;
//} else
if(skSpec.id == baseKey.keyId && skSpec.layer == baseKey.key.layer) {
bk = skElement;
break; // Best possible match has been found.
}

Copy link
Copy Markdown
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jahorton jahorton merged commit 745c328 into beta Jan 22, 2021
@jahorton jahorton deleted the fix/web/default-popup-key branch January 22, 2021 02:44
@keyman-server
Copy link
Copy Markdown
Collaborator

Changes in this pull request will be available for download in Keyman version 14.0.228-beta

@keyman-server
Copy link
Copy Markdown
Collaborator

Changes in this pull request will be available for download in Keyman version 15.0.9-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants