Jared Wein [:jaws] (please needinfo? me) Reporter
	Description • 8 years ago

The Help button should be moved to the bottom left of the preferences page, as described in the attached specs.

As the page scrolls, the help button should move with the page. This can be accomplished by setting the "position:fixed" CSS property on it.

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 3 • 8 years ago

(Copied from Bug 1348740 comment #0 by Tina Hsieh[:Tina_Hsieh] UX)
> Since we have very few users clicking on the "?" (Firefox support) on every
> page of Preferences, we'd like to integrate all of them into one direct
> link:
> https://support.mozilla.org/t5/Manage-preferences-and-add-ons/tkb-p/Manage-
> Preferences
> 
> The new position of the Firefox Support will be fixed on the bottom left. 
> See more details:
> https://mozilla.invisionapp.com/share/ZDAGPK3AF#/219340982_7-1_Visual

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 10 • 8 years ago
mozreview-review

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

https://reviewboard.mozilla.org/r/124478/#review129004

::: browser/components/preferences/in-content/findInPage.js:234
(Diff revision 7)
> -        let brandName = document.getElementById("bundleBrand").getString("brandShortName");
> -        document.getElementById("need-help").innerHTML =
> -          strings.getFormattedString("searchResults.needHelp", [brandName]);
> -
> -        document.getElementById("need-help-link").setAttribute("href", getHelpLinkURL("search"));
> -      }
> +			      }

nit, indents here should use spaces not tabs. I *think* this will fail eslint with the no-mixed-spaces-and-tabs rule.

::: browser/components/preferences/in-content/preferences.xul:171
(Diff revision 7)
>             Remove this keyset once bug 1094240 ("disablefastfind" attribute
>             broken in e10s mode) is fixed. -->
>        <key key="&focusSearch1.key;" modifiers="accel" id="focusSearch1" oncommand=";"/>
>      </keyset>
>  
> +    <html:a class="help-button help-button-custom" target="_blank" aria-label="&helpButton.label;"><div>&newhelp.label;</div></html:a>

Can you try to use <html:button> here instead of <div>? Note that your use of "<div>" here is actually wrong it itself in that "<div>" is an HTML element but by not putting the namespace on it, the element will be created as a XUL element since that is the default namespace for this document.

::: browser/components/preferences/in-content/preferences.xul:171
(Diff revision 7)
>             Remove this keyset once bug 1094240 ("disablefastfind" attribute
>             broken in e10s mode) is fixed. -->
>        <key key="&focusSearch1.key;" modifiers="accel" id="focusSearch1" oncommand=";"/>
>      </keyset>
>  
> +    <html:a class="help-button help-button-custom" target="_blank" aria-label="&helpButton.label;"><div>&newhelp.label;</div></html:a>

Why do we need two CSS classes here ("help-button" and "help-button-custom")? We should only need "help-button" here unless you can find other places that "help-button" will still be used.

::: browser/components/preferences/in-content/preferences.xul:171
(Diff revision 7)
>             Remove this keyset once bug 1094240 ("disablefastfind" attribute
>             broken in e10s mode) is fixed. -->
>        <key key="&focusSearch1.key;" modifiers="accel" id="focusSearch1" oncommand=";"/>
>      </keyset>
>  
> +    <html:a class="help-button help-button-custom" target="_blank" aria-label="&helpButton.label;"><div>&newhelp.label;</div></html:a>

This is still using &helpButton.label;. Please switch both instances to use the new string. And with that, I think we shouldn't call it "newhelp" because after some time it won't be so new ;)

Can we go with just helpButton2 ?

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:26
(Diff revision 7)
>  <!ENTITY  paneUpdates.title             "Updates">
>  
>  <!-- LOCALIZATION NOTE (paneSync1.title): This should match syncBrand.fxAccount.label in ../syncBrand.dtd -->
>  <!ENTITY  paneSync1.title          "Firefox Account">
>  
>  <!ENTITY  helpButton.label        "Help">

With the new string we shouldn't need this to be defined anymore.

::: browser/themes/shared/incontentprefs/preferences.inc.css:574
(Diff revision 7)
> +  left: 15px;
> +  bottom: 10px;

I'll take another look at the necessity/correctness of these after you upload your next patch.

::: browser/themes/shared/incontentprefs/preferences.inc.css:576
(Diff revision 7)
> +  color: white;
> +  text-align: center;
> +  font-size: 14px;
> +  text-decoration: none;

