-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
refactor(web): better centralizes OSK layout internals to prepare for optimization efforts 🪠 #11176
Conversation
User Test ResultsTest specification and instructions User tests are not required |
// Should not trigger a new layout reflow; VisualKeyboard should have made | ||
// no further DOM style changes since the last one. | ||
|
||
// For touch-based OSK layouts, kmwosk.css may include top & bottom | ||
// padding on the layer-group element. | ||
const computedGroupStyle = getComputedStyle(this.element); |
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.
Also of note: VisualKeyboard.refreshLayout()
and .buildDocumentationKeyboard
will not call this method unless the OSK is in the DOM, so we don't need to check for that.
Only just now realized that I left that assumption unstated here.
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.
This comment above ^^^ should be in the code, not in a PR comment.
Changes in this pull request will be available for download in Keyman version 17.0.307-beta |
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.
This looks fine
// Should not trigger a new layout reflow; VisualKeyboard should have made | ||
// no further DOM style changes since the last one. | ||
|
||
// For touch-based OSK layouts, kmwosk.css may include top & bottom | ||
// padding on the layer-group element. | ||
const computedGroupStyle = getComputedStyle(this.element); |
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.
This comment above ^^^ should be in the code, not in a PR comment.
To prepare to optimize layout-reflow for Web's OSK, it will help us to better centralize functionality and properties related to the OSK's "layer group" element. Doing so will help us to precalculate and cache values related to layout reflow without actually triggering layout reflow repeatedly during use. An initial reflow may be needed to obtain specific values, though.
This PR has been spun off of an older state of #11140. Certain design decisions may make more sense with the whole picture in mind. In particular, the new
verticalPadding
property and its backing, cached field will help in reducing layout-reflow in followups.@keymanapp-test-bot skip
Any testing relevant to do here may as well be done as part of #11140.