Closed Bug 807691 Opened 9 years ago Closed 9 years ago

Work - Replace "About Firefox" page with a Metro-style sidebar UI

Categories

(Firefox for Metro Graveyard :: Shell, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mbrubeck, Assigned: bbondy)

References

Details

(Keywords: polish, ux-tone, Whiteboard: feature=work [completed-elm])

Attachments

(5 files, 4 obsolete files)

Attached image rough mockup (obsolete) —
Choosing "About Firefox" under Settings should display information about the application in a overlay that slides in from the right side of the screen, like in the attached mockup created by Yuan.

Asa noted that we should keep the content in this overlay very minimal.  (Users can always go to the desktop UI for more details.)  Asa, would you like to take a stab at the final content and copy for our About Firefox on Metro?
Flags: needinfo?(asa)
Keywords: polish
Whiteboard: [metro-mvp?][LOE:1]
I like the rough mockup but I think it's too much stuff. How about this:

"Firefox" (in big logo font just like you have it in the mockup)
"By Mozilla®" (regular text. also don't say "corporation")
<version_number> in plain text
"Your Firefox is up to date" text or "Restart to update" button if update is pending
"You are using the <channel_name>" (if other than release. empty if release)
<link>"Read the Firefox privacy policy online"</link>
Flags: needinfo?(asa) needinfo?(asa) → needinfo+
Whiteboard: [metro-mvp?][LOE:1] → [metro-mvp][LOE:1][metro-it2]
Assignee: nobody → ywang
Keywords: uiwanted
This removes all the code and strings for the old Fennec "about:firefox" page, since we have no plan to use it in Metro.
Assignee: ywang → mbrubeck
Status: NEW → ASSIGNED
Attachment #699423 - Flags: review?(mark.finkle)
Comment on attachment 699423 [details] [diff] [review]
part 1: remove old code

We don't plan to have an about:firefox page? Or we'll just use the desktop version?
Attachment #699423 - Flags: review?(mark.finkle) → review+
Comment on attachment 699423 [details] [diff] [review]
part 1: remove old code

https://hg.mozilla.org/projects/elm/rev/d3b1e85d75b4

(In reply to Mark Finkle (:mfinkle) from comment #3)
> We don't plan to have an about:firefox page? Or we'll just use the desktop
> version?

Instead of a page we plan to use a sidebar, as in attachment 677443 [details].  We also have the desktop "about:" page for anyone who goes looking for it.  "about:firefox" was always a Fennec-only thing.
Depends on: 831955
Assignee: mbrubeck → netzen
Blocks: 831955
No longer depends on: 831955
Whiteboard: [metro-mvp][LOE:1][metro-it2] → [metro-mvp][LOE:1][metro-it2] feature=work
Summary: Replace "About Firefox" page with a Metro-style sidebar UI → Work - Replace "About Firefox" page with a Metro-style sidebar UI
Whiteboard: [metro-mvp][LOE:1][metro-it2] feature=work → feature=work
Attached image Bottom image from Yuan
Attachment #677443 - Attachment is obsolete: true
Attachment #705321 - Attachment description: Design from Yuan → Bottom image from Yuan
Some extra info from Yuan:

> This is what we have for the content of start page:
> http://cl.ly/image/2i430p08163Y
>
> We are awaiting a visual reference guide from Stephen. 
> Also, we will probably get a new logo for Metro Firefox.
>
> So I would suggest get the basic info into the About flyout, 
> but not apply the visual style or logo at this point.
>
> Basic style you could use for this About flyout:
> Full screen height;
> Width: 346px;
> Header text for "About": Segoe UI Light, 20pt;
> Body text: Segoe UI Regular, 11pt;
Attached image Reference UI
Attaching reference image in case the link ever becomes invalid.
Note about animations of flyout panel: 
Panel slide in: 
http://msdn.microsoft.com/en-us/library/windows/apps/br230467.aspx
Panel slide out: 
http://msdn.microsoft.com/en-us/library/windows/apps/br212677.aspx

Most Windows store app has this for edge panel. We should either import from MS code base or implement animations by ourselves. Please let me know if that makes sense from implementation perspective.
We'll just use CSS to do the animations, for this about flyout though we want to go back to the metro settings flyout. And there's a bug we have for when Metro UI is shown our code doesn't run at all.  So I'm not going to do the animation in this bug for this iteration.
That work is being donee in bug 817641
Attached patch WIP (obsolete) — Splinter Review
Works but I need to load some labels dynamically like the version number. And the privacy URL from a pref. I think I'm also going to refactor the flyout as an XBL control.
Attached patch Patch v1 - Front end. (obsolete) — Splinter Review
Attachment #706360 - Attachment is obsolete: true
Attachment #706718 - Flags: review?(mbrubeck)
This is code to activate the Metro settings flyout panel.  There's a back button on the Firefox about panel and the Firefox options pane that will also use it.
Attachment #706719 - Flags: review?(jmathies)
Attachment #706719 - Flags: review?(jmathies) → review+
Comment on attachment 706718 [details] [diff] [review]
Patch v1 - Front end.

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

Looks great; I'd just like one more chance to review it with the comments below addressed.

::: browser/metro/base/content/bindings/flyoutpanel.xml
@@ +47,5 @@
> +
> +      <property name="isVisible" readonly="true">
> +        <getter>
> +          <![CDATA[
> +            return this.getAttribute("visible") == "true";

Minor suggestion: Let's just 'return this.hasAttribute("visible")' here.  Similarly in the CSS we can just use [visible] instead of [visible=true].

(Rationale: Conciseness, and it saves the reader from wondering whether any other values are possible.)

@@ +52,5 @@
> +          ]]>
> +        </getter>
> +      </property>
> +      
> +      <method name="dismiss">

Could you call this "hide" instead (purely for consistency with other show/hide pairs in our UI code)?

@@ +68,5 @@
> +          ]]>
> +        </body>
> +      </method>
> +
> +      <method name="toggle">

