1393136 - Download notification arrow is misaligned to the toolbar button

Status: NEW

(Firefox :: Downloads Panel, defect, P5)

Description

[Sam Foster [:sfoster] (he/him)]

Since bug 1371765, when you start the first download in a session the arrow animation *sometimes* drops into a spot to the left of where the download button appears. 

This was happening for me every time this morning with a new session, but when I went to file the bug I can no longer reproduce so beware the observer effect!

Comment 1

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1393136 - Schedule measurement and positioning of the notifier with promisedLayoutFlushed.

Review commit: https://reviewboard.mozilla.org/r/173532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173532/

Comment 2

[Sam Foster [:sfoster] (he/him)]

I think the proposed fix in attachment 8902096 [details] would also fix bug 1386456 as it removes the sync reflow when we unhide and measure the notifier element. 

I'm only able to reproduce this misalignment problem with bug 1371765, so I may end up moving this patch to 1386456 if the as-needed download button isn't coming back soonish.

Comment 3

[Sam Foster [:sfoster] (he/him)]

My understanding is that the no-download-button-until-needed behaviour from bug 1371765 will come back in some form for this release, so I'll leave this here. It is harder to verify however as this bug seems to only reproduce when that patch is applied (and it need conflicts resolving manually when you apply it) What I can verify is that the patch doesnt regress anything.

Comment 4

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8902096 [details]
Bug 1393136 - Schedule measurement and positioning of the notifier with promisedLayoutFlushed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173532/diff/1-2/

Comment 5

[:Paolo Amadini]

Comment on attachment 8902096 [details]
Bug 1393136 - Schedule measurement and positioning of the notifier with promisedLayoutFlushed.

https://reviewboard.mozilla.org/r/173532/#review179424

In general, this looks like an improvement, however I'm not sure that this would fix the misalignment reliably.

The misalignemnt was probably caused by not waiting enough time after the overlay was loaded, and this patch makes us wait some more time, which happens to be enough time, but there is nothing that connects the two events in an obvious way. Probably just waiting a tick would have had the same effect.

::: browser/components/downloads/content/indicator.js:337
(Diff revision 2)
>     * brief time a visual notification of a relevant event, like a new download.
>     *
>     * @param aType
>     *        Set to "start" for new downloads, "finish" for completed downloads.
>     */
> -  _showNotification(aType) {
> +  async _showNotification(aType) {

You should change the callers of this function to use the returned Promise, for example adding ".catch(Cu.reportError)".

::: browser/components/downloads/content/indicator.js:396
(Diff revision 2)
>        let widthDiff = anchorRect.width - notifierRect.width;
> -      let translateX = (leftDiff + .5 * widthDiff) + "px";
> -      let translateY = (topDiff + .5 * heightDiff) + "px";
> -      notifier.style.transform = "translate(" + translateX + ", " + translateY + ")";
> +      notifierCoords.x = (leftDiff + .5 * widthDiff) + "px";
> +      notifierCoords.y = (topDiff + .5 * heightDiff) + "px";
> +    }
> +
> +    await new Promise(resolve => requestAnimationFrame(resolve));

Unfortunately, most of the implications of using requestAnimationFrame and promiseLayoutFlushed, especially when combined with microtasks and event loop ticks, are still unclear to me.

Can we run the operations we normally do inside requestAnimationFrame in the next microtask rather than in the callback itself?

Comment 6

[Jim Mathies [:jimm]]

Hey Sam, haven't seen activity on this P1 in a while, are we still blocking on this for 57?

Comment 7

[Sam Foster [:sfoster] (he/him)]

I'm unassigning myself for now as I'm not actively working on this. We do want this resolved if possible, but it needn't block 57. 

From Comment #5, it sounds like the approach wasnt exactly wrong, but I need to find an event or some other way to tie the measurement and start of the animation to the loading and completion of the indicator overlay. Also, since this bug was filed, bug 1397447 has landed with a revised implementation of the download button appearing on first use, so we'll want to confirm this is still reproducible.

Comment 8

[Jim Mathies [:jimm]]

P3, unassigned, and referencing comment 7 -> fix-optional for 57