Closed Bug 747786 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox for Metro Graveyard :: General, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bbondy, Assigned: jwilde)

References

Details

(Keywords: feature, Whiteboard: completed-elm [metro-mvp][meta])

Attachments

(3 files, 4 obsolete files)

Attached image 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.
Attached image Tab bar
OS: Windows 8 → Windows 8 Metro
Assignee: nobody → jonathan
Status: NEW → ASSIGNED
Product: Fennec → Firefox
QA Contact: general → general
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.
Attached patch patch v0.5 (obsolete) — Splinter Review
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.
Attachment #636556 - Flags: feedback?(netzen)
Attachment #636556 - Flags: feedback?(fryn)
Also missing from above patch:
- Only show tabs button.
- Plain tabs styling without thumbnails.
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.
Attachment #636556 - Flags: feedback?(netzen)
Initial version pushed to elm (http://hg.mozilla.org/projects/elm/rev/9089bd7394c6).
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
Two other issues:
- The unified forward-back button shouldn't be animated during tab switching
- Tab open/close toggle needs to be implemented
Attachment #636556 - Flags: feedback?(fryn)
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.
:/ 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.
I get the problem on both the windows 8 Metro browser and when running firefox.exe -metrobrowser
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.
(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
Attached patch awesomebar wallpaper patch (obsolete) — Splinter Review
here's a wallpaper patch that seems to fix the problem. I've landed this on elm.
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.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #636556 - Attachment is obsolete: true
Attachment #638178 - Attachment is obsolete: true
Attachment #638949 - Flags: feedback?(netzen)
Attachment #638949 - Flags: feedback?(jmathies)
Attachment #638949 - Flags: feedback?(fryn)
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.
Attachment #638949 - Flags: feedback?(netzen)
I just tried it on my laptop so I am using a normal keyboard btw.
Depends on: 771348
Attachment #638949 - Flags: feedback?(jmathies)
Attachment #638949 - Flags: feedback?(fryn)
Attached patch Fixed URL bar locking (obsolete) — Splinter Review
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.
Attachment #638949 - Attachment is obsolete: true
Attachment #639561 - Flags: feedback?(netzen)
Attachment #639561 - Flags: feedback?(jmathies)
Attachment #639561 - Flags: feedback?(fryn)
Attachment #639561 - Flags: feedback?(netzen)
Attachment #639561 - Flags: feedback?(jmathies)
Attachment #639561 - Flags: feedback?(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.
> @@ +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 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.
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
Attachment #639561 - Attachment is obsolete: true
Attachment #639906 - Flags: feedback?(netzen)
Attachment #639906 - Flags: feedback?(fryn)
Attachment #639906 - Flags: feedback?(netzen)
Attachment #639906 - Flags: feedback?(fryn)
Merged with bbondy's latest work to integrate with the find bar, snap view, etc.  Relanded. https://hg.mozilla.org/projects/elm/rev/ea88e925d52f
Whiteboard: completed-elm
Depends on: 772304
Depends on: 772308
Depends on: 772309
Depends on: 772709
Depends on: 774857
Depends on: 774778
Depends on: 774352
Depends on: 774847
Depends on: 774850
Depends on: 774863
Depends on: 774867
Depends on: 774870
Depends on: 774872
Depends on: 774967
Depends on: 776722
Depends on: 784073
Product: Firefox → Firefox for Metro
Keywords: feature
Whiteboard: completed-elm → completed-elm [metro-mvp]
Depends on: 811390
Depends on: 811392
Depends on: 811406
Depends on: 811413
Blocks: 831671
Blocks: 803928
No longer depends on: 772308
Summary: Support new Windows 8 navigation bar and tab bar → [tracking] Support new Windows 8 navigation bar and tab bar
Whiteboard: completed-elm [metro-mvp] → completed-elm [metro-mvp][meta]
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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
No longer depends on: 811413
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.