883953 - Implement Download Infobar Behaviours

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Downloads, defect)

Description

[Marina Samuel [:emtwo]]

Includes handling of swipes and taps that open and hide the app bar and tap strip.

Comment 1

[Marina Samuel [:emtwo]]

Attachment: Part 1: v1: Move infobar to bottom of screen.

Comment 2

[Marina Samuel [:emtwo]]

Attachment: Part 2: v1: Implement infobar behaviours

So far:
* Makes the appbar appear 1000ms after a download request
* Displays infobars above the appbar

Question:
Right now for some reason when both the appbar and inforbar are visible, clicking a button on the infobar will not register as a button click but instead as a content click causing the appbar to dismiss. Matt, any idea how to fix this? Thanks!

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to Marina Samuel [:emtwo] from comment #2)
> Right now for some reason when both the appbar and inforbar are visible,
> clicking a button on the infobar will not register as a button click but
> instead as a content click causing the appbar to dismiss. Matt, any idea how
> to fix this? Thanks!

I think this is caused by the "mousedown" handler in ContextUI.js:
http://hg.mozilla.org/mozilla-central/file/9ab28833eda0/browser/metro/base/content/ContextUI.js#l275

This treats any mousedown within <deck id="browsers"> as a content interaction.  You can change this code to check the event target and ignore events that start within a <notification>.  We have code to do something similar in PopupBlockerObserver:
http://hg.mozilla.org/mozilla-central/file/9ab28833eda0/browser/metro/base/content/browser.js#l1302

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 789072 [details] [diff] [review]
Part 1: v1: Move infobar to bottom of screen.

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

