881905 - Make Downloads Panel anchor to the chevron if it is overflowed

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

In the UX branch, the downloads panel can be overflowed into the overflow panel. If the downloads button is clicked in that overflow panel, the downloads panel should anchor to the overflow panel chevron.

Comment 1

[Marco Bonardo [:mak]]

I thought when in the panel the button just opens the Library when clicked, not the panel, is the overflow panel still another different kind of panel? Sorry if it's a dumb question, I thought the menu was also working as an overflow panel and the sorting of widgets was the only relevant thing.

Comment 2

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #1)
> I thought when in the panel the button just opens the Library when clicked,
> not the panel, is the overflow panel still another different kind of panel?
> Sorry if it's a dumb question, I thought the menu was also working as an
> overflow panel and the sorting of widgets was the only relevant thing.

Yes, it's a separate panel.

Mike, I'm guessing this needs bug 880458, so marking as such.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1

r? on mak for indicator.js.  jaws for OverflowableToolbar.

Jared - I had to update OverflowableToolbar because the Downloads button does some sleight-of-hand on first-click, where it loads an overlay with the downloads indicator, inserts the indicator before the downloads button, and then hides the downloads button. If the downloads button was originally in the overflow panel, this caused the indicator to permanently stay in the overflow panel because it never got overflowed in the first place (and so it's not in the _collapsed Array).

So what I've got going on here instead - we just pop off the first element in the overflowed panel on lazy resize. If we have a minSize for it - great, we can check for that and bail out. If not, we just try to append it.

Does this sound sane?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Note that one must apply bug 873712 before this patch in order to test it.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 777857 [details] [diff] [review]
Patch v1.1

Review of attachment 777857 [details] [diff] [review]:
-----------------------------------------------------------------

the indicator part looks ok

::: browser/components/downloads/content/indicator.js
@@ +580,5 @@
>  
>    get indicatorAnchor()
>    {
> +    let widgetGroup = CustomizableUI.getWidget("downloads-button");
> +    let widget = widgetGroup.forWindow(window);

couldn't you merge these by chaining? we don't need widgetGroup

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 777857 [details] [diff] [review]
Patch v1.1

Review of attachment 777857 [details] [diff] [review]:
-----------------------------------------------------------------

Do we have tests for this? It would have been good to have something that we could have added a regression test to.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2295,5 @@
>      this._lazyResizeHandler.start();
>    },
>  
>    _moveItemsBackToTheirOrigin: function(shouldMoveAllItems) {
> +    while(this._list.firstChild) {

nit: space between keyword and opening paren when the statement it is not a function call.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Test patch

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Test Patch v1.1

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2 (r+'d by jaws, mak)

Thanks! Concerns fixed.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 781036 [details] [diff] [review]
Test Patch v1.1

Here's my regression test. I had to do a bit of tweaking in indicator.js due to the initial anchor retrieval bugging out if the panel attempts to open itself while the overflow panel is closed.

Also had to modify browser_first_download_panel.js because I seem to have set a trap for myself - it set things up so that the next thing to open the panel would cause a test failure. :/ It's like a prank from my past-self.

How does this look, mak?

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 781036 [details] [diff] [review]
Test Patch v1.1

Review of attachment 781036 [details] [diff] [review]:
-----------------------------------------------------------------

some questions:

::: browser/components/downloads/content/indicator.js
@@ +177,2 @@
>      // Determine if the placeholder is located on an invisible toolbar.
> +    if (!isElementVisible(placeholder.parentNode) && !widget.overflowed) {

you could make _placeholder return the widget, and use _placeholder.node around, for the node

how does this handle an overflowed downloads-indicator?

::: browser/components/downloads/test/browser/browser_first_download_panel.js
@@ +31,5 @@
>      // Next, make sure that if we start another download, we don't open
>      // the panel automatically.
> +    let originalOnViewLoadCompleted = DownloadsPanel.onViewLoadCompleted;
> +    DownloadsPanel.onViewLoadCompleted = function () {
> +      DownloadsPanel.onViewLoadCompleted = originalOnViewLoadCompleted;

you should rather put this into a registerCleanupFunction o protect from timeouts

we go regardless through the code you added below that sets back the handler in the common case, so there should be no need to set it back here already.

@@ +41,1 @@
>      yield waitFor(2);

not your fault, I'm not sure why we accepted this sucky waiting in the past, instead of properly wait for events

::: browser/components/downloads/test/browser/browser_overflow_anchor.js
@@ +17,5 @@
> +
> +    // The downloads button should not be overflowed to begin with.
> +    let button = CustomizableUI.getWidget("downloads-button")
> +                               .forWindow(window);
> +    ok(!button.overflowed, "Downloads button should not be overflowed.");

are you sure we have the button here and not the indicator? how can you tell if previous tests didn't already replace it? would the test still work in such a case?

Comment 13

[Mike de Boer [:mikedeboer]]

Mike, does your patch still need work or is it OK to land as-is?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

The original patch is probably OK to land, but I haven't had cycles to address mak's feedback on the regression test. If you want to push that part though, be my guest. :)

Comment 15

[Mike de Boer [:mikedeboer]]

I will run through the regression test. Will land patches with you name on 'em, don't worry ;)

Comment 16

[Marco Bonardo [:mak]]

the most important comment was 

> how does this handle an overflowed downloads-indicator?

Comment 17

[Marco Bonardo [:mak]]

I'm asking cause this seems to use downloads-button, not downloads-indicator, those are different elements.

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to Marco Bonardo [:mak] (Away Aug 07-11) from comment #17)
> I'm asking cause this seems to use downloads-button, not
> downloads-indicator, those are different elements.

The code in the test is similar to the code in attachment 781037 [details] [diff] [review]; getWidget() will work with the widget ID and return the widget object, even when the placeholder node is replaced with the indicator node.

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.3 (r+'d by jaws, mak)

added patch metadata & moved indicator.js code from test patch to functional patch.

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Test Patch v1.2

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 787484 [details] [diff] [review]
Test Patch v1.2

Review of attachment 787484 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/test/browser/Makefile.in
@@ +12,5 @@
>  
>  MOCHITEST_BROWSER_FILES = \
>    browser_basic_functionality.js \
>    browser_first_download_panel.js \
> +  browser_overflow_anchor.js \

did you forget to attach the test file?

Comment 22

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.4 (r+'d by jaws, mak)

Unbitrotted patch.

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: Test Patch v1.3

now WITH test file ;)

Comment 24

[Mike de Boer [:mikedeboer]]

Attachment: Test Patch v1.4

nits.

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 805902 [details] [diff] [review]
Test Patch v1.4

Review of attachment 805902 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/downloads/test/browser/browser_overflow_anchor.js
@@ +44,5 @@
> +    // Unlock the widths on the flex-y items.
> +    unlockWidth(kFlexyItems);
> +
> +    // Put the window back to its original dimensions.
> +    window.resizeTo(oldWidth, window.outerHeight);

may we worth to also do these (unlock and resize) in a registerCleanupFunction, in case the test should timeout we should reset browser's state

@@ +58,5 @@
> +    is(panel.anchorNode.id, "downloads-indicator-anchor");
> +
> +    DownloadsPanel.hidePanel();
> +  } catch(e) {
> +    ok(false, e.stack);

are not exceptions  in add_task already reported as errors by the harness?

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.5 (r+'d by jaws, mak)

Unbitrotted patch. Carrying over r=mak.

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Test Patch v1.5

Addressed Marco's review comments. Carrying over r=mak.

Comment 28

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/projects/ux/rev/a1d4ebbbf339
https://hg.mozilla.org/projects/ux/rev/2f55c459c3fb

Comment 29

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/a1d4ebbbf339
https://hg.mozilla.org/mozilla-central/rev/2f55c459c3fb