747786 - [tracking] Support new Windows 8 navigation bar and tab bar

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Brian R. Bondy [:bbondy]]

Attachment: Address bar

This task is to add support for the new navigation controls on Windows 8 Metro.
Please see the attached screenshots.

- The current tab bar on the left should be removed
- When you visit a new site, the address bar should show up. 
- You can click on the content to dismiss the address bar. 
- You should be able to swipe down from the top edge to reveal the address bar at any time.  
- If you swipe down again it should reveal the tab bar.  
- If you do a long swipe down it should slide in the address bar and snap to it, if you keep dragging it should continue to slide in the tab bar. 
- The tab bar should have the open tabs along the top with a preview of each.  
- Each tabp review should have an x on the top right to close the tab.
- The tab bar should have a + icon for new tab on the right and a ... for always show tabs.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Tab bar

Comment 2

[Brian R. Bondy [:bbondy]]

How's this work going? I just wanted to mention also that I landed the appbar work on elm so you may have to rebase slightly if you are also coding on elm.  Most of its work is in new files though.

Comment 3

[Jonathan Wilde [:jwilde]]

Attachment: patch v0.5

I just got most of my work rebased
Here's a very early patch for the tab bar and URL bar.  Feedback appreciated.

What's there:
- Overall layout and styling.
- Underlying slide-in, slide-out animation.
- Unified forward-back button has the correct interaction (pulled from desktop FF)

What's not:
- Unified forward-back button looks ugly.  Need to ping shorlander for styling assistance and for a forward icon.  Need to clean up margins.
- URL bar text label needs modification to show highlighted URL instead of page title.
- Slide-in interaction needs to be aligned with Yuan's design documents.
- Tap targets for tab close buttons are frustratingly small.
- Need stop icon.
- Need to integrate some sort of throbber animation.
- No integration with edge swipes.
- Identity panel is 100% broken at this point.  Need to pull icons representing page security (from Australis mockups) and apply Metro styling from Metro PSD.

Comment 4

[Jonathan Wilde [:jwilde]]

Also missing from above patch:
- Only show tabs button.
- Plain tabs styling without thumbnails.

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 636556 [details] [diff] [review]
patch v0.5

Great progress, I imported the patch in my local repo and it looks awesome!
I did see a problem with loading sites though, not sure if it's only on my side though.  Looking forward to seeing the next version of the patch.

Comment 6

[Jonathan Wilde [:jwilde]]

