927319 - Toggling the highlighted class of a keyboard key create reflow

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: View keyboard app reflows

Everytime the user hit a key the highlighted class is added to the key. This class triggers a lot of positionning changes that are creating a reflow. When this class is remove a new reflow is created.

So there is a basically 2 reflows per key touch. This hurts keyboard responsiveness. Could we come up with a solution for highlighting that does not trigger reflows, for example by having a set of highlighting elements (likely 2) that are positioned with a transform when the user hit the key, or by having those elements pre-build into the dom and hitting a key will just change their visibility and maybe a pointer-events rule?

Tim do you have some resources to work on that? Attached is the script I'm using to see keyboard reflows. You will notice that there is a lot of reflows happening as well when there is a suggestion but I will open a separate bug for that. So when debugging this one I suggest that text suggestions is disabled.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

For what it worth the code to see reflows will move into a devtool to make it easier to use (see bug 926371)

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I would redirect ni? to djf as he intend to take over keyboard UI from v1.3.

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

At the moment we do all this positioning in keyboard.css for .keyboard-key.highlighted. If we can get rid of that we don't trigger reflows anymore.

Comment 4

[David Flanagan [:djf]]

There is visual design work happening in 1.3 to simplify the appearance of the keyboard.

And we may end up rewriting a bunch of stuff for dynamic hit targets anyway.

Thanks, Vivien!

Comment 5

[Doug Sherk (:drs) (inactive)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/15336

Comment 6

[Doug Sherk (:drs) (inactive)]

This is a temporary fix. If the keyboard is rewritten I'm sure this will no longer be an issue.

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8360149 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/15336

Stealing review.

Even in the new keyboard we might make the same mistakes.

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8360149 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/15336

Hi Douglas, With this patch enabled the key disappears when pressing it, the bubble isn't shown and it's not possible to type variants anymore (by holding key); so I guess something went wrong.

The UI tests cover most of this, you can run the tests through:

$ make && gaiatest --binary=/Applications/B2G.app/Contents/MacOS/b2g-bin --profile=$HOME/repos/gaia/profile/ --testvars=testvars.json --restart ./tests/python/gaia-ui-tests/gaiatest/tests/functional/keyboard/* --app-arg=-jsconsole

That way you don't have to go through Travis all the time.

Comment 9

[Doug Sherk (:drs) (inactive)]

Strange, it worked for me. I'll look more into it, thanks!

Comment 10

[Jan Jongboom [:janjongboom] (inactive)]

Yeah, let me know. i'd really like this patch.

Comment 11

[Doug Sherk (:drs) (inactive)]

Hey Jan, I tried reproing the issues that you described and I wasn't able to on B2G desktop or on a Hamachi device. I also ran the keyboard test suite and it didn't pick up any errors.

Would you please describe more about your setup to me?

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Video of patch on B2G desktop

I'm on this commit:

commit b57e4b8b5c7baebb165db1f94bf6ba2e970c6bda
Author: DouglasSherk <[e-mail]>
Date:   Tue Jan 14 17:43:04 2014 -0800

    Bug 927323 - Fix key presses causing reflows of the entire keyboard.

Run via: make && /Applications/B2G.app/Contents/MacOS/b2g-bin -profile $PWD/profile

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

As you can see the key disappears when clicking on it...

Comment 14

[Doug Sherk (:drs) (inactive)]

Attachment: Video of patch on B2G desktop, with paint flashing enabled

I still can't repro this, and I did exactly what you did. I've attached a video of myself using it with the exact same setup. As you can see from the paint flashing, it's definitely not reflowing the entire keyboard anymore. I've also re-tested it on my Hamachi and I get the exact same functionality. I'll see if I can get someone else to test it.

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Maybe a stupid question but are you sure we're on the same commit?

Comment 16

[Doug Sherk (:drs) (inactive)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/15424

Comment 17

[Doug Sherk (:drs) (inactive)]

(In reply to Jan Jongboom [:janjongboom] from comment #15)
> Maybe a stupid question but are you sure we're on the same commit?

I redid the pull request because I botched a rebase of the old one. I last tested it with my commit rebased on top of ff30172a, which was dated Jan 7 but was merged in about half an hour ago.

Comment 18

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8361242 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/15424

r=me. Works great now, tested in B2G desktop and on device. Now only repaints the parts of the screen that are affected.

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Landed in https://github.com/mozilla-b2g/gaia/commit/815b4520d6e05eeebef2ab25598dc3e2658db117

Comment 20

[Doug Sherk (:drs) (inactive)]

Noming for 1.3.

Comment 21

[Preeti Raghunath(:Preeti)]

Please ask for approval

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

Ask for approval from who?

Comment 23

[Doug Sherk (:drs) (inactive)]

This is a fairly low-risk fix. Sanity checks on the keyboard would reveal any serious problems with it.

Comment 24

[Jason Smith [:jsmith]]

blocking- - this isn't a perf regression. We should consider this for approval.

Comment 25

[Doug Sherk (:drs) (inactive)]

(In reply to Jason Smith [:jsmith] from comment #24)
> blocking- - this isn't a perf regression. We should consider this for
> approval.

It is a perf regression. I don't know when from, but the keyboard didn't always reflow on every tap.

Comment 26

[Jan Jongboom [:janjongboom] (inactive)]

I'm pretty sure it does on 1.1

Comment 27

[Mason Chang [Inactive] [:mchang]]

We save ~5% CPU usage on a tarako w/ this patch on v1.3t.

Comment 28

[Mason Chang [Inactive] [:mchang]]

Landed in v1.3t - 920e16f7b0b9d6c5474c35d689be5a73c5216213

Comment 29

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I cannot see whole keyboard repanit even without this patch, and after this patch, we could still see 2 reflows for each keystroke, which I guess is still tracked by bug 969544.

Hi Doug,

Thanks for doing this patch, can I confirm with you the above is the same as what you see when testing this patch?

Thanks.

Comment 30

[Doug Sherk (:drs) (inactive)]

(In reply to Rudy Lu [:rudyl] from comment #29)
> I cannot see whole keyboard repanit even without this patch, and after this
> patch, we could still see 2 reflows for each keystroke, which I guess is
> still tracked by bug 969544.

The whole keyboard used to repaint on every key press. This patch took it to the latter state where, when pressed, there is a reflow of the key itself and the bubble above it. bug 969544 gets us down to no reflows on key press.