Move the Help button to the bottom left of the Preferences page

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Preferences
VERIFIED FIXED
5 months ago
3 months ago

People

(Reporter: jaws, Assigned: Ian Ferguson)

Tracking

(Blocks: 1 bug)

52 Branch
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 8850214 [details]
Page-Shot-2017-3-22 General-Reorg.png

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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(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
(Reporter)

Updated

5 months ago
Duplicate of this bug: 1348740
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 10

5 months 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?
Attachment #8852250 - Flags: review?(jaws) → review-
(Reporter)

Updated

5 months ago
Attachment #8852250 - Flags: review?(mconley)
Comment hidden (mozreview-request)
(Reporter)

Comment 12

5 months 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.
Attachment #8852250 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 14

4 months 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.
Attachment #8852250 - Flags: review?(jaws) → review-
(Reporter)

Comment 15

4 months 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.

Comment 16

4 months 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.

Comment 17

4 months 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

Comment 18

4 months 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 {
Attachment #8852250 - Flags: review?(mconley) → review-
(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.
Comment hidden (mozreview-request)
(Reporter)

Comment 21

4 months 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);
Attachment #8852250 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 25

4 months 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
Attachment #8852250 - Flags: review?(jaws) → review-
Hey Ian, are you planning on fixing the issue above? Note you will need to rebase this patch most likely.
Flags: needinfo?(xfergusi)

Comment 27

4 months 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.
Attachment #8852250 - Flags: review?(mconley)
Comment hidden (mozreview-request)
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.
Attachment #8852250 - Attachment is obsolete: true
Flags: needinfo?(xfergusi)
(Reporter)

Updated

4 months ago
Attachment #8857685 - Attachment is obsolete: true
(Reporter)

Updated

4 months ago
Depends on: 1357841
Comment hidden (mozreview-request)

Comment 31

4 months 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.
Attachment #8859685 - Flags: review?(mconley) → review+

Comment 32

4 months 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

Comment 33

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e057f7f9bc3c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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]
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).
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.