971446 - When typing quickly, bubbles don't show up

Status: RESOLVED FIXED

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

Description

[Doug Sherk (:drs) (inactive)]

If you type quickly, the little blue bubbles don't show up, which are a good quick visual indication that you're typing the right keys.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16195

Asking for review from :djf, because this might be useless with the coming keyboard rewrite.

Comment 2

[David Flanagan [:djf]]

Doug,

I just noticed this bug for the first time today, when testing a different keyboard patch on a nexus 4. Is this a new regression, do you think?  Has AZPC changed they way we handle these things?

Your patch might be useless for the refactored keyboard, but if the same bug manifests there then a similar fix would presumably work.  Have you tried the new keyboard?  (Settings->Keyboard->Enabled Layouts->Demo Keyboard/English or something like that) and then use the globe icon to switch. If you see lowercase letters on the keys (they're going away :-() you've found the right keyboard.  

Also, before I can review this, can you explain why it works?  Isn't 0s the default transition?  I guess what I'm asking for is comments in the CSS file explaining what these properties do, since they look to me like no-ops. If you don't comment them, they'll get cut by someone who doesn't realize they're doing something.

I'll all for fixing this bug, especially if it is just a matter of a simple CSS patch like this!  I'm forwarding the review to Rudy, however, because I'm completely swamped with reviews.

Also setting qa-wanted. If this reproduces on 1.3, we might want to block on it.

Comment 3

[David Flanagan [:djf]]

Comment on attachment 8374605 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16195

Rudy,

I'm passing this review request on to you because I don't understand the patch and have too many reviews in my queue already.

Comment 4

[Doug Sherk (:drs) (inactive)]

(In reply to David Flanagan [:djf] from comment #2)
> Your patch might be useless for the refactored keyboard, but if the same bug
> manifests there then a similar fix would presumably work.  Have you tried
> the new keyboard?  (Settings->Keyboard->Enabled Layouts->Demo
> Keyboard/English or something like that) and then use the globe icon to
> switch. If you see lowercase letters on the keys (they're going away :-()
> you've found the right keyboard.

Yeah, I have the same issue on the new keyboard.

> Also, before I can review this, can you explain why it works?  Isn't 0s the
> default transition?  I guess what I'm asking for is comments in the CSS file
> explaining what these properties do, since they look to me like no-ops. If
> you don't comment them, they'll get cut by someone who doesn't realize
> they're doing something.

Sure, here's how it works:

1) We start in the non-highlighted state, with these attrs:
transition: 0s opacity;
transition-delay: 0.01s;

This means that when we set opacity, we shouldn't actually set it until 0.01s from now, but when the transition actually happens, it's instant.

2) The user presses a key. This instantly adds the highlighted class, which has:
transition-delay: 0s;

This overrides the previously set 0.01s delay, which means that the bubble will show instantly with no delay.

3) The user lifts their finger off the key. This removes the highlighted class, so the transition-delay goes back to 0.01s. This makes the bubble not actually disappear until 0.01s after it has been shown.

I found that 0.01s is enough to guarantee that the bubble is shown, the keyboard still feels snappy and not laggy, and finally, if you swipe your finger over the whole keyboard, you won't see 2 bubbles at once during the transition.

> I just noticed this bug for the first time today, when testing a different
> keyboard patch on a nexus 4. Is this a new regression, do you think?  Has
> AZPC changed they way we handle these things?

> Also setting qa-wanted. If this reproduces on 1.3, we might want to block on
> it.

I'm not sure if it's a new regression or not. I know for sure this happens on 1.3 as well as master. I tried disabling "APZC for all content", which I believe disables it for the keyboard as well, and the problem persisted. So I'm reasonably sure it's not a regression due to APZC, but it might be a new regression since 1.2. Based on what you wrote, I'll nom this for 1.3+.

Comment 5

[Doug Sherk (:drs) (inactive)]

Updated the pull request to include some comments.

Comment 6

[Matthew Vaughan]

This issue does reproduce for me on the 02/12/14 1.3 build.

Device: Buri v1.3 MOZ RIL
BuildID: 20140212004003
Gaia: ce17d5eae7b1893ae4397c814b10ae598fcbdb58
Gecko: ab07e61c2eb0
Version: 28.0
Firmware Version: V1.2-device.cfg

Comment 7

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

Comment on attachment 8374605 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16195

Sorry that I haven't got the time to review this.
Will take a look tomorrow.

Thanks.

Comment 8

[Doug Sherk (:drs) (inactive)]

Attachment: Video without patch on B2G v1.3