Initial version pushed to elm (http://hg.mozilla.org/projects/elm/rev/9089bd7394c6).

Comment 7

[Jonathan Wilde [:jwilde]]

Current known issues:
- URL bar shows title rather than URL with highlighted hostname
- Identity panel does not open
- Meta-Z works on some platforms but not others
- No Metro-styled stop icon
- No throbber
- Animation performance issues
- No thumbnail closing animation
- Active/hover states of buttons are not implemented

Comment 8

[Jonathan Wilde [:jwilde]]

Two other issues:
- The unified forward-back button shouldn't be animated during tab switching
- Tab open/close toggle needs to be implemented

Comment 9

[Brian R. Bondy [:bbondy]]

I'm having the same problem as previously with the patches but on elm tip.  When I type in a URL into the address bar and click enter, it seems to delete the whole URL except for the first 2 letters: "ht".  Then it doesn't load the page.

It does load pages for links I click in content successfully though.

Comment 10

[Jonathan Wilde [:jwilde]]

:/ I'm currently not experiencing this when running the UI under Win32, but it seems like it's related to the larger problem of broken AwesomePanel integration. I'm currently pulling out AwesomePanel in preparation for adding AwesomeScreen.

Comment 11

[Brian R. Bondy [:bbondy]]

I get the problem on both the windows 8 Metro browser and when running firefox.exe -metrobrowser

Comment 12

[Brian R. Bondy [:bbondy]]

Jim or Tim, could you verify if you also have the problem? If it's just me that's ok, but if not we should backout from elm for now.

Comment 13

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #12)
> Jim or Tim, could you verify if you also have the problem? If it's just me
> that's ok, but if not we should backout from elm for now.

I can confirm, not able to navigate and I get the weird "ht" in the addressbar afterward.

In the console I get 

mozilla::widget::winrt::FrameworkView::OnWindowActivated
JavaScript error: chrome://browser/content/commandUtil.js, line 107: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "toolbar is null" {file: "chrome://browser/cont
ent/AwesomePanel.js" line: 144}]' when calling method: [nsIController::doCommand]
JavaScript error: chrome://browser/content/AwesomePanel.js, line 144: toolbar is null

Comment 14

[Jim Mathies [:jimm]]

Attachment: awesomebar wallpaper patch

here's a wallpaper patch that seems to fix the problem. I've landed this on elm.

Comment 15

[Brian R. Bondy [:bbondy]]

I'm still having trouble navigating even with your patch Jim, I'm going to backout the recent patches. 

Please feel free to re-land Jonathan after fixing and testing in metro mode itself.  It also caused some problems with the appbar.

Comment 16

[Brian R. Bondy [:bbondy]]

Backed out these changesets:
https://hg.mozilla.org/projects/elm/rev/f6c514f09117
https://hg.mozilla.org/projects/elm/rev/6ad7b233fc90
https://hg.mozilla.org/projects/elm/rev/cdc4636c154d
https://hg.mozilla.org/projects/elm/rev/9089bd7394c6


Via these changesets:
http://hg.mozilla.org/projects/elm/rev/9dde8779c554
http://hg.mozilla.org/projects/elm/rev/56f803ccf49b
http://hg.mozilla.org/projects/elm/rev/2f6a14966e61
http://hg.mozilla.org/projects/elm/rev/d3b0c898b493

Comment 17

[Jonathan Wilde [:jwilde]]

Attachment: patch v2

Here's a new patch that fixes most of the issues with the changesets that were backed tout.  There is now an added issue with the URL bar that I'm still trying to sort out in the Metro version of the app: sometimes, seemingly at random, the URL bar can become focused without the onscreen keyboard appearing or being able to type in text with the keyboard.

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 638949 [details] [diff] [review]
patch v2

I can load a page but after that the urlbar seems to lock up.  I can't navigate to another page after that or type in a new address.  I didn't actually take a look at the code yet but just tried out the patch btw.

Comment 19

[Brian R. Bondy [:bbondy]]

I just tried it on my laptop so I am using a normal keyboard btw.

Comment 20

[Jonathan Wilde [:jwilde]]

Attachment: Fixed URL bar locking

Turns out that the autocomplete XBL binding for the URL bar don't get along well with the accessibility layer.  Because of that, Windows 8 was having trouble recognizing the URL bar, causing the URL bar to appear locked.

I swapped out the current Fennec-specific autocomplete binding with <chrome://global/content/bindings/autocomplete.xml#autocomplete>.  There are currently still some a11y-related assertions thrown that shouldn't be, but the URL bar seems to work now.

There's also some cleanup of some debugging comments and code that unfortunately made it into the previous patch.

Comment 21

[Frank Yan (:fryn)]

Comment on attachment 639561 [details] [diff] [review]
Fixed URL bar locking

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

::: browser/metro/base/content/appbar.js
@@ +5,4 @@
>    },
>  
>    dismiss: function Appbar_dismiss() {
> +    ContextUIHelper.dismiss();

Why remove show and not dismiss?
Seems odd to make this asymmetric.

::: browser/metro/base/content/bindings/appbar.xml
@@ +6,2 @@
>    <binding id="appbarBinding">  
> +    <resources></resources>

If it doesn't need resources, you can just remove this.
Also, nit: blank line above.

@@ +13,5 @@
> +      <constructor>
> +        <![CDATA[
> +          window.addEventListener('MozContextUIShow', this.show.bind(this));
> +          window.addEventListener('MozContextUIExpand', this.show.bind(this));
> +          window.addEventListener('MozContextUIDismiss', this.dismiss.bind(this));

The problem with using `bind` like this is that it becomes more complex to remove the event listener without using arguments.callee, since this.dismiss !== this.dismiss.bind(this).

Another way to ensure that `this` is the element itself is to have a <method name="handleElement"> with `switch...case`. If we need to remove event listeners, we should probably use this method. If not, I leave it up to you.

@@ +40,2 @@
>  
> +    <handlers></handlers>

If it doesn't need handlers, you can just remove this, I think.

::: browser/metro/base/content/browser-ui.js
@@ +277,4 @@
>    },
>  
>    get toolbarH() {
> +    delete this._toolbarH;

If we aren't setting _toolbarH anymore, we should remove this line.

@@ +704,5 @@
> +    let hidden = stylesheet.insertRule("#tray {}", stylesheet.cssRules.length);
> +    stylesheet.cssRules[hidden].style.marginTop = -1 * (toolbarHeight + tabsHeight) + "px";
> +
> +    let visible = stylesheet.insertRule("#tray[visible=true]:not([expanded=true]) {}", stylesheet.cssRules.length);
> +    stylesheet.cssRules[visible].style.marginTop = -1 * tabsHeight + "px";

This is pretty wacky, but it works. :P
Oh CSSOM.

@@ +710,5 @@
> +    Elements.tray.addEventListener("transitionend", function () {
> +      setTimeout(function (){
> +        ViewableAreaObserver.update({ force: true });
> +        BrowserUI.sizeControls();
> +      }, 0);

Why is this timeout needed?

::: browser/metro/base/content/browser.js
@@ +192,3 @@
>      let stylesheet = document.styleSheets[0];
> +    for each (let style in ["window-width", "window-height",
> +                            "viewable-height", "viewable-width"]) {

Use `for...of` here.
https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of

@@ +719,4 @@
>      return this._selectedTab;
>    },
>  
> +  set selectedTab(tab) { 

Nit: you added a trailing space here.

::: browser/metro/base/content/browser.xul
@@ +182,3 @@
>                  ondoubletap="BrowserUI.newTab();"
>                  onselect="BrowserUI.selectTab(this);"
> +                onclosetab="BrowserUI.closeTab(this)" />

This is ferrealz XML, not wonky HTML that needs to support legacy parsers, so no space is needed before the /. :P Likewise everywhere else in our XML files.

@@ +253,5 @@
>  
> +      <!-- Content viewport -->
> +      <stack>
> +        <deck id="browsers" flex="1" observes="bcast_uidiscovery"/>
> +        <box id="vertical-scroller" class="scroller" orient="vertical" end="2" top="0"/>

What does the end="2" do here?

::: browser/metro/theme/platform.css
@@ +10,4 @@
>  
>  /* general stuff ------------------------------------------------------------ */
>  :root {
> +  font-family: "Segoe UI", Tahoma, sans-serif !important;

`font: message-box` might make sense here (if it resolves to 'Segoe UI' by default in Windows 8), but I'm not going to worry about it for now.

Comment 22

[Jonathan Wilde [:jwilde]]

> @@ +253,5 @@
> >  
> > +      <!-- Content viewport -->
> > +      <stack>
> > +        <deck id="browsers" flex="1" observes="bcast_uidiscovery"/>
> > +        <box id="vertical-scroller" class="scroller" orient="vertical" end="2" top="0"/>
> 
> What does the end="2" do here?

It shifts the scrollbars away from the edge of the screen.  Given that we need to completely re-style these scrollbars anyway to make them feel metro-y, I'm changing "2" to "0" in my next patch.

Comment 23

[Jonathan Wilde [:jwilde]]

Comment on attachment 639561 [details] [diff] [review]
Fixed URL bar locking

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

::: browser/metro/base/content/browser-ui.js
@@ +710,5 @@
> +    Elements.tray.addEventListener("transitionend", function () {
> +      setTimeout(function (){
> +        ViewableAreaObserver.update({ force: true });
> +        BrowserUI.sizeControls();
> +      }, 0);

On release builds, resizing the controls without the setTimeout caused massive lagginess.

Comment 24

[Jonathan Wilde [:jwilde]]

Attachment: Addresses review comments, adds new features, fixes URL bar

Includes with all of the following improvements: 
- URLbar works now
- Initial loading feels faster
- Snap view implemented for URL bar, improved for appbar
- Integrated disabled state for back button
- Fixed right-click detection for ContextUI under metro mode
- Removed autocomplete XBL (which was colliding with the a11y layer, causing the URL bar to be inoperable under Metro mode)
- URL bar auto-highlights content
- Integrated suggestions from fryn

Comment 25

[Jonathan Wilde [:jwilde]]

Merged with bbondy's latest work to integrate with the find bar, snap view, etc.  Relanded. https://hg.mozilla.org/projects/elm/rev/ea88e925d52f

Comment 26

[Jonathan Wilde [:jwilde]]

Removed obsolete line.

https://hg.mozilla.org/projects/elm/rev/3651f3ed9c06

Comment 27

[Matt Brubeck (:mbrubeck)]

Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.