-
-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(web): key tip constraint logic requires .bottom CSS #6784
Conversation
User Test ResultsTest specification and instructions ✅ SUITE_1: no predictive text24 tests in 6 groups PASSED
✅ SUITE_2: predictive text active3 tests in 3 groups PASSED
Test Artifacts
|
@@ -78,7 +78,7 @@ namespace com.keyman.osk.browser { | |||
let canvasWidth = xWidth + Math.ceil(xWidth * 0.3) * 2; | |||
let canvasHeight = Math.ceil(2.3 * xHeight) + (ySubPixelPadding); // | |||
|
|||
kts.top = Math.floor(y - canvasHeight) + 'px'; | |||
kts.bottom = Math.floor(keyman.osk.computedHeight - y) + 'px'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this fix the issue?
keyman/web/source/osk/browser/keytip.ts
Lines 116 to 119 in 14b1485
let cs = getComputedStyle(this.element); | |
let oskHeight = keyman.osk.computedHeight; | |
let bottomY = parseFloat(cs.bottom); | |
let tipHeight = parseFloat(cs.height); |
Line 118 was broken - as it turns out, cs.bottom == 'auto'
in our currently-released version. That doesn't convert nicely to a number... causing bottomY == NaN
. That introduces a lovely set of issues, as you can imagine.
keyman/web/source/osk/browser/keytip.ts
Lines 128 to 129 in 14b1485
if(this.constrain && tipHeight + bottomY > oskHeight) { | |
const delta = tipHeight + bottomY - oskHeight; |
The problem is, we need bottomY
for our calculations that constrain the keytip within bounds. So, the best answer: flip from a .top
-based approach to a .bottom
-based one.
After a bit of analysis, I determined y
to be "the distance of the key tip's bottom from the top of its parent element." Thing is, for CSS .bottom
use, we need that to be "... from the bottom of its parent element." Fortunately, a simple calc.
- Before, for use of
.top
, it needed "the distance of the key tip's top...".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
SUITE_1: no predictive textGROUP_IPHONE_13_APP: - use the Keyman app with a (simulated or real) iPhone 13.
|
... wherein we gain yet another case of cleanup-mode "oh, I can just remove this one extra line I threw in during development" turning out to be a "bad idea." You can see that the line originally existed there before #6383 in its changelog; turns out there actually was a reason for it. SUITE_1 |
SUITE_1: no predictive textGROUP_ANDROID_APP: - use the Keyman app with a (simulated or real) Android device - your pick.
|
SUITE_1: no predictive textGROUP_IPHONE_SE_2_APP: - use the Keyman app with a (simulated or real) iPhone SE 2nd gen.
|
SUITE_1: no predictive textGROUP_IPHONE_13_APP: - use the Keyman app with a (simulated or real) iPhone 13.
|
SUITE_1: no predictive textGROUP_IPHONE_13_BROWSER: - use Safari on a (simulated or real) iPhone 13.
|
SUITE_1: no predictive textGROUP_IPHONE_SE_2_BROWSER: - use Safari on a (simulated or real) iPhone SE 2nd gen.
|
SUITE_2: predictive text activeGROUP_IPHONE_13_APP: - use the Keyman app with a (simulated or real) iPhone 13.
GROUP_IPHONE_SE_2_APP: - use the Keyman app with a (simulated or real) iPhone SE 2nd gen.
|
SUITE_1: no predictive textGROUP_ANDROID_BROWSER: - use Chrome on a (simulated or real) Android device - your pick.
|
🤦 Silly me forgot to change up the group names for in-app vs browser tests. Guess I'll patch that up manually... |
So... when the keytip got redone, apparently the 'cap' - the popup-part below - didn't actually get any styling in kmwosk.css, unlike the iOS version. Without it, we get an invisible one. Simple enough to fix; patch coming. Stuff like this is why I didn't pre-build a 🍒 version; it's rougher to update a pre-built one than to just make it right on the first go. |
@keymanapp-test-bot retest SUITE_1 GROUP_ANDROID_BROWSER all |
OK... might have pulled the trigger a little too quickly on the Android key-tip bit. It definitely needs some extra styling work, so I'll need to figure out how to fix that up. Root cause was right; it's just that there's extra work beyond it that needs doing. So, uh... Test ResultsSUITE_1: no predictive textGROUP_ANDROID_BROWSER: - use Chrome on a (simulated or real) Android device - your pick.
|
SUITE_1: no predictive textGROUP_ANDROID_BROWSER: - use Chrome on a (simulated or real) Android device - your pick.
|
The 🍒 is up at #6795. |
I'm not 100% sure of the test results. #6784 (comment) shows "PASSED" with screenshots that look clearly wrong to me, e.g. TEST_TAMIL_NORMAL_PREVIEW which has the following associated screenshot, annotated here: I'll wait to review the #6795 until these tests are confirmed okay. |
Given @mcdurdin's note above... SUITE_1 GROUP_IPHONE_SE_2_APP TEST_TAMIL_NORMAL_PREVIEW: OPEN They still pass on my local machine via local build, but the attached screenshots, as noted, should not have been marked as PASSing. So, we should double-check that they work properly for our testers, too. (It shouldn't be related, but note that my local tests are targeting iOS 14.4, rather than iOS 15.x.) |
SUITE_1: no predictive textGROUP_IPHONE_SE_2_APP: - use the Keyman app with a (simulated or real) iPhone SE 2nd gen.
|
Changes in this pull request will be available for download in Keyman version 16.0.16-alpha |
Fixes #6783.
Key tips should properly remain within the OSK's bounds again after this fix.
As this is a top-row key preview, note that its base key is completely obscured by the preview.
This fixes a regression introduced in #6383. (Gotta test the top row without predictive-text!)
User Testing
These tests are derived from those of #6383, but they're a bit more precise and targeted than before.
SUITE_1: no predictive text
Test environments
In-app groups:
In-browser groups:
Tests
TEST_TAMIL_TOP_PREVIEW: Using the Tamil99 keyboard (
ekwtamil99uni
), verify that top row keypresses do not result in a cropped key preview.TEST_TAMIL_NORMAL_PREVIEW: Using the Tamil99 keyboard (
ekwtamil99uni
), verify that keypresses in the second to last row display nicely.TEST_CHEROKEE_TOP_PREVIEW: Using the Cherokee Nation (SIL) keyboard (
sil_cherokee_nation
), verify that top row keypresses do not result in a cropped key preview.TEST_CHEROKEE_NORMAL_PREVIEW: Using the Cherokee Nation (SIL) keyboard (
sil_cherokee_nation
), verify that keypresses in the second to last row display nicely.SUITE_2: predictive text active
This is only written as a separate suite because the predictive-text test can't be done with the (same) in-browser Web test page.
Test environments
Test