Closed Bug 881905 Opened 6 years ago Closed 6 years ago

Make Downloads Panel anchor to the chevron if it is overflowed

Categories

(Firefox :: Downloads Panel, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M9][Australis:P2])

Attachments

(2 files, 10 obsolete files)

5.52 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
4.69 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
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.
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.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P2]
(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.
Depends on: 880458
Attached patch Patch v1 (obsolete) — Splinter Review
Depends on: 873712
Attached patch Patch v1.1 (obsolete) — Splinter Review
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?
Attachment #777851 - Attachment is obsolete: true
Attachment #777857 - Flags: review?(mak77)
Attachment #777857 - Flags: review?(jaws)
Note that one must apply bug 873712 before this patch in order to test it.
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
Attachment #777857 - Flags: review?(mak77) → review+
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.
Attachment #777857 - Flags: review?(jaws) → review+
Attached patch Test patch (obsolete) — Splinter Review
Attached patch Test Patch v1.1 (obsolete) — Splinter Review
Attachment #781034 - Attachment is obsolete: true
Attached patch Patch v1.2 (r+'d by jaws, mak) (obsolete) — Splinter Review
Thanks! Concerns fixed.
Attachment #777857 - Attachment is obsolete: true
Attachment #781037 - Flags: review+
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?
Attachment #781036 - Flags: review?(mak77)
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?
Attachment #781036 - Flags: review?(mak77)
Status: NEW → ASSIGNED
Mike, does your patch still need work or is it OK to land as-is?
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. :)
I will run through the regression test. Will land patches with you name on 'em, don't worry ;)
Assignee: mconley → mdeboer
the most important comment was 

> how does this handle an overflowed downloads-indicator?
I'm asking cause this seems to use downloads-button, not downloads-indicator, those are different elements.
(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.
Attached patch Patch v1.3 (r+'d by jaws, mak) (obsolete) — Splinter Review
added patch metadata & moved indicator.js code from test patch to functional patch.
Attachment #781037 - Attachment is obsolete: true
Attachment #787483 - Flags: review+
Attached patch Test Patch v1.2 (obsolete) — Splinter Review
Attachment #781036 - Attachment is obsolete: true
Attachment #787484 - Flags: review?(mak77)
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?
Flags: needinfo?(mdeboer)
Attached patch Patch v1.4 (r+'d by jaws, mak) (obsolete) — Splinter Review
Unbitrotted patch.
Attachment #787483 - Attachment is obsolete: true
Attachment #804382 - Flags: review+
Flags: needinfo?(mdeboer)
Attached patch Test Patch v1.3 (obsolete) — Splinter Review
now WITH test file ;)
Attachment #787484 - Attachment is obsolete: true
Attachment #787484 - Flags: review?(mak77)
Attachment #804384 - Flags: review?(mak77)
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M9][Australis:P2]
Attached patch Test Patch v1.4 (obsolete) — Splinter Review
nits.
Attachment #804384 - Attachment is obsolete: true
Attachment #804384 - Flags: review?(mak77)
Attachment #805902 - Flags: review?(mak77)
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?
Attachment #805902 - Flags: review?(mak77) → review+
Unbitrotted patch. Carrying over r=mak.
Attachment #804382 - Attachment is obsolete: true
Attachment #805915 - Flags: review+
Attached patch Test Patch v1.5Splinter Review
Addressed Marco's review comments. Carrying over r=mak.
Attachment #805902 - Attachment is obsolete: true
Attachment #805916 - Flags: review+
https://hg.mozilla.org/projects/ux/rev/a1d4ebbbf339
https://hg.mozilla.org/projects/ux/rev/2f55c459c3fb
Whiteboard: [Australis:M9][Australis:P2] → [Australis:M9][Australis:P2][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/a1d4ebbbf339
https://hg.mozilla.org/mozilla-central/rev/2f55c459c3fb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P2][fixed-in-ux] → [Australis:M9][Australis:P2]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.