816637 - [SMS] [Keyboard performance] We need to remove the 'spinner' in 'loading' in order to avoid infinite rendering.

Status: RESOLVED FIXED

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

Description

[Borja Salguero [:borjasalguero]]

This was not included in https://github.com/mozilla-b2g/gaia/pull/6720, and it's one of the key points for getting keyboard working properly.

Comment 1

[Borja Salguero [:borjasalguero]]

Use  'Show frames per second' and 'Flash repainted area' for showing the problem.

Comment 2

[Borja Salguero [:borjasalguero]]

https://github.com/mozilla-b2g/gaia/pull/6720#issuecomment-10862569

Comment 3

[Borja Salguero [:borjasalguero]]

Attachment: PR

Comment 4

[Borja Salguero [:borjasalguero]]

Comment on attachment 686727 [details]
PR

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 5

[Borja Salguero [:borjasalguero]]

This was the path that I was trying to send to [:ttaubert] https://bugzilla.mozilla.org/show_bug.cgi?id=816538#c5 , so please as a 'not-written rule'/convention take into account the review of the 'App owner' before merging directly.

Comment 6

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

(In reply to Borja Salguero [:borjasalguero] from comment #5)
> This was the path that I was trying to send to [:ttaubert]
> https://bugzilla.mozilla.org/show_bug.cgi?id=816538#c5 , so please as a
> 'not-written rule'/convention take into account the review of the 'App
> owner' before merging directly.

I do. But it has been said multiple times to *not* use position: relative and to make divs of the size of the screen. There is even a video from Andreas about it.

Futhermore -moz-element has some known issues with repainting that make the keyboard slow and NO it is not working correctly in Contacts as you mentioned in the comment. Contacts repaints all the app on every keystroke and this will go away once moz-element will be removed.

Comment 7

[Borja Salguero [:borjasalguero]]

[:vingtetun] As I've said https://github.com/mozilla-b2g/gaia/pull/6720#issuecomment-10862569 Im not against the change of 'moz-element', I've tested the code and the transition is smooth and the code simple. In fact I was sending a patch to [:ttaubert] branch for the 'spinner' issue, but suddenly the PR was merged despite of https://bugzilla.mozilla.org/show_bug.cgi?id=816538#c5 (I was only asking the reasons for removing it, and I was testing the fix for the spinner). 

So please, before merging, and above everything when the owner of the app is requesting few minutes for reviewing, hold on until the review was done. Taking into consideration the people who was working in each app as reviewers I think is needed in order to keep the stability and consistency of Firefox OS.

I've added the attachment for fixing the 'spinner' issue, because was the reason of CPU consumption and Keyboard performance.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

The approach is good and does fix the leftovers I mentioned in bug 816538. Very nice. Typing reaches the same speed as in the calendar app. I added my thoughts to the PR.

Comment 9

[Borja Salguero [:borjasalguero]]

[:ttaubert] All your suggestions included. Thanks!

Comment 10

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

(In reply to Borja Salguero [:borjasalguero] from comment #7)
> [:vingtetun] As I've said
> https://github.com/mozilla-b2g/gaia/pull/6720#issuecomment-10862569 Im not
> against the change of 'moz-element', I've tested the code and the transition
> is smooth and the code simple. In fact I was sending a patch to [:ttaubert]
> branch for the 'spinner' issue, but suddenly the PR was merged despite of
> https://bugzilla.mozilla.org/show_bug.cgi?id=816538#c5 (I was only asking
> the reasons for removing it, and I was testing the fix for the spinner). 

His bug was about removing moz-element. What is the relationship with the spinner? It's good to have a second bug for it.

> 
> So please, before merging, and above everything when the owner of the app is
> requesting few minutes for reviewing, hold on until the review was done.

Review is not about adding more stuff on the pile. Small bugs are easier to scope and review and less regression prone.

> I've added the attachment for fixing the 'spinner' issue, because was the
> reason of CPU consumption and Keyboard performance.

It is a *part* of the reason. And that's good that it's been finally fixed!

Comment 11

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

Comment on attachment 686727 [details]
PR

This is part of the underlying work for having a fast keyboard. Small changes that change the world. a=me.

Comment 12

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

https://github.com/mozilla-b2g/gaia/commit/9058317544abe7b4759c6535e1471c65ed9ef71f