If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement Download Infobar Behaviours

RESOLVED FIXED in Firefox 26

Status

Firefox for Metro
Downloads
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: emtwo, Assigned: emtwo)

Tracking

unspecified
Firefox 26
x86_64
Windows 8
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [preview])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Includes handling of swipes and taps that open and hide the app bar and tap strip.
(Assignee)

Updated

4 years ago
Blocks: 831942

Updated

4 years ago
No longer blocks: 831942

Updated

4 years ago
Blocks: 886942
(Assignee)

Updated

4 years ago
Blocks: 883951

Updated

4 years ago
No longer blocks: 886942

Updated

4 years ago
Blocks: 893066
Whiteboard: [preview]
(Assignee)

Comment 1

4 years ago
Created attachment 789072 [details] [diff] [review]
Part 1: v1: Move infobar to bottom of screen.
Attachment #789072 - Flags: feedback?(mbrubeck)
(Assignee)

Comment 2

4 years ago
Created attachment 789074 [details] [diff] [review]
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!
Attachment #789074 - Flags: feedback?(mbrubeck)
Flags: needinfo?(mbrubeck)
(Assignee)

Updated

4 years ago
Attachment #789074 - Attachment is patch: true
(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 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.
Attachment #789072 - Flags: feedback?(mbrubeck) → feedback+
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.
Attachment #789074 - Flags: feedback?(mbrubeck) → feedback+
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 893129

Updated

4 years ago
Assignee: nobody → msamuel
(Assignee)

Comment 8

4 years ago
Created attachment 789613 [details] [diff] [review]
Part 1: v2: Move infobar to bottom of screen.

Fixed nit.
Attachment #789072 - Attachment is obsolete: true
Attachment #789613 - Flags: review?(mbrubeck)
(Assignee)

Comment 9

4 years ago
Created attachment 789616 [details] [diff] [review]
Part 2: v2: Implement infobar behaviours

* Addressed comments
Attachment #789074 - Attachment is obsolete: true
Attachment #789616 - Flags: review?(mbrubeck)
Attachment #789613 - Flags: review?(mbrubeck) → review+
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.
Attachment #789616 - Flags: review?(mbrubeck) → review+
Flags: needinfo?(mbrubeck)
(Assignee)

Comment 11

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/e4d7f9580e5a
https://hg.mozilla.org/integration/fx-team/rev/d864c70c64c9
(Assignee)

Updated

4 years ago
Whiteboard: [preview] → [fixed-in-fx-team][preview]
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
(Assignee)

Comment 13

4 years ago
Created attachment 790500 [details] [diff] [review]
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.
Attachment #790500 - Flags: review?(mbrubeck)

Comment 14

4 years ago
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 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.
Attachment #790500 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 16

4 years ago
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
(Assignee)

Comment 17

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/f9cdc959a5a0
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.
(Assignee)

Comment 19

4 years ago
Ah sorry about that. Will be more specific next time. Are there any other issues or confusion to be cleared up?
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][preview] → [preview]
Target Milestone: --- → Firefox 26
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.
Depends on: 906772
You need to log in before you can comment on or make changes to this bug.