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

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Shell, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: rough mockup

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?

Comment 1

[Asa Dotzler [:asa]]

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>

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: remove old code

This removes all the code and strings for the old Fennec "about:firefox" page, since we have no plan to use it in Metro.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 4

[Matt Brubeck (:mbrubeck)]

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.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Bottom image from Yuan

Comment 6

[Brian R. Bondy [:bbondy]]

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;

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Reference UI

Attaching reference image in case the link ever becomes invalid.

Comment 8

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

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.

Comment 9

[Brian R. Bondy [:bbondy]]

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.

Comment 10

[Brian R. Bondy [:bbondy]]

That work is being donee in bug 817641

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: WIP

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.

Comment 12

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Front end.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Widget code

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.

Comment 14

[Matt Brubeck (:mbrubeck)]

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.)

Comment 15

[Matt Brubeck (:mbrubeck)]

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 16

[Matt Brubeck (:mbrubeck)]

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.

Comment 17

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

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.

Comment 18

[Matt Brubeck (: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.

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Front end code

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.

Comment 20

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/projects/elm/rev/a034278d403e
https://hg.mozilla.org/projects/elm/rev/5bbfc495ad19

Comment 21

[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.