This seems to be unused, unless I missed something.  If so, let's just drop it unless/until there's a use case for it.

@@ +71,5 @@
> +
> +      <method name="toggle">
> +        <body>
> +          <![CDATA[
> +            if (this.getAttribute("visible") === "true") {

if (this.isVisible)

::: browser/metro/base/content/browser.css
@@ +239,5 @@
> +
> +#about-flyoutpanel {
> +  width: 350px;
> +  background-image:url('chrome://browser/skin/images/about-footer.png');
> +  background-repeat: no-repeat;

This stuff should go in theme/browser.css (confusing, I know).

::: browser/metro/base/content/browser.xul
@@ +465,5 @@
> +  <flyoutpanel id="about-flyoutpanel" headertext="&aboutHeader.label;">
> +        <label id="about-product-label" value="&aboutHeaderProduct.label;"/>
> +        <label value="&aboutHeaderCompany.label;"/>
> +#expand <label id="about-version-label">__MOZ_APP_VERSION__</label>
> +        <label value="&update.noUpdatesFound;"/>

Let's just leave out the update message until we have actual update functionality here.  (Please file a follow-up bug and we'll get Asa's decision on whether/where to put the update check in Metro.)

::: browser/metro/base/content/helperui/FindHelperUI.js
@@ +55,5 @@
>      let json = aMessage.json;
>      switch(aMessage.name) {
>        case "FindAssist:Show":
>          ContextUI.dismiss();
> +        FlyoutPanelsUI.hide();

I don't like the way this is growing... we have N different bits of overlay UI, each of which has to potentially hide itself when one of the other (N-1) is displayed.  We should come up with a single notification or event or dispatcher to handle this, instead of a growing set of special cases in every UI controller.

You don't need to solve that now (though you can if you want to!); just a reminder to myself to follow up on this.

::: browser/metro/theme/platform.css
@@ +525,5 @@
> +  height: 100%;
> +  border-width: 2px;
> +  border-color: #d7d6d6;
> +  background-color: #ffffff;
> +  border-left-style: solid;

Please use -moz-border-start-style instead (for RTL friendliness).

@@ +529,5 @@
> +  border-left-style: solid;
> +  visibility: collapse;
> +  position: fixed;
> +  bottom: 0;
> +  right: 0;

...and we'll need separate flyoutpanel:-moz-dir(ltr) and flyoutpanel:-moz-dir(rtl) rules, with right:0 and left:0 respectively.

@@ +533,5 @@
> +  right: 0;
> +  -moz-transition: -moz-transform 0.2s ease-out;
> +}
> +
> +[anonid="flyout-container"] {

For performance reasons, please add classes to all the anonymous elements in the flyoutpanel binding, and use class selectors instead of attribute selectors or element type selectors to style them.

Also note that you can add a <stylesheet> element to the XBL file, if you want to separate these out into a separate CSS stylesheet that applies only within the binding.

@@ +547,5 @@
> +  background-color: #002147;
> +  height: 80px;
> +  width: 348px;
> +  color: #ffffff;
> +  font-family: Segoe UI;

I think this is a syntax error (needs quotes around "Segoe UI"?), and anyway it is not needed because it should inherit the "Segoe UI" font-family from the root element.

@@ +568,5 @@
> +
> +[anonid="flyout-close-button"] {
> +  border: 0 none;
> +  -moz-appearance: none;
> +  list-style-image: url(chrome://browser/skin/images/flyout-back-button.png);

Eventually we will need an RTL version of the back button too, but not now. I'll wait and bug shorlander for flipped versions of all our images once we the UI is closer to final.  (No action needed; just another note to myself.)
Attachment #706718 - Flags: review?(mbrubeck) → review-
Comment on attachment 706718 [details] [diff] [review]
Patch v1 - Front end.

>diff --git a/objdir/dist/bin/metro/chrome/chrome/skin/images/about-footer.png b/objdir/dist/bin/metro/chrome/chrome/skin/images/about-footer.png
>new file mode 100644

You accidentally added the images in the objdir instead of the srcdir.
Comment on attachment 706718 [details] [diff] [review]
Patch v1 - Front end.

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

::: browser/metro/theme/jar.mn
@@ +40,5 @@
>    skin/images/identity-icons-https-mixed.png  (images/identity-icons-https-mixed.png)
>    skin/images/identity-icons-https.png      (images/identity-icons-https.png)
>    skin/images/firefox-watermark.png         (images/firefox-watermark.png)
> +  skin/flyout-back-button.png               (images/flyout-back-button.png)
> +  skin/about-footer.png                     (images/about-footer.png)

Note: "skin" should be "skin/images" here.
Attached patch Patch v2. (obsolete) — Splinter Review
Thanks for the review!

The stuff in Find UI for hiding the panel wasn't even needed so I removed it.  I posted bug 835151 for the update functionality.
Attachment #706718 - Attachment is obsolete: true
Attachment #706885 - Flags: review?(mbrubeck)
Comment on attachment 706885 [details] [diff] [review]
Patch v2.

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

Awesome, thanks.  r=mbrubeck with just a few more CSS tweaks.

Note: The image files are still in the objdir.

::: browser/metro/base/content/bindings/flyoutpanel.xml
@@ +65,5 @@
> +
> +      <method name="show">
> +        <body>
> +          <![CDATA[
> +            this.setAttribute("visible", "");

Let's continue using setAttribute("visible", "true") here even though we don't depend on the value anymore -- I do think it makes this line clearer.

::: browser/metro/locales/en-US/chrome/preferences.dtd
@@ +4,5 @@
>  
>  <!ENTITY prefsHeader.label                         "Preferences">
> +<!ENTITY aboutHeader.label                         "About">
> +<!ENTITY aboutHeaderProduct.label                  "&brandShortName;">
> +<!ENTITY aboutHeaderCompany.label                  "By &vendorShortName; &#174;">

One thing I missed before:  I don't think the "®" should be here; we don't usually write "Mozilla®" in our products or web sites.  But if it is here, it should probably be part of the vendorShortName entity (since it may not apply to other vendors) and there should not be a space before it.

::: browser/metro/theme/flyoutpanel.css
@@ +10,5 @@
> +  -moz-border-start-style: solid;
> +  visibility: collapse;
> +  position: fixed;
> +  -moz-transition: -moz-transform 0.2s ease-out;
> +  font-family: "Segoe UI";

Is the font-family property needed here? We set "Segoe UI" as the default font-family for the whole XUL document.

@@ +16,5 @@
> +  right: 0;
> +}
> +
> +flyoutpanel:-moz-dir(rtl) {
> +  left: 0;

You'll also need to set "right: auto;" here to override the "right: 0;" from above.

(You can set intl.uidirection.en = "rtl" in about:config if you want to test this, though at the moment we have all sorts of problems with RTL layout.)

@@ +35,5 @@
> +
> +.flyoutpanel-header toolbarbutton {
> +  margin-top: 30px !important;
> +  margin-left: 40px !important;
> +}

Please use classes for the toolbarbutton and label too. Rationale:
https://developer.mozilla.org/en-US/docs/CSS/Writing_efficient_CSS#Avoid_the_descendant_selector.21

Use "-moz-margin-start" instead of margin-left, and remove "!important" unless it's necessary (I don't think it should be).

@@ +40,5 @@
> +
> +.flyoutpanel-header label {
> +  margin-top: 30px !important;
> +  margin-left: 10px !important;
> +}

Same changes here, please.

@@ +42,5 @@
> +  margin-top: 30px !important;
> +  margin-left: 10px !important;
> +}
> +
> +flyoutpanel .flyoutpanel-header .close-button {

Just use ".close-button" as the selector here, please.

::: browser/metro/theme/jar.mn
@@ +7,5 @@
>  chrome.jar:
>  % skin browser classic/1.0 %skin/
>    skin/aboutPage.css                        (aboutPage.css)
>    skin/about.css                            (about.css)
> +  skin/flyoutpanel.css

I think there should be a "(flyoutpanel.css)" at the end of this line.
Attachment #706885 - Flags: review?(mbrubeck) → review+
Thanks for the review. Fixed review comments.
Carrying forward r+ but let me know if you need anything else, I'm going to pull in recent changes and rebuild before pushing to elm.
Attachment #706885 - Attachment is obsolete: true
Attachment #706908 - Flags: review+
https://hg.mozilla.org/projects/elm/rev/a034278d403e
https://hg.mozilla.org/projects/elm/rev/5bbfc495ad19
OS: Windows 7 → Windows 8 Metro
Whiteboard: feature=work → feature=work [completed-elm]
Depends on: 835615
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: 9 years ago
Resolution: --- → FIXED
Depends on: 843742
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.