Why do we need to set the color/font-size/text-decoration? I thought this was using a graphic?

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 12 • 8 years ago
mozreview-review

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

https://reviewboard.mozilla.org/r/124478/#review129116

There is no change in this patch. All I see when viewing https://reviewboard.mozilla.org/r/124478/diff/7-8/ is that the commit message has changed.

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

https://reviewboard.mozilla.org/r/124478/#review131140

You should not have a <div> (which is a block element) inside of an <a> (which is an inline element).

::: browser/components/preferences/in-content/findInPage.js:237
(Diff revisions 8 - 9)
>          document.getElementById("sorry-message").textContent =
>            strings.getFormattedString("searchResults.sorryMessage", [query]);
> +        let brandName = document.getElementById("bundleBrand").getString("brandShortName");
> +        document.getElementById("need-help").innerHTML =
> +          strings.getFormattedString("searchResults.needHelp", [brandName]);
>  			}

The indentation here is still busted.

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 15 • 8 years ago
mozreview-review

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

https://reviewboard.mozilla.org/r/124478/#review131142

::: browser/components/preferences/in-content/preferences.js:148
(Diff revision 9)
>    if (!item) {
>      category = kDefaultCategoryInternalName;
>      item = categories.querySelector(".category[value=" + category + "]");
>    }
>  
> +  // Adding Help button features

This comment should be removed.

::: browser/components/preferences/in-content/preferences.xul:171
(Diff revision 9)
>             Remove this keyset once bug 1094240 ("disablefastfind" attribute
>             broken in e10s mode) is fixed. -->
>        <key key="&focusSearch1.key;" modifiers="accel" id="focusSearch1" oncommand=";"/>
>      </keyset>
>  
> +    <html:a class="help-button" target="_blank" aria-label="&helpButton2.label;"><div>&helpButton2.label;</div></html:a>

Why do you need the extra nesting of the <div> here at all? Can you just put the text inside of the <a> and put some padding-inline-start on the <a>?

I remember you had an issue with the text wrapping, and the <div> worked around that? I think you need to look deeper in to what was going on there.

	Manotej Meka (:manotejmeka)
	Comment 16 • 8 years ago
mozreview-review-reply

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

https://reviewboard.mozilla.org/r/124478/#review129004

> This is still using &helpButton.label;. Please switch both instances to use the new string. And with that, I think we shouldn't call it "newhelp" because after some time it won't be so new ;)
> 
> Can we go with just helpButton2 ?

Just made it to be $helpButtonPreferences.label = "Firefox Support", because it seems to be used at other places.

> Can you try to use <html:button> here instead of <div>? Note that your use of "<div>" here is actually wrong it itself in that "<div>" is an HTML element but by not putting the namespace on it, the element will be created as a XUL element since that is the default namespace for this document.

I will leave this unfixed and untouched for you to look again again when you I push for review.

> With the new string we shouldn't need this to be defined anymore.

In this case I will just make the other one be helpButton.label

> I'll take another look at the necessity/correctness of these after you upload your next patch.

Just did this so that it would be binded to the bottom left corner.

> Why do we need to set the color/font-size/text-decoration? I thought this was using a graphic?

Please explain further more.

	Manotej Meka (:manotejmeka)
	Comment 17 • 8 years ago
mozreview-review-reply

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

https://reviewboard.mozilla.org/r/124478/#review131140

> The indentation here is still busted.

The code looks fine on my laptop. Do you see this issue if you pull it. I don't think I see the issue if I pulled it but I can double check

	Mike Conley (:mconley) (:⚙️)
	Comment 18 • 8 years ago
mozreview-review

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

https://reviewboard.mozilla.org/r/124478/#review131518

::: browser/components/preferences/in-content/findInPage.js:237
(Diff revision 9)
>          let brandName = document.getElementById("bundleBrand").getString("brandShortName");
>          document.getElementById("need-help").innerHTML =
>            strings.getFormattedString("searchResults.needHelp", [brandName]);
> -
> -        document.getElementById("need-help-link").setAttribute("href", getHelpLinkURL("search"));
> -      }
> +			      }

