Closed
Bug 1239042
Opened 9 years ago
Closed 9 years ago
Add minimum display time to Syncing… and spinner
Categories
(Firefox :: Sync, defect, P2)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: rfeeley, Assigned: lina)
References
Details
Attachments
(2 files)
Very often the user will click Sync Now and will only see Syncing… appear for a split second.
We should add a minimum display time of 1600ms and four frames of animation to the ellipsis character.
Syncing
Syncing.
Syncing..
Syncing… (or if that looks odd, Syncing...)
The animation should run twice. Each step should taking 200ms for a total of 1600ms.
Updated•9 years ago
|
Flags: firefox-backlog+
Priority: -- → P2
Comment 1•9 years ago
|
||
Hijacking this bug for minimum display time and spinning out the animation to a separate bug.
Summary: Add minimum display time and animation to Syncing… → Add minimum display time to Syncing… and spinner
Reporter | ||
Comment 2•9 years ago
|
||
Similar treatment done by Mac App Store on web pages: https://search.itunes.apple.com/WebObjects/MZContentLink.woa/wa/link?path=mac%2fPDFScanner
Updated•9 years ago
|
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/32549/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32549/
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
This is a quick way to do it. (Hard-coded constants FTW!) It'll affect the "Sync Now" button in the hamburger menu, since it also observes the `sync-status` element.
Another approach is to decouple the observer in "Synced Tabs". That seems better, but we need to be extra careful that we keep the state consistent with the "real" sync status. WDYT?
Attachment #8712459 -
Flags: feedback?(markh)
Comment 5•9 years ago
|
||
https://reviewboard.mozilla.org/r/32549/#review29525
I've a niggling concern about the delays cascading here - part of me wants to say it's not going to be a problem in practice, but another part thinks it might be - eg, we sync on each bookmark change, so someone managing their bookmarks might implicitly trigger many bookmark syncs, which if they complete in less than 1.6 seconds might have the delay stack up.
I could be wrong, but if I'm not, it might be worth replacing that \_numActiveSyncs complexity with some complexity to kill an existing timer when a new one is started. What do you think?
For diagnostics, it might also be worth writing .debug entries to the log as we start a new timer and as each timer is resolved - these entries will appear in sync-logs.
(also not sure how to flag f+ in reviewboard - "ship it" doesn't sound correct for feedback.)
::: browser/base/content/browser-syncui.js:183
(Diff revision 1)
> if (++this._numActiveSyncTasks == 1) {
I'm inclined to think we should kill the \_numActiveSyncTasks variable here too - it was used to support the now-defunct readinglist.
::: browser/base/content/browser-syncui.js:185
(Diff revision 1)
> + new Promise(resolve => setTimeout(resolve, 1600))
If I'm reading this correctly, a user mashing the sync button many times will cause the delay to get quite long - I'm not sure if I think that's a problem yet though :)
Comment 6•9 years ago
|
||
(In reply to Kit Cambridge [:kitcambridge] from comment #4)
> Another approach is to decouple the observer in "Synced Tabs". That seems
> better, but we need to be extra careful that we keep the state consistent
> with the "real" sync status. WDYT?
I think your approach is good and that the hamburger menu *should* see this increased time before the spinner stops (ie, all sync indicators should remain consistent)
Updated•9 years ago
|
Attachment #8712459 -
Flags: feedback?(markh) → feedback+
Comment 7•9 years ago
|
||
I think we can make this simpler still - the request is more to say "the spinner should spin for a minimum time" rather than "the spinner should spin for some time after sync completes".
IOW, if the spinner is still spinning (due to this delay) when a new sync starts, there's no reason to *extend* the time further. I *think* that makes things easier.
Assignee | ||
Updated•9 years ago
|
Attachment #8712459 -
Attachment description: MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. → MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
Attachment #8712459 -
Flags: feedback+ → review?(markh)
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32549/diff/1-2/
Comment 9•9 years ago
|
||
Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
https://reviewboard.mozilla.org/r/32549/#review32789
LGTM, thanks!
Attachment #8712459 -
Flags: review?(markh) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 10•9 years ago
|
||
It would help if I updated `browser/base/content/test/general/browser_syncui.js`. ;-)
Keywords: checkin-needed
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32549/diff/2-3/
Attachment #8712459 -
Attachment description: MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh → MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r=markh
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
Keywords: checkin-needed
Reporter | ||
Comment 14•9 years ago
|
||
(In reply to Kit Cambridge [:kitcambridge] from comment #12)
> There we go:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5b42ff33cb1
I've been using it all day! Great work!
Comment 15•9 years ago
|
||
Backed out for frequent Windows intermittent browser_syncui.js failures. Seemed to hit mostly on PGO builds, but did occasionally strike on regular opt as well. WinXP was the most regular offender, but Win7 also got in the act.
https://hg.mozilla.org/integration/fx-team/rev/ccfb45a887bf
https://treeherder.mozilla.org/logviewer.html#?job_id=7538012&repo=fx-team
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32549/diff/3-4/
Attachment #8712459 -
Attachment description: MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r=markh → MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
As we discussed, I reverted the test changes. That should squelch the FxA and Sync UI failures for PGO builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=499e422a6c58
Mark, could you take another look, please?
Attachment #8712459 -
Flags: review+ → review?(markh)
Comment 18•9 years ago
|
||
Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
https://reviewboard.mozilla.org/r/32549/#review45063
I thought that the earlier patch struck issues with the browser_syncui test? I know we agree to skip specific testing of this animation, but I thought there may still be test changes needed for existing tests?
::: browser/base/content/browser-syncui.js:160
(Diff revision 4)
> this._promiseUpdateUI().catch(err => {
> this.log.error("updateUI failed", err);
> })
> },
>
> + _hideBroadcaster(id, hidden) {
I'm that keen on this change TBH - it means a typo in an element ID will not be picked up, and I see no reason a valid ID will ever be null here (apart from the "browser is tearing down" case, which is why the gBrowser checks exist). Did you strike an actual problem that requires this?
::: browser/base/content/browser-syncui.js:206
(Diff revision 4)
> - if (++this._numActiveSyncTasks == 1) {
> +
> + clearTimeout(this._syncAnimationTimer);
> + this._syncStartTime = Date.now();
> +
> - let broadcaster = document.getElementById("sync-status");
> + let broadcaster = document.getElementById("sync-status");
> + if (broadcaster) {
ditto here and below - we've checked gBrowser, so I'm interested to know when broadcaster may be null? And if it *is* null, I doubt that falling through to updateUI is the right thing to do.
Attachment #8712459 -
Flags: review?(markh)
Assignee | ||
Comment 19•9 years ago
|
||
Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32549/diff/4-5/
Attachment #8712459 -
Flags: review?(markh)
Assignee | ||
Comment 20•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=72a1212cda0b
FWIW, the `TypeError: document.getElementById(...) is null` issue is still there, if you search in the log for a run that failed for unrelated reasons: https://treeherder.mozilla.org/logviewer.html#?job_id=19894786&repo=try I don't know if it fails on the green runs, too, since Try silences logs when the test suite passes.
I wish this were easier to investigate. :-( I can't reproduce it locally on my Windows box, even with PGO. I've yet to set up or try http://rr-project.org/ on my Linux VM; maybe it could help. But that seems like a yak for another day.
Comment 21•9 years ago
|
||
(In reply to Kit Cambridge [:kitcambridge] from comment #20)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=72a1212cda0b
>
> FWIW, the `TypeError: document.getElementById(...) is null` issue is still
> there, if you search in the log for a run that failed for unrelated reasons:
> https://treeherder.mozilla.org/logviewer.html#?job_id=19894786&repo=try I
> don't know if it fails on the green runs, too, since Try silences logs when
> the test suite passes.
The implication of that though is that these messages aren't actually causing any problems, just log noise in tests. If that's correct then I think we should just leave it alone and have those messages keep appearing - hopefully that will still cause someone to prioritize working out what's going on here. If that's *not* correct (ie, that your patch is somehow making these existing messages cause failures) then I do think we need to dig a little deeper into this.
Assignee | ||
Comment 22•9 years ago
|
||
It has shown up on other runs before; the test that I added to the original patch just exposed it: https://treeherder.mozilla.org/logviewer.html#?job_id=26644359&repo=mozilla-inbound, https://treeherder.mozilla.org/logviewer.html#?job_id=26635532&repo=mozilla-inbound, https://treeherder.mozilla.org/logviewer.html#?job_id=26619189&repo=mozilla-inbound
Now that I've removed the test, of course, there's no visible problem. :-) But I'm stumped why it sometimes fails to find the broadcasters, even when there's a gBrowser.
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32549/diff/5-6/
Assignee | ||
Comment 24•9 years ago
|
||
Try push without test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61a3dcd82a3b
With test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7f62880fde2
Looks like that wasn't the root cause. :-(
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8712459 [details]
MozReview Request: Bug 1239042 - Show sync status for a minimum of 1.6s. r?markh
Mark rubber-stamped this over IRC. :-)
Attachment #8712459 -
Flags: review?(markh) → review+
Comment 27•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in
before you can comment on or make changes to this bug.
Description
•