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

RESOLVED FIXED in 2.2 S3 (9jan)

Status

Firefox OS
Gaia::Components
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

unspecified
2.2 S3 (9jan)
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sms-sprint-2.2S3])

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
This happens when there is a nested markup in a gaia-header and we're changing several elements at once.
(Assignee)

Comment 1

3 years ago
Created attachment 8534524 [details] [review]
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.
Assignee: nobody → felash
(Assignee)

Updated

3 years ago
Summary: [gaia-header] trigger the reformat only once even if several mutations happen → [gaia-header] trigger the reformat only once even if several mutations happen, for the same heading
(Assignee)

Comment 2

3 years ago
Bug 1109805 filed for l10n.js.
(Assignee)

Comment 3

3 years ago
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 :/
Attachment #8534524 - Flags: feedback?(gmarty)
(Assignee)

Comment 4

3 years ago
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.
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.
(Assignee)

Comment 6

3 years ago
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 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.
Attachment #8534524 - Flags: feedback?(gmarty) → feedback+
(Assignee)

Comment 8

3 years ago
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.
Attachment #8534524 - Flags: review?(gmarty)
We should consider making it a feature blocker if this improves launch time.
feature-b2g: --- → 2.2?
Comment on attachment 8534524 [details] [review]
gaia-header github PR

Looks all good to me! Thanks Julien.
Attachment #8534524 - Flags: review?(gmarty) → review+
(Assignee)

Comment 11

3 years ago
gaia-header master: 0a6d93154c2cac8356f1abf9779f2fd016a8d26b

Comment 12

3 years ago
Since it's landed to master, +'ed this one.
feature-b2g: 2.2? → 2.2+
(Assignee)

Comment 13

3 years ago
note that it's landed to gaia-header master only, not gaia master yet :)
(Assignee)

Comment 14

3 years ago
Created attachment 8543930 [details] [review]
gaia github PR

Carrying over r=gmarty
Waiting for a try run.
Attachment #8543930 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8534524 - Attachment description: github PR → gaia-header github PR
(Assignee)

Comment 15

3 years ago
gaia master: 8f8995f24ea9a0f5568b786f2c0668d6415de222
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Whiteboard: [sms-sprint-2.2S3]
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?
Flags: needinfo?(felash)
(Assignee)

Comment 17

3 years ago
Hey Sherman,

I'd suggest to wait for bug 1089920 and then uplift all of this.
Flags: needinfo?(felash)
Cool, thank you Juilen :)

Updated

3 years ago
feature-b2g: 2.2+ → ---
Target Milestone: --- → 2.2 S3 (9jan)
You need to log in before you can comment on or make changes to this bug.