Implement new download start/finish animations under MOZ_PHOTON_ANIMATIONS flag. This work was a part of bug 1352065 but is being broken out into its own sub-task to facilitate incremental development and easier review of smaller steps towards the whole.

Paolo, Attachment #8886725 [details] adds the photon-themed notification animations. We use the #downloads-notification-anchor for the download-start notification only - the download-finish just animates the icon in-place (which we can do since we split it from the progressbar during the work-week) I went over this with Amy and addressed most of her comments in this patch, but we'll still want a final OK from her. There's 2 major issues remaining, which came out of her UI review: 1. start animation appears to run fast for a small/fast download. If the finish event comes in before the start animation is complete, the animation gets truncated, which looks glitchy. 2. If another download is already in progress, we should not dip the icon (downloadsIndicatorStartDip animation) when the new download starts - only show the downloadsIndicatorNotificationStart These probably need some discussion as they might a challenge to implement using the current approach of simply updating the notification attribute value when an event comes in.

:jaws pointed out a issue that I've been able to reproduce. If you bump up the font-size, as the toolbar uses -moz-box-align:stretch, the toolbar height is variable. The "magic offset" I've got on indicator.inc.css line #352 min-height: 66px; gives the wrong result if font-size is larger/smaller than default. I'm watching bug #1355922 to see if the solution there is applicable here.

Thanks for the patch! I've taken a look and it seems to me this will be an easy review. I have a few other things going on at the moment so it may have to wait a couple more days, but I don't expect any major issues. For the cases mentioned in comment 2, it may be fine to handle them as separate bugs if necessary.

Paolo: I've updated the patch/review request to fix a couple of linting/syntax errors pointed out in the last try push. I also addressed the issue of variable toolbar height - by vertically centering the animation frames. This meant I had to adjust the height of the svg frames to make the icon vertically centered, so we have to live with extra whitespace there. These changes actually remove the need for the _getPositioningCoordsForRects helper in indicator.js, as we no longer need the ability to pass which edge we want to align to. I've left the helper there as a useful refactoring, but can also remove it if you prefer - which would essentially revert all changes to indicator.js

