Closed Bug 927319 Opened 8 years ago Closed 8 years ago

Toggling the highlighted class of a keyboard key create reflow

Categories

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

defect
Not set
normal

Tracking

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

RESOLVED FIXED
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: vingtetun, Assigned: drs)

References

Details

(Keywords: perf)

Attachments

(4 files, 1 obsolete file)

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.
Flags: needinfo?(timdream)
For what it worth the code to see reflows will move into a devtool to make it easier to use (see bug 926371)
Keywords: perf
I would redirect ni? to djf as he intend to take over keyboard UI from v1.3.
Flags: needinfo?(timdream) → needinfo?(dflanagan)
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.
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!
Assignee: nobody → bugzilla
Attachment #8360149 - Flags: review?(dflanagan)
This is a temporary fix. If the keyboard is rewritten I'm sure this will no longer be an issue.
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.
Attachment #8360149 - Flags: review?(dflanagan) → review?(janjongboom)
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.
Attachment #8360149 - Flags: review?(janjongboom) → review-
Strange, it worked for me. I'll look more into it, thanks!
Yeah, let me know. i'd really like this patch.
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?
I'm on this commit:

commit b57e4b8b5c7baebb165db1f94bf6ba2e970c6bda
Author: DouglasSherk <github@sherk.me>
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
As you can see the key disappears when clicking on it...
Flags: needinfo?(bugzilla)
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.
Flags: needinfo?(bugzilla)
Maybe a stupid question but are you sure we're on the same commit?
Flags: needinfo?(bugzilla)
Attachment #8360149 - Attachment is obsolete: true
Flags: needinfo?(bugzilla)
(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.
Attachment #8361242 - Flags: review?(janjongboom)
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.
Attachment #8361242 - Flags: review?(janjongboom) → review+
Landed in https://github.com/mozilla-b2g/gaia/commit/815b4520d6e05eeebef2ab25598dc3e2658db117
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Flags: needinfo?(dflanagan)
Noming for 1.3.
blocking-b2g: --- → 1.3?
Please ask for approval
Flags: needinfo?(janjongboom)
Ask for approval from who?
Flags: needinfo?(janjongboom) → needinfo?(praghunath)
This is a fairly low-risk fix. Sanity checks on the keyboard would reveal any serious problems with it.
blocking- - this isn't a perf regression. We should consider this for approval.
blocking-b2g: 1.3? → -
(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.
I'm pretty sure it does on 1.1
blocking-b2g: - → 1.3T+
We save ~5% CPU usage on a tarako w/ this patch on v1.3t.
Blocks: 971446
Landed in v1.3t - 920e16f7b0b9d6c5474c35d689be5a73c5216213
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.
Flags: needinfo?(drs+bugzilla)
(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.
Flags: needinfo?(drs+bugzilla)
Flags: needinfo?(praghunath)
You need to log in before you can comment on or make changes to this bug.