(In reply to Matthew Vaughan from comment #6)
> This issue does reproduce for me on the 02/12/14 1.3 build.
> 
> Device: Buri v1.3 MOZ RIL
> BuildID: 20140212004003
> Gaia: ce17d5eae7b1893ae4397c814b10ae598fcbdb58
> Gecko: ab07e61c2eb0
> Version: 28.0
> Firmware Version: V1.2-device.cfg

Here is a video of me reproing it on a Hamachi running 1.3. I don't have a Buri to test on, but :djf says it occurs for him on Nexus 4, and I've seen it on Fugu and Hamachi now.

Comment 9

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

Comment on attachment 8374605 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16195

From my test, I still could reproduce this issue when typing really fast as your video did, though I am not sure this kind of fast typing is a general case.

I am testing this on buri, with the latest master branch.
Besides, testing this is kind-of harmful to my eyes, I think. :)
Maybe we should leverage eideticker and some automated test to verify this fix?

I guess this is a graphic performance issue, since I don't remember we had this kind of transition-delay back in v1.2.

Comment 10

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

Sorry, pressing Enter too soon.
I think adding a little delay before the highlight pop disappear sounds good to me, so r+.

Doug, thanks.

Comment 11

[Doug Sherk (:drs) (inactive)]

(In reply to Rudy Lu [:rudyl] from comment #9)
> From my test, I still could reproduce this issue when typing really fast as
> your video did, though I am not sure this kind of fast typing is a general
> case.
> 
> I am testing this on buri, with the latest master branch.
> Besides, testing this is kind-of harmful to my eyes, I think. :)

We can try tweaking the 0.01s to something else. Maybe a very slightly higher number would be better for you. I don't have a Buri, could you try it?

Comment 12

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

Hi Doug,

ping for https://github.com/mozilla-b2g/gaia/pull/16195#discussion_r9904001.

Comment 13

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

I found that maybe the original patch could not work, and should be refined as comment 12 indicated.
Could you confirm?

Thanks.

Comment 14

