150ms sync reflow in keyboard resizeUI on capitalization



5 years ago
4 years ago


(Reporter: janjongboom, Assigned: bkelly)



Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v1.4 fixed)



(1 attachment, 1 obsolete attachment)

We use window.innerWidth / window.innerHeight in render.js to determine whether we're in landscape or portrait mode. This causes a big reflow.


if (window.innerWidth <= window.innerHeight) {

We should get rid of this.
We should maybe use the props under |screen|, which will break in FF nightly and B2G desktop; so we should patch that.

Comment 2

5 years ago
Thanks for filing Jan!

This causes a 150ms sync reflow on the first key press when typing in Messages on the tarako device.  Its a noticeable pause that we probably want to address for any demonstration.  It will effect any app input field that auto-capitalizes.

Here is the patch that removes the sync reflow.  We may want to land just on the 1.3t branch for now if this would break b2g-desktop or the browser environment.  It does seem, though, that maybe we should make |screen| work as it would with the device in those environments.

    diff --git a/apps/keyboard/js/render.js b/apps/keyboard/js/render.js
    index c50e78e..2cbe112 100644
    --- a/apps/keyboard/js/render.js
    +++ b/apps/keyboard/js/render.js
    @@ -705,13 +705,14 @@ const IMERender = (function() {
         var changeScale;
         // Font size recalc
    -    if (window.innerWidth <= window.innerHeight) {
    -      changeScale = window.innerWidth / 32;
    +    var orientation = screen.mozOrientation;
    +    if (orientation.contains('portrait')) {
    +      changeScale = screen.width / 32;
           document.documentElement.style.fontSize = changeScale + 'px';
         } else {
    -      changeScale = window.innerWidth / 64;
    +      changeScale = screen.width / 64;
           document.documentElement.style.fontSize = changeScale + 'px';
    @@ -733,7 +734,7 @@ const IMERender = (function() {
         layoutWidth = layout.width || 10;
         // Hack alert! we always use 100% of width so we avoid calling
         // keyboard.clientWidth because that causes a costy reflow...
    -    var totalWidth = window.innerWidth;
    +    var totalWidth = screen.width;
         var placeHolderWidth = totalWidth / layoutWidth;
         var rows = activeIme.querySelectorAll('.keyboard-row');
blocking-b2g: --- → 1.3T?
Keywords: perf
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Summary: There's a unnecessary reflow in resizeUI → 150ms sync reflow in keyboard resizeUI on capitalization

Comment 3

5 years ago
This profile of the 1.3t with bug 875963, bug 967292, and bug 971651 shows the sync reflow:


It takes roughly 850ms from keyboard touch to end of paint in the client process.

With the patch from comment 2:


This removes the sync reflow and reduces the touch-to-paint time to roughly 650ms.

There is style a sync reflow in endPress() from the getComputedStyle() call, but it only takes ~5ms here.
Here's an idea: can we check on keyboard init code if screen width is the same as window.innerWidth (as long as we don't get a perf hit from this of course, but probably there's already reflows going on there that we can piggyback on...), and if so we use screen.width and screen.height; otherwise we use the window.innerWidth methods. That way it'd still work in b2g desktop, because now the keyboard will always render in landscape mode (as desktop screen width is bigger than height). It's still a bit f* up, but I don't think we'll change and fix the screen API before MWC :p
Flags: needinfo?(bkelly)
Keep in mind that we want that to land on master, not only on tarako.
Jan idea is worth trying.
Maybe window.matchMedia('(min-aspect-ratio: 2/1)').matches can help?
A Fake Screen will arrive in the future from the devtools guys. In the meantime it worth exploring Rik's idea in order to not change to much the logic, and the Settings API won't feel bad to be violated again until a real emulation has landed.

Comment 8

5 years ago
I'm going to try caching window.innerwidth in resize events and see if that works.  It may just move the sync reflow, though.

If that doesn't work well, then I'll try screen.width == window.innerwidth detection.

Flags: needinfo?(bkelly)


5 years ago
Assignee: nobody → bkelly
Triage to 1.3T
blocking-b2g: 1.3T? → 1.3T+


5 years ago
Depends on: 875963

Comment 10

5 years ago
Created attachment 8376048 [details] [review]
Pull request at https://github.com/mozilla-b2g/gaia/pull/16275

Rudy, this patch avoids the sync reflow by caching the window size on 'resize' event.  As far as I can tell this does not cause any other sync reflows during the event, although its been hard to capture the event in the profiler.

Here is a profile with the bug 875963 and this patch applied on 1.3t:


This patch depends on bug 875963.
Attachment #8376048 - Flags: review?(rlu)
Comment on attachment 8376048 [details] [review]
Pull request at https://github.com/mozilla-b2g/gaia/pull/16275

This patch looks good to me.


Thank you very much.
Attachment #8376048 - Flags: review?(rlu) → review+
Comment on attachment 8376048 [details] [review]
Pull request at https://github.com/mozilla-b2g/gaia/pull/16275

Hi Ben,

It seems we have to fix the unit test failure before getting this patch to land.
Do you have bandwidth to work on that?

If not, please let us know.
Thank you.
Flags: needinfo?(bkelly)
I fixed the tests in https://github.com/mozilla-b2g/gaia/pull/16399
Flags: needinfo?(bkelly)
Created attachment 8377484 [details] [review]
Patch with fixed tests
Attachment #8376048 - Attachment is obsolete: true
Attachment #8377484 - Flags: review+
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 17

5 years ago
Thank you Jan!  Jet lag hit me particularly hard coming back and I've basically been unconscious for the last three days.


5 years ago
Blocks: 972790


4 years ago
status-b2g-v1.4: --- → fixed
You need to log in before you can comment on or make changes to this bug.