::: browser/metro/base/content/ContentAreaObserver.js
@@ +187,5 @@
>    },
>  
> +  onBrowserCreated: function onBrowserCreated(aNotificationBox) {
> +    aNotificationBox.classList.add("content-width");
> +    aNotificationBox.classList.add("content-height");

Minor nit: I think I'd prefer to keep passing the browser itself to this function, and then use "aBrowser.parent" to set styles on the notification box.  Mostly a naming thing - it seems surprising to have onBrowserCreated receive something other than browser.

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 789074 [details] [diff] [review]
Part 2: v1: Implement infobar behaviours

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

Looks good but I'd like to see some of the code shuffled around a little:

::: browser/metro/base/content/ContextUI.js
@@ +109,5 @@
>        shown = true;
>      }
>  
>      if (shown) {
> +      ContentAreaObserver.update(window.innerWidth, this._windowHeight);

I'd prefer not to lie to ContentAreaObserver about the window dimensions.  "window-width" and "window-height" should always reflect the actual window size.  Things that need to shrink when toolbars appear should use "content-width" and "content-height" instead.

(Note: ContentAreaObserver is more complex and confusing than it should be, because of leftover cruft.  For example viewable-width and viewable-height are no longer used but we haven't gotten around to removing them yet.)

@@ +228,5 @@
>  
> +  _updateHeight: function updateHeight() {
> +    let notificationBox = Browser.getNotificationBox();
> +    if (!notificationBox.notificationsHidden &&
> +        notificationBox.allNotifications.length != 0) {

...so, please move this logic into ContentAreaObserver._getContentHeightForWindow please, and just call ContentAreaObserver.updateContentArea here.

@@ +229,5 @@
> +  _updateHeight: function updateHeight() {
> +    let notificationBox = Browser.getNotificationBox();
> +    if (!notificationBox.notificationsHidden &&
> +        notificationBox.allNotifications.length != 0) {
> +     this._windowHeight -= 64;

Please replace the 64 with a getBoundingClientRect or similar, if possible.

::: browser/metro/base/content/downloads.js
@@ +449,5 @@
> +      case "dl-request":
> +        let timer = new Util.Timeout(function() {
> +          ContextUI.displayNavbar();
> +        });
> +        timer.once(1000);

There's an annoying problem with nsITimer that shows up here: this might not fire because "timer" may be garbage collected:
http://www.joshmatthews.net/blog/2011/03/nsitimer-anti-pattern/

You can just use setTimeout() here instead.

I'm also curious why this is on a one-second delay.

Comment 6

[Marina Samuel [:emtwo]]

(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> I'm also curious why this is on a one-second delay.

Thanks for the really quick reviews! The one-second delay is part of the spec. I believe Yuan said it was to draw the user's attention that there's a download request and then show the appbar for download progress.

Comment 8

[Marina Samuel [:emtwo]]

Attachment: Part 1: v2: Move infobar to bottom of screen.

Fixed nit.

Comment 9

[Marina Samuel [:emtwo]]

Attachment: Part 2: v2: Implement infobar behaviours

* Addressed comments

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 789616 [details] [diff] [review]
Part 2: v2: Implement infobar behaviours

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

Nice!

The one-second delay still seems a bit odd to me; it causes some jumpiness near the start of a download.  But we can polish that later.

::: browser/metro/base/content/ContentAreaObserver.js
@@ +134,5 @@
>      let newHeight = height || this.contentHeight;
>  
> +    if (Browser.selectedBrowser) {
> +      let notificationBox = Browser.getNotificationBox();
> +      if (ContextUI.navbarVisible && !notificationBox.notificationsHidden &&

Please add a comment here to explain why we are doing this only when a notification is present.

::: browser/metro/base/content/ContextUI.js
@@ -280,4 @@
>          break;
>        case "ToolPanelShown":
>        case "ToolPanelHidden":
> -      case "AlertActive":

You can also remove the addEventListener("AlertActive"...) call earlier in this file, which is now unused.

Comment 11

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/e4d7f9580e5a
https://hg.mozilla.org/integration/fx-team/rev/d864c70c64c9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-mc orange.
https://hg.mozilla.org/integration/fx-team/rev/05d5261cdb70

https://tbpl.mozilla.org/php/getParsedLog.php?id=26498921&tree=Fx-Team

Comment 13

[Marina Samuel [:emtwo]]

Attachment: Part 3: Fix Test Failure

This was pretty weird but here's what I think was happening:

Moving notifications to the bottom of the screen pushed the browser content up so that a synthesized text selection was not triggering an expected popup menu but instead it brought down the tab bar, leaving the test hanging and timing out.

Based on this hypothesis, I moved the selected content down a bit which seems to have fixed the issue.

Please let me know if this makes sense.

Comment 14

[Jim Mathies [:jimm]]

We might take the opportunity to tidy this test up a bit: instead of using sendContextMenuClick, we could use sendContextMenuClickToElement without coords, then touch up the position bounds check below it.

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/browser_context_menu_tests.js#323

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 790500 [details] [diff] [review]
Part 3: Fix Test Failure

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

This is fine, as long as we're pretty sure there's no user-visible bug here.

Comment 16

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/8a1c9731a05d
https://hg.mozilla.org/integration/fx-team/rev/f0f8ef217538
https://hg.mozilla.org/integration/fx-team/rev/f07b2e81d2ff

Comment 17

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/f9cdc959a5a0

Comment 18

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

There was a little confusion about this landing on top of the earlier orange push. Maybe reference which test & push its fixing in the commit msg or bug comment next time.

Comment 19

[Marina Samuel [:emtwo]]

Ah sorry about that. Will be more specific next time. Are there any other issues or confusion to be cleared up?

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a1c9731a05d
https://hg.mozilla.org/mozilla-central/rev/f0f8ef217538
https://hg.mozilla.org/mozilla-central/rev/f07b2e81d2ff
https://hg.mozilla.org/mozilla-central/rev/f9cdc959a5a0

Comment 21

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

Maybe those "magic" numbers in the test need an explanatory comment or explanatory variable? But for now it looks like you're all landed and good.