1109770 - [gaia-header] trigger the reformat only once even if several mutations happen, for the same heading

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

This happens when there is a nested markup in a gaia-header and we're changing several elements at once.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: gaia-header github PR

I still need to test properly the change.

This happens especially in the SMS app, in the new message panel, when entering recipients. One part of the issue is bug 1109801, and I also think that l10n.js has the same issue with its MutationObserver.

Comment 2

[Julien Wajsberg [:julienw]]

Bug 1109805 filed for l10n.js.

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8534524 [details] [review]
gaia-header github PR

Hey Guillaume,

I don't have a good idea to do a unit test for this. Especially I couldn't reproduce the issue using a more synthetic testcase in jsbin... (see http://jsbin.com/jujuvavura/1/edit?html,js,console) so I don't know how to reproduce in a test :/

Comment 4

[Julien Wajsberg [:julienw]]

Oh, funnily, I reproduce in this jsbin when I just load it and it autoruns, but not when I click on "run JS". I don't know why.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Julien, I don't think you can reliably use jsbin for that. The reason why you see 2 vs 1 mutations is because jsbin is adding/removing scripts. To see that, just do "console.log(mutations[i]);" in your loop and you'll see added/removed nodes.

Can you create a minimized testcase that works in a browser? The one from jsbin does not reproduce for me.

Comment 6

[Julien Wajsberg [:julienw]]

Yep I'll try; on the device and in the SMS app, I admit it happened on places I didn't expect it to happen, but didn't on the places I did expect :) So likely I don't understand how this is working ;)

Comment 7

[Guillaume Marty [:gmarty]]

Comment on attachment 8534524 [details] [review]
gaia-header github PR

Julien, try to replace the `/edit?blabla` in the URL by `/quiet` in jsbin and you'll just get the HTML and JS loaded as a regular HTML page.

Apart from that, using Set to dedupe the elements looks good to me, so f+. Waiting on tests before the review.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8534524 [details] [review]
gaia-header github PR

Hey Guillaume,

I managed to write a test that's failing with master and working with my code. Good thing: it also exercizes the whole mutation handling :)

My previous issues were indeed directly caused by jsbin. Testing using a plain HTML file showed me it was working as I expected.

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

We should consider making it a feature blocker if this improves launch time.

Comment 10

[Guillaume Marty [:gmarty]]

Comment on attachment 8534524 [details] [review]
gaia-header github PR

Looks all good to me! Thanks Julien.

Comment 11

[Julien Wajsberg [:julienw]]

gaia-header master: 0a6d93154c2cac8356f1abf9779f2fd016a8d26b

Comment 12

[Kai-Chih Hu [:khu]]

Since it's landed to master, +'ed this one.

Comment 13

[Julien Wajsberg [:julienw]]

note that it's landed to gaia-header master only, not gaia master yet :)

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: gaia github PR

Carrying over r=gmarty
Waiting for a try run.

Comment 15

[Julien Wajsberg [:julienw]]

gaia master: 8f8995f24ea9a0f5568b786f2c0668d6415de222

Comment 16

[Sherman Chen [:chens] (inactive)]

Hi Julien, do you think we can take this patch to improve sms perfromance on 2.1? I found gaia-header version has big gap in between, would that be a problem if we uplift this?

Comment 17

[Julien Wajsberg [:julienw]]

Hey Sherman,

I'd suggest to wait for bug 1089920 and then uplift all of this.

Comment 18

[Sherman Chen [:chens] (inactive)]

Cool, thank you Juilen :)