[Doug Sherk (:drs) (inactive)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17858

You were right about the change to the transition prop, so I added that in.

Comment 15

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

Comment on attachment 8400182 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17858

Doug,

Sorry for the delay to get to review this.
Could you please rebase the pr and help refer to bug 986165 for using visibility instead of opacity?

Thanks.

Comment 16

[Doug Sherk (:drs) (inactive)]

Attachment: Add a very slight delay to text bubbles and highlighting disappearing in the demo keyboard.

Updated patch. I also added the same delay to the highlighting on each key.

PR: https://github.com/mozilla-b2g/gaia/pull/17858

Comment 17

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

Comment on attachment 8405477 [details] [diff] [review]
Add a very slight delay to text bubbles and highlighting disappearing in the demo keyboard.

Doug,

Thanks for the change.
I think this is ok, however, in my test, when you press a key really fast, it still may not show the key pop up even we added 0.01 sec.

I'd like to set this for ui review to see how designer think of this patch.

Stephany,

Could you please help on the UI review part?
Thanks.

Comment 18

[Doug Sherk (:drs) (inactive)]

(In reply to Rudy Lu [:rudyl] from comment #17)
> Comment on attachment 8405477 [details] [diff] [review]
> Thanks for the change.
> I think this is ok, however, in my test, when you press a key really fast,
> it still may not show the key pop up even we added 0.01 sec.

Yes, this doesn't completely solve the problem, but I believe it's the most we can reasonably do within the UI. I was planning on investigating the IPC side of this, but I haven't gotten around to it yet.

Comment 19

[Stephany Wilkes]

Comment on attachment 8405477 [details] [diff] [review]
Add a very slight delay to text bubbles and highlighting disappearing in the demo keyboard.

Flagging Carrie for ui-review on keyboard.

Comment 20

[Stephany Wilkes]

Also flagging Rob and Patryk on the frameworks review side of keyboard.

Comment 21

[Carrie Wang [New account: [e-mail]]]

Comment on attachment 8405477 [details] [diff] [review]
Add a very slight delay to text bubbles and highlighting disappearing in the demo keyboard.

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

I've checked the patch with Rudy and we found the "flashing" effect is still very obvious. Hence, maybe we can extend the delay time (0.3/0.5s)and have a try again. Thanks!

Comment 22

[Doug Sherk (:drs) (inactive)]

(In reply to Carrie Wang [:carrie] from comment #21)
> Comment on attachment 8405477 [details] [diff] [review]
> Add a very slight delay to text bubbles disappearing in the demo keyboard.
> 
> Review of attachment 8405477 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've checked the patch with Rudy and we found the "flashing" effect is still
> very obvious. Hence, maybe we can extend the delay time (0.3/0.5s)and have a
> try again. Thanks!

Unfortunately, it's not that simple. If we increase the time to any more than 0.01s, it appears laggy when you move your finger over the keyboard. There's only so much we can do here with UI hacks. :( I'll be looking into IPC issues later.

Comment 23

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8405477 [details] [diff] [review]
Add a very slight delay to text bubbles and highlighting disappearing in the demo keyboard.

I don't think we should be holding this back on UX feedback. This is a fairly tiny and incremental fix and we can file followups to better deal with what UX wants. I don't think there's anything more we can do within the scope of this bug.

Comment 24

[Doug Sherk (:drs) (inactive)]

I filed bug 999556 and gave it the ux-most-wanted whiteboard tag to investigate this from other angles, particularly within the backend.

Comment 25

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

Comment on attachment 8405477 [details] [diff] [review]
Add a very slight delay to text bubbles and highlighting disappearing in the demo keyboard.

As we already switched the focus back to the original keyboard code base, I guess we could not land this since this is a change to demo-keyboard.

BTW, 
1. With bug 985333, I might need to have an alternative way to show the key popup without using :after pseudo-element.
2. I was thinking if it would be helpful if we should handle this by JS code,
   then we don't have to set and remove ".touched" class repeatedly when you type really fast.

Comment 26

[Doug Sherk (:drs) (inactive)]

(In reply to Rudy Lu [:rudyl] from comment #25)
> Comment on attachment 8405477 [details] [diff] [review]
> Add a very slight delay to text bubbles disappearing in the demo keyboard.
> 
> As we already switched the focus back to the original keyboard code base, I
> guess we could not land this since this is a change to demo-keyboard.

Ok, I'll port this back to the old keyboard, haha. Is there a place where this was discussed? The new keyboard being scrapped is news to me.
 
> BTW, 
> 1. With bug 985333, I might need to have an alternative way to show the key
> popup without using :after pseudo-element.

That's fine, I'll just work on top of that patch.

> 2. I was thinking if it would be helpful if we should handle this by JS code,
>    then we don't have to set and remove ".touched" class repeatedly when you
> type really fast.

Hmm, this is unclear to me. In the old keyboard, we use the ".highlighted" class to indicate that the key is being touched. In the new/scrapped keyboard, we use ".touched". I don't see a reasonable/better alternative.

Comment 27

[Doug Sherk (:drs) (inactive)]

Attachment: Add a slight delay to text bubbles and highlighting disappearing.

Ported back to the old keyboard and written on top of the patch in bug 985333.

PR: https://github.com/mozilla-b2g/gaia/pull/17858

Comment 28

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

(In reply to Doug Sherk (:drs) from comment #26)
> (In reply to Rudy Lu [:rudyl] from comment #25)
> > Comment on attachment 8405477 [details] [diff] [review]
> > Add a very slight delay to text bubbles disappearing in the demo keyboard.
> > 
> > As we already switched the focus back to the original keyboard code base, I
> > guess we could not land this since this is a change to demo-keyboard.
> 
> Ok, I'll port this back to the old keyboard, haha. Is there a place where
> this was discussed? The new keyboard being scrapped is news to me.
>

I didn't mean we will drop the new keyboard code base entirely.
It is just we have some UX/visual updates (bug 983043) and dynamic hit area that need to be addressed earlier for v2.0, and we cannot wait for the new keyboard code to be refactored. 
So, we plan to do the above 2 items onto the old keyboard and then refactor it piece by piece to make it structured like the new one.

> > BTW, 
> > 1. With bug 985333, I might need to have an alternative way to show the key
> > popup without using :after pseudo-element.
> 
> That's fine, I'll just work on top of that patch.
> 

Thanks!

> > 2. I was thinking if it would be helpful if we should handle this by JS code,
> >    then we don't have to set and remove ".touched" class repeatedly when you
> > type really fast.
> 
> Hmm, this is unclear to me. In the old keyboard, we use the ".highlighted"
> class to indicate that the key is being touched. In the new/scrapped
> keyboard, we use ".touched". I don't see a reasonable/better alternative.

What I meant is:  
(for old keyboard code)
```js
  
function highlightKey(key) {
  window.clearTimeout(this.timeout);
  key.classList.add('highlighted');
} 

function unHighlightKey(key) {  
  this.timeout = window.setTimeout(function() {   
    key.classList.remove('highlighted');
  }, 10  // 10 ms delay);
}  
```

Not tested though, does this make sense to you?

--
Will take a look at your patch later.

Comment 29

[Doug Sherk (:drs) (inactive)]

(In reply to Rudy Lu [:rudyl] from comment #28)
> I didn't mean we will drop the new keyboard code base entirely.
> It is just we have some UX/visual updates (bug 983043) and dynamic hit area
> that need to be addressed earlier for v2.0, and we cannot wait for the new
> keyboard code to be refactored. 
> So, we plan to do the above 2 items onto the old keyboard and then refactor
> it piece by piece to make it structured like the new one.

Oh, ok, I see. I'll just land both of these patches together then once you've reviewed this one.

> What I meant is:  
> (for old keyboard code)
> ```js
>   
> function highlightKey(key) {
>   window.clearTimeout(this.timeout);
>   key.classList.add('highlighted');
> } 
> 
> function unHighlightKey(key) {  
>   this.timeout = window.setTimeout(function() {   
>     key.classList.remove('highlighted');
>   }, 10  // 10 ms delay);
> }  
> ```
> 
> Not tested though, does this make sense to you?

I see. The benefits here are dubious to me and it seems like there could be bad side-effects. For example, if we add any code that checks the class on a key, then it will false-positive during the 10ms that the class should be gone but isn't. I also suspect (but I don't have evidence to prove) that this would be significantly slower. Perhaps you could explain your reasoning for preferring this way? Maybe I'm missing something.

Comment 30

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

Comment on attachment 8414553 [details] [diff] [review]
Add a slight delay to text bubbles and highlighting disappearing.

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

Sorry again for the delay to review this.

Made some comments, please address it and set review flag back again?
Thank you.

::: apps/keyboard/style/keyboard.css
@@ +125,1 @@
>  }

This kind of shorthand seems not working for multiple properties, 
Maybe we can be verbose here,
==
  transition-property: color, background-color;
  transition-duration: 0s;
  transition-timing-function: ease-in;
  transition-delay: 0.01s;

Comment 31

[Doug Sherk (:drs) (inactive)]

Attachment: Add a slight delay to text bubbles and highlighting disappearing.

I addressed the review comments kind of backhandedly (a rebase fixed it accidentally). I also realized that it makes the most sense to set the transition delay to exactly one frame's ideal time, or 0.017s, so I changed it to that.

Updated PR.

Comment 32

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

Comment on attachment 8421119 [details] [diff] [review]
Add a slight delay to text bubbles and highlighting disappearing.

The code looks good to me, with some nits to be addressed.

However, when I test the code, and swipe quickly on the keyboard, the previous popups did stay on the screens with the delay we added, and make this behavior kind-of laggy.

Comment 33

[Stephany Wilkes]

Comment on attachment 8421119 [details] [diff] [review]
Add a slight delay to text bubbles and highlighting disappearing.

Flagging Tif to provide ui-review while Carrie is out. This should definitely have a UI check before shipping. Thanks!

Comment 34

[Doug Sherk (:drs) (inactive)]

Attachment: Add a slight delay to text bubbles and highlighting disappearing.

Addressed code review comments, carried r+, carried ui-review?, updated PR.

Comment 35

[Patryk Adamczyk [:patryk] UX]

I believe Omega has been assigned to this, so he's best to offer a resolution.

Comment 36

[Patryk Adamczyk [:patryk] UX]

Please ignore Comment 35 (wrong bug).

Comment 37

[Tiffanie Shakespeare [:tif]]

Comment on attachment 8421872 [details] [diff] [review]
Add a slight delay to text bubbles and highlighting disappearing.

I can see the bubbles coming up consistently when quickly tapping on the keys (as fast as the video). This issue appears to be fixed!

Thanks

Comment 38

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8405477 [details] [diff] [review]
Add a very slight delay to text bubbles and highlighting disappearing in the demo keyboard.

I'm dusting this off to land together with the old keyboard patch.

Comment 39

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/b7fb829150e5e4b5310c71286c173eebb495bf4e

Comment 40

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

Sorry that I have to back out this change because it seems to cause bug 1010175.

The patch was backed out with this commit,
https://github.com/mozilla-b2g/gaia/commit/a078f2bdeff15c5f9c752aa272bc5540d768789f

Comment 41

[Doug Sherk (:drs) (inactive)]

Attachment: Add a slight delay to text bubbles and highlighting disappearing. Post-backout.

Sorry about that. The attached patch fixes this issue.

New PR: https://github.com/mozilla-b2g/gaia/pull/19253

Comment 42

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

Comment on attachment 8422601 [details] [diff] [review]
Add a slight delay to text bubbles and highlighting disappearing. Post-backout.

Doug,

Thanks for the quick response on this issue!
r=me.

Comment 43

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/40e14cc607bcb5c296b3fb548a61f2086b7c92b4