(In reply to Sam Foster [:sfoster] from comment #7) > I also addressed the issue of variable toolbar height - by vertically > centering the animation frames. This meant I had to adjust the height of the > svg frames to make the icon vertically centered, so we have to live with > extra whitespace there. These changes actually remove the need for the > _getPositioningCoordsForRects helper in indicator.js, as we no longer need > the ability to pass which edge we want to align to. Ah, that's much better, thanks! > I've left the helper > there as a useful refactoring, but can also remove it if you prefer - which > would essentially revert all changes to indicator.js I was actually going to recommend removing the helper anyways, since most of its functionality is unused.

Comment on attachment 8886725 [details] Bug 1376519 - Photon download notifications. https://reviewboard.mozilla.org/r/157526/#review164542 Looks good! I'll review the version with the helper method removed. ::: browser/components/downloads/content/indicator.js:385 (Diff revision 4) > - let widthDiff = anchorRect.width - notifierRect.width; > - let translateX = (leftDiff + .5 * widthDiff) + "px"; > - let translateY = (topDiff + .5 * heightDiff) + "px"; > - notifier.style.transform = "translate(" + translateX + ", " + translateY + ")"; > } > notifier.setAttribute("notification", aType); Looks like the Photon and type check should wrap the entire block that includes "let notifier = " and sets the attribute. ::: browser/themes/shared/downloads/indicator.inc.css:252 (Diff revision 4) > + min-height: 98px; > + max-height: 98px; If there is a reason why you have to use min-height and max-height rather than "height", like overriding other properties, please specify it in a comment. ::: browser/themes/shared/downloads/indicator.inc.css:297 (Diff revision 4) > +%endif > > +%ifndef MOZ_PHOTON_ANIMATIONS nit: looks like you can simplify this by removing these two lines. ::: browser/themes/shared/downloads/indicator.inc.css:307 (Diff revision 4) > 20% { opacity: .65; animation-timing-function: ease-in; } > to { opacity: 0; transform: scale(8); } > } > +%endif > > +%ifdef MOZ_PHOTON_ANIMATIONS This looks like a leftover. Did you test the non-Photon version as well? This might have broken the start animation, unless the preprocessor ignored the unmatched directive.

(In reply to :Paolo Amadini from comment #10) > ::: browser/themes/shared/downloads/indicator.inc.css:252 > (Diff revision 4) > > + min-height: 98px; > > + max-height: 98px; > > If there is a reason why you have to use min-height and max-height rather > than "height", like overriding other properties, please specify it in a > comment. This was necessary in bug 1355924, where I added this comment: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/themes/shared/toolbarbutton-icons.inc.css#89-92 You could just add a comment that links to bug 1379332.

Comment on attachment 8886725 [details] Bug 1376519 - Photon download notifications. https://reviewboard.mozilla.org/r/157526/#review164542 > Looks like the Photon and type check should wrap the entire block that includes "let notifier = " and sets the attribute. I moved the setAttribute for the notifier inside the if block, but kept the let notifier statement outside the if () {} as we also reference it in the setTimeout. > This looks like a leftover. Did you test the non-Photon version as well? This might have broken the start animation, unless the preprocessor ignored the unmatched directive. I did double check the non-Photon version (clobber build with MOZ_PHOTON_THEME= MOZ_PHOTON_ANIMATIONS= mach build) after fixing this and all looks good.

Comment on attachment 8886725 [details] Bug 1376519 - Photon download notifications. https://reviewboard.mozilla.org/r/157526/#review165064 One more issue then we should be good! ::: browser/components/downloads/content/indicator.js:346 (Diff revision 5) > + // Note: no notifier animation for download finished in Photon > + let notifier = this.notifier; > + if (notifier.style.transform == "" && > + !(AppConstants.MOZ_PHOTON_ANIMATIONS && aType == "finish")) { > - let notifier = this.notifier; > + let notifier = this.notifier; There are two separate conditions here. The first one is: !(AppConstants.MOZ_PHOTON_ANIMATIONS && aType == "finish") This determines whether we should set the attribute to show the notification. If this is true, then we should check this condition: notifier.style.transform == "" This determines whether we have already computed the coordinates, and is just an optimization to avoid repeating the calculation. So, this shouldn't wrap the "setAttribute" call. One thing that just occurred to me is that this calculation depends on the height of the anchor. Does the height of the anchor change if the toolbar height changes, now that there is a compact mode and a tablet mode? If the height is the same we can still keep the second condition, otherwise the simplest thing we can do is to remove the check and recalculate every time. Either way, I suggest adding an explanatory comment. There is also a duplicate let statement to fix.

(In reply to Sam Foster [:sfoster] from comment #2) > 1. start animation appears to run fast for a small/fast download. If the > finish event comes in before the start animation is complete, the animation > gets truncated, which looks glitchy. While this was an existing issue, now the finish animation is less visible than the start animation, so this may become a bigger issue for fast downloads that are automatically saved on click, giving the impression that nothing happened. We should definitely have a bug on file. Actually, both animations are much less visible than the old ones if you're looking away from the button, but it's also true that the button will be shown only when there are new downloads in the session, which at least mitigates the issue of informing the user on where to find the new download. We can solve the issue with the finish animation in two ways, the first is by giving priority to the start animation, and the second is by separating the attributes so the animations can happen at the same time. The latter will be easier to do in two weeks when we can remove the non-Photon code path entirely. > 2. If another download is already in progress, we should not dip the icon > (downloadsIndicatorStartDip animation) when the new download starts - only > show the downloadsIndicatorNotificationStart This is a really subtle difference. Is this wanted because the progress bar will be thicker and the arrow might overlap it?

Comment on attachment 8886725 [details] Bug 1376519 - Photon download notifications. https://reviewboard.mozilla.org/r/157526/#review165064 > There are two separate conditions here. The first one is: > > !(AppConstants.MOZ_PHOTON_ANIMATIONS && aType == "finish") > > This determines whether we should set the attribute to show the notification. If this is true, then we should check this condition: > > notifier.style.transform == "" > > This determines whether we have already computed the coordinates, and is just an optimization to avoid repeating the calculation. So, this shouldn't wrap the "setAttribute" call. > > One thing that just occurred to me is that this calculation depends on the height of the anchor. Does the height of the anchor change if the toolbar height changes, now that there is a compact mode and a tablet mode? If the height is the same we can still keep the second condition, otherwise the simplest thing we can do is to remove the check and recalculate every time. Either way, I suggest adding an explanatory comment. > > There is also a duplicate let statement to fix. That's a good point about the anchor height. The anchor in this case is a toolbar button which does indeed change height if the user switches to compact mode etc. So I remove that condition which simplifies the logic here quite a bit.

Comment on attachment 8886725 [details] Bug 1376519 - Photon download notifications. https://reviewboard.mozilla.org/r/157526/#review165450 Thanks!

Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c6bbe349d654 Photon download notifications. r=Paolo

Backed out on suspicion of letting mochitest test_bug574663.html and others fail: https://hg.mozilla.org/integration/autoland/rev/c1c986b9d8ff08525f42435677ee2f3652fc937d Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c6bbe349d654f15bf11baaa1f58638be640fa138&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=117058773&repo=autoland 17:50:12 INFO - 2833 INFO None2834 INFO TEST-START | dom/events/test/test_bug574663.html 17:50:13 INFO - GECKO(4576) | ++DOMWINDOW == 57 (0F5CB000) [pid = 5076] [serial = 149] [outer = 0E3EDC00] 17:50:13 INFO - GECKO(4576) | [Child 5076] WARNING: Unable to find interface object on global: file z:/build/build/src/dom/base/nsDOMClassInfo.cpp, line 1797 17:50:13 INFO - GECKO(4576) | ++DOCSHELL 0F9F5400 == 6 [pid = 5076] [id = {0f7144fd-6992-4101-ab42-cb92b8e7d710}] 17:50:13 INFO - GECKO(4576) | ++DOMWINDOW == 58 (0F9F5800) [pid = 5076] [serial = 150] [outer = 00000000] 17:50:13 INFO - GECKO(4576) | ++DOCSHELL 1E210400 == 4 [pid = 4576] [id = {62a480c4-b7a3-4b97-a3f2-7e1d40e497ec}] 17:50:13 INFO - GECKO(4576) | ++DOMWINDOW == 19 (1E210800) [pid = 4576] [serial = 24] [outer = 00000000] 17:50:13 INFO - GECKO(4576) | ++DOMWINDOW == 20 (1E2C9800) [pid = 4576] [serial = 25] [outer = 1E210800] 17:50:13 INFO - GECKO(4576) | ++DOCSHELL 1E2D6C00 == 5 [pid = 4576] [id = {56365117-fcc8-44b3-9343-cb07eb5a65ec}] 17:50:13 INFO - GECKO(4576) | ++DOMWINDOW == 21 (1E2D7400) [pid = 4576] [serial = 26] [outer = 00000000] 17:50:13 INFO - GECKO(4576) | ++DOCSHELL 1E9E1800 == 6 [pid = 4576] [id = {623c393c-5aad-4469-bcde-86d366952549}] 17:50:13 INFO - GECKO(4576) | ++DOMWINDOW == 22 (1E9E1C00) [pid = 4576] [serial = 27] [outer = 00000000] 17:50:13 INFO - GECKO(4576) | ++DOMWINDOW == 23 (1F0A3400) [pid = 4576] [serial = 28] [outer = 1E9E1C00] 17:50:13 INFO - GECKO(4576) | [Parent 4576] WARNING: [nsFrameLoader] ReallyStartLoadingInternal tried but couldn't show remote browser. 17:50:13 INFO - GECKO(4576) | : file z:/build/build/src/dom/base/nsFrameLoader.cpp, line 794 17:50:13 INFO - GECKO(4576) | ++DOMWINDOW == 24 (1D909800) [pid = 4576] [serial = 29] [outer = 1E2D7400] 17:50:13 INFO - GECKO(4576) | [Child 5076] WARNING: No inner window available!: file z:/build/build/src/dom/base/nsGlobalWindow.cpp, line 10923 17:50:13 INFO - GECKO(4576) | [Child 5076] WARNING: No inner window available!: file z:/build/build/src/dom/base/nsGlobalWindow.cpp, line 10923 17:50:13 INFO - GECKO(4576) | ++DOMWINDOW == 59 (0FAA5C00) [pid = 5076] [serial = 151] [outer = 0F9F5800] 17:50:13 INFO - GECKO(4576) | --DOMWINDOW == 23 (19E3CC00) [pid = 4576] [serial = 15] [outer = 00000000] [url = about:blank] 17:50:13 INFO - GECKO(4576) | --DOMWINDOW == 22 (21E95800) [pid = 4576] [serial = 20] [outer = 00000000] [url = about:blank] 17:50:13 INFO - GECKO(4576) | --DOMWINDOW == 21 (195D2400) [pid = 4576] [serial = 14] [outer = 00000000] [url = about:blank] 17:50:13 INFO - GECKO(4576) | ++DOMWINDOW == 60 (0FAB2C00) [pid = 5076] [serial = 152] [outer = 0F9F5800] 17:50:13 INFO - TEST-INFO | started process screenshot 17:50:14 INFO - TEST-INFO | screenshot: exit 0 17:50:14 INFO - Buffered messages logged at 17:50:13 17:50:14 INFO - 2835 INFO must wait for load 17:50:14 INFO - 2836 INFO must wait for focus 17:50:14 INFO - 2837 INFO TEST-PASS | dom/events/test/test_bug574663.html | Normal scrolling shouldn't change zoom 17:50:14 INFO - Buffered messages finished 17:50:14 ERROR - 2838 INFO TEST-UNEXPECTED-FAIL | dom/events/test/test_bug574663.html | Normal scrolling should scroll - got +0, expected 18 17:50:14 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5 17:50:14 INFO - check@dom/events/test/test_bug574663.html:125:11 17:50:14 INFO - onpaint@dom/events/test/test_bug574663.html:35:7 17:50:14 INFO - EventListener.handleEvent*waitForPaint@dom/events/test/test_bug574663.html:42:3 17:50:14 INFO - postApzFlush@dom/events/test/test_bug574663.html:56:5

I ran the dom/events/test/test_bug574663.html test on osx locally, and it failed both with and without this. So not much help there. :Aryx, did backing out this patch fix the test failures on automation as hoped? Conceptually I'm struggling to understand how this patch could cause this failure - barring anything silly like mis-matched %ifdefs or something which I've pretty much ruled out. I've also tested manually on windows, linux and osx and normal browser behaviour - including scrolling - appears normal. :kats, blame suggests you might know if anything in this patch - or which landed recently - might explain these failures?

Yes, the backout fixed the issue, see e.g. https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=bfdd14469c9906eaa81c7d6a62ec7525aa109456&group_state=expanded&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=fbf2daa8a9dcd4c313cd0ecc7787822be3d275a0&tochange=91ddffcd405d5abcdc46bd8126d25bbd438f85a0 From the log: > TEST-UNEXPECTED-FAIL | dom/events/test/test_bug574663.html | Normal scrolling should scroll - got +0, expected 18 In the past, failures in tests with fake user input were sometimes triggered by focus issues (the window or element which tested it lacked focus). The no difference ("+0") is also a pointer into that direction.

From a quick inspection of the test and failure I think Stone should look at this. I don't know how the event coalescing thing he added works but it seems like test now assumes a certain coalescing pattern (it synthesizes 6 scroll events but only waits for 2 wheel events). If that coalescing pattern is not satisfied then it's plausible that we try to read the scroll position before APZ has had a chance to send the scroll update to the main thread. Other than that I don't see anything in the patch that might affect the test. I looked one of the screenshots and everything looks ok. There's no download-related things visible in the screenshot so I don't see how that might be interfering.

Actually I take that back. This patch also caused a serious spike in bug 1282638 [1] - all of those 96 failures were between this patch landing and getting backed out. So something is definitely fishy with this patch and not just isolated to the test. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1282638#c42

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23) > From a quick inspection of the test and failure I think Stone should look at > this. I don't know how the event coalescing thing he added works but it > seems like test now assumes a certain coalescing pattern (it synthesizes 6 > scroll events but only waits for 2 wheel events). If that coalescing pattern > is not satisfied then it's plausible that we try to read the scroll position > before APZ has had a chance to send the scroll update to the main thread. > > Other than that I don't see anything in the patch that might affect the > test. I looked one of the screenshots and everything looks ok. There's no > download-related things visible in the screenshot so I don't see how that > might be interfering. Ok thanks for taking a look. I can try to narrow down which aspect of the patch is causing the problems. I don't have a lot of leads, but if I can reproduce locally I can at least eliminate possibilities. I'll start by trying a smaller SVG image for that animation, whittling away at the CSS etc. If you or Stone have any further insights into what I should be looking for, let me know. Fundamentally, all that has changed here is that we replaced a CSS animation (it used to scale/translate an image) with a film-strip style animation, see http://msuja.ws/svg.html for an overview of the technique.

It seems the larger size of the content in the #downloads-animation-container now pushes this element down into the space occupied by the tabbrowser and this is upsetting APZ. I'm testing a patch in which we hide (display:none) this element until needed which seems to fix this problem, but as I'm not clear why this wasn't already the case I'll be interested in hearing feedback.

Comment on attachment 8886725 [details] Bug 1376519 - Photon download notifications. Not sure if you want to clear the r+ on this. This is a small but I guess potentially signification change in how we display the download notifications.

(In reply to Sam Foster [:sfoster] from comment #29) > Comment on attachment 8886725 [details] > Bug 1376519 - Photon download notifications. > > Not sure if you want to clear the r+ on this. This is a small but I guess > potentially signification change in how we display the download > notifications. https://reviewboard.mozilla.org/r/157526/diff/6-8/ shows the change best. This is the addition of the notification attribute on the #downloads-animation-container and CSS to display it.

Comment on attachment 8886725 [details] Bug 1376519 - Photon download notifications. https://reviewboard.mozilla.org/r/157526/#review168746 ::: commit-message-5c3e7:10 (Diff revisions 6 - 8) > * Identifier icon bounce animation for download finish (no notification animation in the #downloads-animation-container) > * Always measure the anchor element as toolbarheight may change in a session (e.g. switching to compact mode) > * Conditionally include the start/finish pngs for non-Photon builds > > +New since last review: > +* Have #downloads-notification-anchor display:none by default, and show it before measuring for placement. This avoids the interference with scrolling/APZ when the notifier element is tall enough to overlap the tabbrowser element The second sentence should also be in a code comment. This is only an issue while testing, and there is no visual issue related to showing the indicator while scrolling, right? If so, add this information to the comment. nit: no need for a "New since last review" section. ::: browser/components/downloads/content/indicator.js:351 (Diff revisions 6 - 8) > // Note: no notifier animation for download finished in Photon > let notifier = this.notifier; > > if (aType == "start" || !AppConstants.MOZ_PHOTON_ANIMATIONS) { > + // show the notifier before measuring for size/placement > + notifier.parentNode.setAttribute("notification", "true"); You can probably use the "hidden" attribute on the notifier to achieve the same effect. Just set hidden="true" in the XUL file as the initial value. ::: browser/components/downloads/content/indicator.js:354 (Diff revisions 6 - 8) > if (aType == "start" || !AppConstants.MOZ_PHOTON_ANIMATIONS) { > + // show the notifier before measuring for size/placement > + notifier.parentNode.setAttribute("notification", "true"); > // the anchor height may vary if font-size is changed or > // compact/tablet mode is selected so recalculate this each time > let anchorRect = anchor.getBoundingClientRect(); This is a guaranteed sync reflow. Not a problem for re-landing this bug as I think a sync reflow was already done in some cases, but file a bug about it. Might require a combination of MozAfterPaint and reentrancy protections to fix.

Comment on attachment 8886725 [details] Bug 1376519 - Photon download notifications. https://reviewboard.mozilla.org/r/157526/#review168750 Thank you for requesting the review, this can re-land with the small issues above fixed!

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23) > From a quick inspection of the test and failure I think Stone should look at > this. I don't know how the event coalescing thing he added works but it > seems like test now assumes a certain coalescing pattern (it synthesizes 6 > scroll events but only waits for 2 wheel events). If that coalescing pattern > is not satisfied then it's plausible that we try to read the scroll position > before APZ has had a chance to send the scroll update to the main thread. > > Other than that I don't see anything in the patch that might affect the > test. I looked one of the screenshots and everything looks ok. There's no > download-related things visible in the screenshot so I don't see how that > might be interfering. :Kats, I identified the source of this issue - see Comment 27. I'll add a comment in there to flag this up for future changes. Can you confirm if this is a test-only artifact or could have impact outside tests?

(In reply to Sam Foster [:sfoster] from comment #33) > :Kats, I identified the source of this issue - see Comment 27. Nice work! > I'll add a > comment in there to flag this up for future changes. Can you confirm if this > is a test-only artifact or could have impact outside tests? I'm not totally clear what you're asking. From the screenshot in comment 27 it looks like the panel is sitting on top of the content area, and even though it's not user-visible, it might be eating input events and preventing them from getting to content. That would probably explain the test failures that were seen. If this is the case, I imagine that it would not be restricted to tests, but would impact behaviour in production as well. That being said I assumed that your updated patch somehow fixes this (at least so that the tests pass) so I'm not sure what "future changes" you had in mind?

> From the screenshot in comment 27 > it looks like the panel is sitting on top of the content area, and even > though it's not user-visible, it might be eating input events and preventing > them from getting to content. That would probably explain the test failures > that were seen. If this is the case, I imagine that it would not be > restricted to tests, but would impact behaviour in production as well. The container element has mousethrough="always" on it, but I don't know if might still intercept other input events? > That being said I assumed that your updated patch somehow fixes this (at > least so that the tests pass) so I'm not sure what "future changes" you had > in mind? Sorry for not being clear. Yes, the patch fixes this by ensuring the element is hidden until we need to display the animation, and then re-hidden after. I just want to add a comment explaining the significance of this particular implementation so it doesnt trip someone up later. If mousethrough=always is not fully transparent to all possible events that might interfere with scrolling, that might be a follow-up bug to investigate a better solution.

I've updated the patch with the suggested changes - to use the hidden attribute on #downloads-notification-anchor by default, and to add a comment about why. I also spotted the finish animation wasn't pulsing twice as intended - a result of the timeout not allowing for 2 iterations of the animation - this is fixed. As this had weird fallout last time it landed, I've triggered a try run to check before pushing (post-merge) https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac00e5d9ea980c175b2762085066685abb96ed37 I've also filed bug 1386456 for the sync reflow in showEventNotification.

(In reply to Sam Foster [:sfoster] from comment #35) > > The container element has mousethrough="always" on it, but I don't know if > might still intercept other input events? > Hm, I'd never heard of this mousethrough property before. I looked it up and it looks like it only applies to XUL boxes, and it does apply to all input events. However the code for it seems to be restricted to the main-thread hit-test, and I don't think APZ is not aware of this at all. That's probably a bug. However I think it might be better to deprecate/avoid this attribute and use the "pointer-events: none" CSS property instead which is better supported and more general. I'm not entirely sure it's a drop-in replacement though since there appears some other special behaviour associated with mousethrough [1]. > Sorry for not being clear. Yes, the patch fixes this by ensuring the element > is hidden until we need to display the animation, and then re-hidden after. > I just want to add a comment explaining the significance of this particular > implementation so it doesnt trip someone up later. If mousethrough=always is > not fully transparent to all possible events that might interfere with > scrolling, that might be a follow-up bug to investigate a better solution. Yeah I think it would be good to file a follow-up bug to fix this more completely. [1] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/layout/xul/nsMenuPopupFrame.cpp#284-285

Comment on attachment 8886725 [details] Bug 1376519 - Photon download notifications. https://reviewboard.mozilla.org/r/157526/#review168746 > The second sentence should also be in a code comment. > > This is only an issue while testing, and there is no visual issue related to showing the indicator while scrolling, right? If so, add this information to the comment. > > nit: no need for a "New since last review" section. Comment added. See thread starting at [comment 1376519](https://bugzilla.mozilla.org/show_bug.cgi?id=1376519) the > This is a guaranteed sync reflow. Not a problem for re-landing this bug as I think a sync reflow was already done in some cases, but file a bug about it. Might require a combination of MozAfterPaint and reentrancy protections to fix. I filed [bug 1386456](https://bugzilla.mozilla.org/show_bug.cgi?id=1386456) for this

Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/89ad7cf2bf1f Photon download notifications. r=Paolo

https://hg.mozilla.org/projects/date/rev/89ad7cf2bf1fbad16946895fa0ac01f4be995c69 Bug 1376519 - Photon download notifications. r=Paolo

I have verified this issue on today's nightly.

Based on comment 44 I will change the tracking flag for Firefox 57.0b4 to verified.

For the first download on a new profile is expected to appear the download manager pop-up, and if you does not click anywhere the pop-up stay there? This issue happens only for the first download on a new profile, the second time the download manager pop-up does not appear and you will see the complete download animation.

(In reply to Timea Zsoldos from comment #46) > For the first download on a new profile is expected to appear the download > manager pop-up, and if you does not click anywhere the pop-up stay there? > This issue happens only for the first download on a new profile, the second > time the download manager pop-up does not appear and you will see the > complete download animation. Yes, that's the expected behavior.