Please revert this indentation.

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:26
(Diff revision 9)
>  <!ENTITY  paneUpdates.title             "Updates">
>  
>  <!-- LOCALIZATION NOTE (paneSync1.title): This should match syncBrand.fxAccount.label in ../syncBrand.dtd -->
> -<!ENTITY  paneSync1.title          "Firefox Account">
> +<!ENTITY  paneSync1.title               "Firefox Account">
>  
> -<!ENTITY  helpButton.label        "Help">
> +<!ENTITY  helpButton2.label              "Firefox Support">

Sanity check - are we certain this isn't supposed to be a &brandShortName; usage? Maybe that's more of a question for jaws or UX.

::: browser/themes/shared/incontentprefs/preferences.inc.css:572
(Diff revision 9)
>    .fxaFirefoxLogo {
>      list-style-image: url(chrome://browser/skin/fxa/logo@2x.png);
>    }
>  }
> +
> +.help-button:link, .help-button:visited{

Please break the selectors over two lines, and put a space before {.

::: browser/themes/shared/incontentprefs/preferences.inc.css:582
(Diff revision 9)
> +  text-align: center;
> +  font-size: 14px;
> +  text-decoration: none;
> +}
> +
> +.help-button > div{

Space before {

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 19 • 8 years ago

(In reply to Mike Conley (:mconley) - Review / needinfo queue at max. from comment #18)
> ::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:26
> (Diff revision 9)
> >  <!ENTITY  paneUpdates.title             "Updates">
> >  
> >  <!-- LOCALIZATION NOTE (paneSync1.title): This should match syncBrand.fxAccount.label in ../syncBrand.dtd -->
> > -<!ENTITY  paneSync1.title          "Firefox Account">
> > +<!ENTITY  paneSync1.title               "Firefox Account">
> >  
> > -<!ENTITY  helpButton.label        "Help">
> > +<!ENTITY  helpButton2.label              "Firefox Support">
> 
> Sanity check - are we certain this isn't supposed to be a &brandShortName;
> usage? Maybe that's more of a question for jaws or UX.

Definitely should be &brandShortName;. Thank you for catching that.

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 21 • 8 years ago
mozreview-review

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

https://reviewboard.mozilla.org/r/124478/#review132034

Mostly good, just some styling changes and hiding the text when the window is smaller.

::: browser/components/preferences/in-content/preferences.xul:171
(Diff revision 10)
>             Remove this keyset once bug 1094240 ("disablefastfind" attribute
>             broken in e10s mode) is fixed. -->
>        <key key="&focusSearch1.key;" modifiers="accel" id="focusSearch1" oncommand=";"/>
>      </keyset>
>  
> +    <html:a class="help-button" target="_blank" aria-label="&helpButton2.label;">&helpButton2.label;</html:a>

In the media-query for smaller screens, when we hide the category labels we should also be hiding the helpButton label. You can do this by setting font-size to 0 on .help-button.

You should add this at http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/browser/themes/shared/incontentprefs/preferences.inc.css#96

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:24
(Diff revision 10)
>  <!ENTITY  panePrivacySecurity.title     "Privacy &amp; Security">
>  <!ENTITY  paneContainers.title          "Container Tabs">
>  <!ENTITY  paneUpdates.title             "Updates">
>  
>  <!-- LOCALIZATION NOTE (paneSync1.title): This should match syncBrand.fxAccount.label in ../syncBrand.dtd -->
> -<!ENTITY  paneSync1.title          "Firefox Account">
> +<!ENTITY  paneSync1.title               "Firefox Account">

nit, please undo the spacing changes to paneSync1.title and searchInput.label lines.

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:26
(Diff revision 10)
>  <!ENTITY  paneUpdates.title             "Updates">
>  
>  <!-- LOCALIZATION NOTE (paneSync1.title): This should match syncBrand.fxAccount.label in ../syncBrand.dtd -->
> -<!ENTITY  paneSync1.title          "Firefox Account">
> +<!ENTITY  paneSync1.title               "Firefox Account">
>  
> -<!ENTITY  helpButton.label        "Help">
> +<!ENTITY  helpButton2.label              "Firefox Support">

nit, please fix the alignment with the strings above and below

::: browser/themes/shared/incontentprefs/preferences.inc.css:561
(Diff revision 10)
>  }
> +
> +.help-button {
> +  position: fixed;
> +  left: 0;
> +  bottom: 10px;

bottom: 39px;
so it matches the padding-top of #categories

::: browser/themes/shared/incontentprefs/preferences.inc.css:562
(Diff revision 10)
> +
> +.help-button {
> +  position: fixed;
> +  left: 0;
> +  bottom: 10px;
> +  text-align: center;

remove the text-align

::: browser/themes/shared/incontentprefs/preferences.inc.css:563
(Diff revision 10)
> +.help-button {
> +  position: fixed;
> +  left: 0;
> +  bottom: 10px;
> +  text-align: center;
> +  font-size: 14px;

font-size: 13px;
line-height: 13px;
height: 14px;
background-position: 15px;

::: browser/themes/shared/incontentprefs/preferences.inc.css:564
(Diff revision 10)
> +  position: fixed;
> +  left: 0;
> +  bottom: 10px;
> +  text-align: center;
> +  font-size: 14px;
> +  padding-inline-start: 45px;

padding-inline-start: 35px;

The padding is because we want 15px of gap between the edge of the window and the icon (which we use background-position: 15px; to move the icon 15px in), the image is 14px wide and a 6px gap between the text and the image looks about right.

::: browser/themes/shared/incontentprefs/preferences.inc.css:568
(Diff revision 10)
> +  font-size: 14px;
> +  padding-inline-start: 45px;
> +  white-space: nowrap;
> +}
> +
> +.help-button:link, .help-button:visited {

nit, please put all selectors on their own line

::: browser/themes/shared/incontentprefs/preferences.inc.css:569
(Diff revision 10)
> +  padding-inline-start: 45px;
> +  white-space: nowrap;
> +}
> +
> +.help-button:link, .help-button:visited {
> +  color: white;

color: var(--in-content-category-text);

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 25 • 8 years ago
mozreview-review

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

https://reviewboard.mozilla.org/r/124478/#review132774

Looks good, but still need to fix the brandShortName issue below that mconley had pointed out.

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd:26
(Diff revision 12)
>  <!ENTITY  paneUpdates.title             "Updates">
>  
>  <!-- LOCALIZATION NOTE (paneSync1.title): This should match syncBrand.fxAccount.label in ../syncBrand.dtd -->
>  <!ENTITY  paneSync1.title          "Firefox Account">
>  
> -<!ENTITY  helpButton.label        "Help">
> +<!ENTITY  helpButton2.label        "Firefox Support">

This needs to be "&brandShortName; Support" so that if someone repackages Firefox and calls it something else then the name will get updated.

::: browser/themes/shared/incontentprefs/preferences.inc.css:573
(Diff revision 12)
> +  background-position: 15px;
> +  padding-inline-start: 35px;
> +  white-space: nowrap;
> +}
> +
> +.help-button:link, 

nit, trailing whitespace

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 26 • 8 years ago

Hey Ian, are you planning on fixing the issue above? Note you will need to rebase this patch most likely.

	Mike Conley (:mconley) (:⚙️)
	Comment 27 • 8 years ago
mozreview-review

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

https://reviewboard.mozilla.org/r/124478/#review133920

Clearing r? - jaws' review is solid.

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 29 • 8 years ago

Comment on attachment 8852250 [details]
Bug 1349744 - Help button moved to bottom left corner.

I have fixed up the remaining issues on the patch, as well as adding support in init_dynamic_padding.

	Mike Conley (:mconley) (:⚙️)
	Comment 31 • 8 years ago
mozreview-review

Comment on attachment 8859685 [details]
Bug 1349744 - Move help button to the bottom-left of the preferences per spec.

https://reviewboard.mozilla.org/r/131700/#review134584

Thanks for the great commentary. Tested, and works as advertised.

	Pulsebot
	Comment 32 • 8 years ago

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e057f7f9bc3c
Move help button to the bottom-left of the preferences per spec. r=mconley

	Carsten Book [:Tomcat]
	Comment 33 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e057f7f9bc3c

	Saheda Reza [:Antora]
	Comment 34 • 8 years ago

I have reproduced this bug issue on Firefox nightly according to (2017-03-22)

Fixing bug is verified on latest Nightly-- Build ID:( 20170422030205 ),User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

Tested OS -- Windows7 64bit
 [bugday-20170419]

	Camelia Badau [:cbadau], Desktop QA
	Comment 35 • 7 years ago

Verified fixed on Windows 7 x64, Windows 10 x86, Mac OSX 10.12.4 and Ubuntu 16.04 x64 using latest Nightly 55.0a1 (2017-05-10).