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

Status: RESOLVED FIXED

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

Description

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

The original bug was used to fix a much bigger reflow. This one remove completely the reflows at the cost of a small UI changes - but basically the user can't notice really since is finger is on the key...

+++ This bug was initially created as a clone of Bug #927319 +++

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)]

Attachment: bug969544.patch

I understand that this is not the best solution for UI/UX but since the key is under the finger I think it worth the small change actually.

I'm also inclined to uplift that to 1.3.

Comment 2

[David Flanagan [:djf]]

Comment on attachment 8372486 [details] [diff] [review]
bug969544.patch

Review of attachment 8372486 [details] [diff] [review]:
-----------------------------------------------------------------

I think the visual design people will be unhappy with the highlighting changes caused by this patch. Below I propose some additional CSS changes that mitigate those changes.  If you can apply my additional CSS and if they do not cause reflow, then r+ from me.  You might want to check with Casey before landing, however.  Or at least leave a comment here explaining what user-perceived performance improvement is and justifying the slightly degraded visuals as the price to be paid for enhanced responsiveness.

Can we measure the increase in performance?  If so, then I support uplifting this patch to 1.3.

Also, if you could teach me how to detect reflows that do not cause repaints, I can try to ensure that I don't duplicate this reflow bug in the refactored keyboard I'm working on.

::: apps/keyboard/style/keyboard.css
@@ -142,5 @@
>  .keyboard-key.highlighted > .visual-wrapper > span {
>    color: #00caf2;
>    background-color: #4a5255;
> -  margin: -0.4rem 0 -0.4rem -0.1rem;
> -  padding: 0.4rem 0 0 0.1rem;

Removing these lines changes the part of the key that is highlighted when you press it. Most of the time the user won't notice, but the highlight does look bad when you press the space bar, for example.

Adding this class seems to get back much closer to the original visuals: 

.keyboard-key.highlighted {
  background-color: #4a5255;
}

It still leaves the left border of the highlighted area imperfect, but if performance is noticeably improved, that is more important than having the visuals just right.

Comment 3

[Doug Sherk (:drs) (inactive)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #1)
> Created attachment 8372486 [details] [diff] [review]
> bug969544.patch
> 
> I understand that this is not the best solution for UI/UX but since the key
> is under the finger I think it worth the small change actually.
> 
> I'm also inclined to uplift that to 1.3.

This patch doesn't seem to fix the issue for me. This puzzles me because you showed it to me a few days ago in action and it seemed to work then :)

Comment 4

[Doug Sherk (:drs) (inactive)]

(In reply to David Flanagan [:djf] from comment #2)
> Also, if you could teach me how to detect reflows that do not cause
> repaints, I can try to ensure that I don't duplicate this reflow bug in the
> refactored keyboard I'm working on.

In the case of the keyboard, reflow and painting are basically synonymous, so paint flashing will give you a pretty good idea as to what is happening. Its weakness is that it won't tell you when the same region is being invalidated multiple times within an imperceptible amount of time.

:janx was working on a developer HUD in bug 968220 which will show you the number of reflows that have happened in each visible frame. It's on inbound now and will be in m-c soon.

Comment 5

[Doug Sherk (:drs) (inactive)]

:janx informs me that the Gaia patch for bug 968220 hasn't actually landed yet, but will soon once they re-open the tree.

Comment 6

[Jan Keromnes [:janx]]

To see the on-screen reflow counter, you need to enable `Settings > Developer > Show devtools overlay`. However, until bug #962577 is fixed, this only works for non-certified apps (e.g. Marketplace). To make it work for certified apps (e.g. the Keyboard), you can set the `devtools.debugger.forbid-certified-apps` pref to false.

Bug #968220 that Doug mentioned simply replaces the "Show devtools overlay" checkbox with a "Developer HUD" sub-menu where you can choose exactly which metrics you want to track.

Comment 7

[Doug Sherk (:drs) (inactive)]

Thanks, Jan.

I disabled all "highlighted" classes in keyboard.css and I'm still seeing the same reflow issue. When I did this, I noticed that, if you put down 2 fingers on different spots on the keyboard, then move the second one around, it doesn't seem to reflow for them. I'm not sure if that's useful information at all.

I did notice that sliding the second finger around in the above case enters every key I slide over as if I tapped it. I think that should be filed as a separate bug.

Comment 8

[Doug Sherk (:drs) (inactive)]

Attachment: Pressing hard, probably a hardware problem

Here are a couple of videos describing what I mean. This one is when I press slightly too hard. I don't think this is actually signficant, and it's probably a hardware problem.

Comment 9

[Doug Sherk (:drs) (inactive)]

Attachment: Pressing softer, not getting reflows while swiping

This one is more interesting. If I let go of my first finger, I get a reflow in that spot, but while dragging around my second finger, I don't. It's clearly registering my touch as you can see it add that letter to the search box when I lift it up. Note that in both of these videos, I have all highlighting styles disabled.

The only thing I could possibly think of that could be causing the reflows is that maybe somewhere in JS we're querying dimensions from the keys. Or maybe I'm missing a style somewhere that isn't in keyboard.css.

Comment 10

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

(In reply to Doug Sherk (:drs) from comment #9)
> Created attachment 8375002 [details]
> Pressing softer, not getting reflows while swiping
> 
> This one is more interesting. If I let go of my first finger, I get a reflow
> in that spot, but while dragging around my second finger, I don't. It's
> clearly registering my touch as you can see it add that letter to the search
> box when I lift it up. Note that in both of these videos, I have all
> highlighting styles disabled.
> 
> The only thing I could possibly think of that could be causing the reflows
> is that maybe somewhere in JS we're querying dimensions from the keys. Or
> maybe I'm missing a style somewhere that isn't in keyboard.css.

I assume you're running a keyboard OOP ? Just asking in order to be sure that you don't ends up in a reflow starts from BrowserElementPanning.js at http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#222

Comment 11

[Doug Sherk (:drs) (inactive)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #10)
> I assume you're running a keyboard OOP ? Just asking in order to be sure
> that you don't ends up in a reflow starts from BrowserElementPanning.js at
> http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/
> BrowserElementPanning.js#222

That might explain it. I was having trouble getting it to run OOP recently for some reason. "APZC for all content" is on, as is hardware composer. Do you know how to force it OOP?

Comment 12

[[:fabrice] Fabrice Desré]

(In reply to Doug Sherk (:drs) from comment #11)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #10)
> > I assume you're running a keyboard OOP ? Just asking in order to be sure
> > that you don't ends up in a reflow starts from BrowserElementPanning.js at
> > http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/
> > BrowserElementPanning.js#222
> 
> That might explain it. I was having trouble getting it to run OOP recently
> for some reason. "APZC for all content" is on, as is hardware composer. Do
> you know how to force it OOP?

build/settings.js:179:   'keyboard.3rd-party-app.enabled': false,

Comment 13

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ to benefit tarako keyboard performance

Comment 14

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

https://github.com/mozilla-b2g/gaia/commit/6e9b110a7a0c8d99bcfd5ab05b83da6dc44a292a

Comment 15

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

I opened bug 973050 as a followup to replicate the old style but without introducing new reflows.

Comment 16

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

Uplifted to v1.3t for performance improvement.
https://github.com/mozilla-b2g/gaia/commit/4ab0bdb237a9b94f32eb7011bd6c3f7244481769