The keypad creates a lot of reflows when a key is entered. One of the reason is because we try to determine the best size for the text. The proposed helper used the measureText method of a canvas context to determine the best size. There is also an attached POC demonstrating how to use it in the dialer app.

While trying to find a fix for bug 917193 I've found out that an effective way of removing most reflows caused by the font sizing algorithm would be to tweak the font's starting size. We start from 18px currently which requires 6 steps with associated layouts to get us to 40px (on devices with GAIA_DEV_PIXELS_PER_PX=1). There's nothing preventing us from starting with the correct size for the device and it's such a small change that I think it's worth it.

Here an updated patch that should handle both the keypad and the call screen. I did not yet use the suggestion from Gabriele, but this actually makes a lot of sense to do it.

Here is an updated text_utils.js file that use some caching since in some of my profile I realized that calling context.font = ... is expensive.

Comment on attachment 8373016 [details] [diff] [review] foo.dialer.patch Review of attachment 8373016 [details] [diff] [review]: ----------------------------------------------------------------- Some measurements on the keypad: - For the first key tap: before ~140ms; after ~50ms - For every tap with no resize: before ~40ms; after ~5ms That looks like a good improvement :) As we discussed, I think passing an array of sizes will improve performance a bit more (no fine tuning of the steps, the app is in control) and give more design control to the app. We should use getComputedStyle when querying the existing font size. Instead of getOverflowCount, we should have something like getLongestStringWithEllipsis. And unit tests are very nice delicacies, we should have more of them.

Rik, do you have to tackle that and provide a 1.3t version?

I'm not sure to understand the exact question. (Maybe the word time is missing in the question?) Before a potential 1.3T version, we still need to finish this patch for master. I'm currently focused on DSDS work so I cannot work on this at the moment. But happy to review if someone else provides a patch.

thanks Rik. Do you know who could work on that?

Anyone :) We have Vivien's patch, my feedback, so all we need is someone to address those and add some tests. No prior knowledge of Dialer is needed.

Thomas, could your team help to verify on Tarako? thanks!

If you can confirm that the first 2 attachments help for Tarako I started to fix the review comments and write some tests for the helper. I think the getOverflowCount versus getLongestStringWithEllipsis can be fixed in a followup as it is an implementation detail and the first goal here is to reduce the latency.

Add a shared/js/text_utils.js file and some tests into apps/gallery/test/unit/text_utils_test.js since bug 841422 still prevent us to land the tests into shared/.

Let's start the review cycle with the shared/js/text_utils.js file. As I mentioned I agree that a better method than getOverflowCount would be nice, but since this code is used only for the Dialer at the moment, it sounds OK to address that in a followup.

