Download notification arrow is misaligned to the toolbar button

NEW
Unassigned

Status

()

P5
normal
a year ago
6 months ago

People

(Reporter: sfoster, Unassigned)

Tracking

({polish, regression})

57 Branch
polish, regression
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fix-optional, firefox60 fix-optional, firefox61 fix-optional, firefox62 fix-optional)

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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!
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox57: --- → affected
status-firefox-esr52: --- → unaffected
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → 57 Branch
Flags: qe-verify+
Priority: -- → P3
QA Contact: stefan.georgiev
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]
(Reporter)

Updated

a year ago
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Iteration: --- → 57.2 - Aug 29
Priority: P3 → P1
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Comment hidden (mozreview-request)
(Reporter)

Comment 2

a year ago
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.
(Reporter)

Comment 3

a year ago
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 hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
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?
Attachment #8902096 - Flags: review?(paolo.mozmail)
Hey Sam, haven't seen activity on this P1 in a while, are we still blocking on this for 57?
Flags: needinfo?(sfoster)
Assignee: sfoster → nobody
Status: ASSIGNED → NEW
Iteration: 57.3 - Sep 19 → ---
Priority: P1 → P3
(Reporter)

Comment 7

a year ago
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.
(Reporter)

Updated

a year ago
Flags: needinfo?(sfoster)
P3, unassigned, and referencing comment 7 -> fix-optional for 57
status-firefox57: affected → fix-optional
Keywords: polish
Priority: P3 → P4

Updated

6 months ago
status-firefox60: --- → fix-optional
status-firefox61: --- → fix-optional
status-firefox62: --- → fix-optional
Priority: P4 → P5
You need to log in before you can comment on or make changes to this bug.