Dialer changes using the text_utils.js helper file. I still need to address the getComputedStyle comment (I also need to check that it won't create any additional reflows), and the call screen use font-weight which is not supported yet by the helper. I have to admit that I'm tempted to just fix the keypad for now and fix the rest in a followup as it introduce a lot of changes and I would be happy to minimize the risk of regressions for Tarako.

I made a dialer patch version that targets only the keypad for now. The call screen changes and its various dependencies can be done in a followup for master. The only changes with this patch is the logic of |formatPhoneNumber|.

One of the overflow test for text_utils fails on Travis. Likely because of the different font configuration options between platforms... I guess I can modify the test in a different way so it does not fails before landing by s/129/ > 2/

Hi William, we want to check how much will the patch improve on Tarako? could you help to check it? Thanks!

(In reply to Anthony Ricaud (:rik) from comment #5) > Comment on attachment 8373016 [details] [diff] [review] > foo.dialer.patch > > Review of attachment 8373016 [details] [diff] [review]: > ----------------------------------------------------------------- > > Some measurements on the keypad: > - For the first key tap: before ~140ms; after ~50ms > - For every tap with no resize: before ~40ms; after ~5ms > That looks like a good improvement :) Hi Anthony, may I know how did you measure above number? I used the build on 3/10 with your patch, but I didn't feel the obvious improvement. So I'd like to sync the test steps with you for precise measurement. Thanks.

(In reply to Steven Yang [:styang] from comment #17) > Hi William, we want to check how much will the patch improve on Tarako? > could you help to check it? Thanks! Hi, Steven, Sorry for the late reply. Sure! I can help on this but I need Vivien to land this patch on Gaia master. Because I don't prefer to measure local build performance, it is usually inaccuracy. Thanks.

(In reply to William Hsu [:whsu] from comment #19) > Sure! I can help on this but I need Vivien to land this patch on Gaia master. We all need something. For my part I need someone to review them ;)

Comment on attachment 8388070 [details] [diff] [review] bug967440.text_utils.patch Review of attachment 8388070 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the jshint change. ::: shared/js/text_utils.js @@ +1,1 @@ > +/* jshint unused: false */ /* exported TextUtils */ is better

Comment on attachment 8388072 [details] [diff] [review] bug967440.keypad.patch Review of attachment 8388072 [details] [diff] [review]: ----------------------------------------------------------------- This looks good but keypad.js and utils.js needs some new tests.

(In reply to Danny Liang [:dliang] from comment #18) > Hi Anthony, may I know how did you measure above number? I used the build on > 3/10 with your patch, but I didn't feel the obvious improvement. So I'd like > to sync the test steps with you for precise measurement. Thanks. I should have explained when I put those measurements :( If I remember correcly, I just added some new Date() around |this.formatPhoneNumber(ellipsisSide, maxFontSize);| in keypad.js. I added some document.body.offsetLeft to ask for a reflow to make sure I was measuring the reflow in there.

Can Danny confirm again after know how rik measures the numbers? Thanks Also 1.3T? again since end users may not be able to tell the difference per earlier comments

When can we land this patch?

Hi James, do you feel the current dialpad response is still unacceptable? thanks

Comment on attachment 8394084 [details] [diff] [review] bug967440.dialer.patch Review of attachment 8394084 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/communications/dialer/test/unit/keypad_test.js @@ +414,5 @@ > MockMultiSimActionButtonSingleton._phoneNumberGetter()); > }); > }); > + > + suite('Phone Number View: Font Size', function() { We don't need all those tests. We just need to check that we call Utils.adjustTextForElement with the proper parameters. That way, you only have to include mock_utils.js earlier. ::: apps/communications/dialer/test/unit/utils_test.js @@ +68,5 @@ > }); > }); > }); > + > + suite('Font Size utilities', function() { Same here, we don't need those tests. We just need to check that we set the proper fontSize based on what a mock textutils returns. Keep the ellipsis tests (but convert to use a mock of textutils). We also need to check that the length of the final string is respecting the counter value returned.

(In reply to James Zhang from comment #26) > When can we land this patch? As soon as it has a r+. But you can take the content of attachment 8394084 [details] [diff] [review] and apply it to check if it's helping.

I'll try to test the user scenario around this. Will need to ferret out an STR also.

(In reply to Joe Cheng [:jcheng] from comment #27) > Hi James, do you feel the current dialpad response is still unacceptable? > thanks Yes, slower than 6821 android version.

1.3T+ to improve performance

Mason - I could use your help. I need to come up with an STR for this bug. What tools are available and what's a useful STR for testing these reflows? Thanks.

Hi John, You can use the reflow counter in the developer tools - https://wiki.mozilla.org/B2G/Performance/App_Performance_Validation#2._Reflows.2FRestyles As you type, you should see reflow counts in adb logcat as well, which will tell you how much time is spent in the reflow. In terms of STR, just try typing in an app and see if we're reflowing or not. Or was there a specific workload? Anthony, what STR did you use to measure in comment 5?

(In reply to Mason Chang [:mchang] from comment #34) > Anthony, what STR did you use to measure in comment 5? That should be in comment 23.

triage: should not block release. please ask for approval to land in 1.3T when ready .thanks

Vivien, I'd like to use shared TextUtils for bug 908300. Is this going to land soon? It looks like it's ready to go. Also, as an aside question: why do we create a new canvas for each fontSize+fontFamily combination? Is it faster than setting `ctx.font` for each iteration of the resize loop? Seem like you are allocating a bunch of extra memory/causing garbage collection when doing the resize (especially if you are clearing the cache after each element resize).

(In reply to Michael Henretty [:mhenretty] from comment #38) > Vivien, I'd like to use shared TextUtils for bug 908300. Is this going to > land soon? It looks like it's ready to go. > Sounds like I need to rebase this and land it. I forgot it... If you don't want to wait for me and land it that's fine too ;) > Also, as an aside question: why do we create a new canvas for each > fontSize+fontFamily combination? Is it faster than setting `ctx.font` for > each iteration of the resize loop? Seem like you are allocating a bunch of > extra memory/causing garbage collection when doing the resize (especially if > you are clearing the cache after each element resize). Yeah, setting ctx.font was very expensive on the tests I made. So I favor performance against memory, but the app author can just reset all the canvases when the app is going in the background (visibilitychange) or when it does not need it anymore.

I tried to land this patch, but it failed on travis. It seems that the reflow count for large text gives different results locally as it does in travis. In any case, this is the only test that consistently fails: https://github.com/mozilla-b2g/gaia/pull/19005/files#diff-7cfe73a7cfe695190d4eba65afd3c059R174 Vivien, do you know what is going on here?

link to failing travis build: https://travis-ci.org/mozilla-b2g/gaia/jobs/24585869#L2193

(In reply to Michael Henretty [:mhenretty] from comment #40) > Created attachment 8418469 [details] > Github PR, text utils, also moving to shared > > I tried to land this patch, but it failed on travis. It seems that the > reflow count for large text gives different results locally as it does in > travis. In any case, this is the only test that consistently fails: > > https://github.com/mozilla-b2g/gaia/pull/19005/files#diff- > 7cfe73a7cfe695190d4eba65afd3c059R174 > > Vivien, do you know what is going on here? Likely different fonts :(. Janx and some of the fonts guys are pushing to have a solution for that in bug 992210

Comment on attachment 8418469 [details] Github PR, text utils, also moving to shared In general, I think testing for an exact count of reflows makes these tests overly brittle. What if we get a reflow optimization from the platform, should that cause these tests to go red? I think testing for *less than* a certain amount of reflows is sufficient here. Vivien, what do you think?

(In reply to Michael Henretty [:mhenretty] from comment #43) > Comment on attachment 8418469 [details] > Github PR, text utils, also moving to shared > > In general, I think testing for an exact count of reflows makes these tests > overly brittle. What if we get a reflow optimization from the platform, > should that cause these tests to go red? I think testing for *less than* a > certain amount of reflows is sufficient here. Vivien, what do you think? In the theory I think I agree. On the practice, it may depends on what exactly you are trying to do. For this particular test, you're probably right, unless there is 0 reflows. For some other tests, we want to make sure that we know the exact amount of reflows. But that's probably mostly when it should be 0 or 1 reflow and that's it.

Comment on attachment 8418469 [details] Github PR, text utils, also moving to shared This particular test is flaky anyway because of cross-platform fonts.

master: https://github.com/mozilla-b2g/gaia/commit/3bfa8074c49441e59f82fe5929573a39cc6ff00a I'll try to watch TBPL to make sure I didn't burn anything there. Also, just to note, the whole point of text utils is attachment 8395665 [details] [diff] [review], which still needs to be rebased against the dialer app changes on master. So I'm leaving this bug open.

Is this going to be landed before FL?

(In reply to Wesley Huang [:wesley_huang] from comment #47) > Is this going to be landed before FL? Probably not. In bug 908300, we made some significant changes to the TextUtils class, not to mention the dialer portion of this patch has to be rebased against recent changes in the dialer. Vivien says this is still on his radar, but he probably won't get the free time to work on it before Friday.

Hi Vivien, Any concern if we defer it to 2.1? I didn't hear strong request that this must be in 2.0.

There's been some big font changes recently in bug 1007709 so this patch probably has a lot of work to finish. I'll take it for the next sprint.

this one shouldn't be a feature blocker for 2.0. I'm removing the flag and mark [priority] instead. Approval is required when patch is ready to uplift.

This was more work than anticipated. I refactored a bunch of stuff (listed in the commit message). I think the test coverage is good but let me know if there's more to add. Germán: Since you worked on this very recently, could you check that I didn't break stuff?

Comment on attachment 8463418 [details] https://github.com/mozilla-b2g/gaia/pull/22219 Generally really good. The comments on the PR that I left are mostly nits. Thanks for all the cleanup you did and tests you added here. I didn't test this since Germán is going to.

Comment on attachment 8463418 [details] https://github.com/mozilla-b2g/gaia/pull/22219 Wow! Awesome work going on here :O Thank you, guys! I have only a minor comment you may have not noticed. It is related to using 4 pixel steps in https://github.com/mozilla-b2g/gaia/pull/22219/files#diff-bc0feadf5c699af9ea9684b6b18be649R48 You will notice the effect if you try the Dialer using master and using the proposed patch. You will see that the size adaptation is not that smooth and the final (minimum) size (when using the left ellipsis) is not as precise as it could be :) Just mentioning if you want to rethink these steps ;)

You use the Dialer and you tap on many keys so the size adaptation takes place, I mean :p

Let's go with a step of 4px in this patch. We can always change it later if needed.

I've made a new commit on top of the previous one for the review fixes.

Comment on attachment 8463418 [details] https://github.com/mozilla-b2g/gaia/pull/22219 I left some comments on the PR. Mostly indentation issues.

Woot, finally landed! https://github.com/mozilla-b2g/gaia/commit/bc3e3b7f09a32c73d9ca285d44a143b51146501f