989890 - Port the help button to in-content prefs

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[u428464]

It would be nice to add the help button to the in-content preferences. It is in current preferences a button with text but the in-content specs shows a yellow icon : https://bug738796.bugzilla.mozilla.org/attachment.cgi?id=8344776

Comment 1

[u428464]

Just to precise the help button appears on every pane, but also in submenus.

Comment 2

[u428464]

The Help button can be seen on these screens at the left bottom : http://people.mozilla.org/~jboriss/temp/preferences_expanded.jpg

Comment 3

[Richard Marti (:Paenglab)]

Attachment: inContentHelp.patch

Missing is now the helpTopic="prefs-tabs" as this is merged in in-content prefs to the main pane. I think it looks weird with two help buttons on this pane and I propose a new helpTopic like "prefs-main-tab" or "prefs-ic-main" with both topics merged in one help page.

I don't know what is needed to create such a page and redirect. Whoever knows this, please file bugs on the correct components.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8409599 [details] [diff] [review]
inContentHelp.patch

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

As far as merging the two pages, you will want to talk to :sheppy. You can help :sheppy out by creating a new MDN page that has the contents of the two aforementioned pages merged.

::: browser/components/preferences/in-content/advanced.xul
@@ +211,5 @@
>        </groupbox>
>  #endif
> +
> +      <hbox  pack="end">
> +        <button label="?"

The help button should use a graphic instead of a button made to look like this. Doing so will make it easier for themes to change the look of the preferences without having to mess with changing the actual labels of the button.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Mike, can you provide the help button graphics for this? As usual, we'll also need a HiDPI graphic too. Thanks!

Comment 6

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: help-glyph.zip

CSS Properties & Values:

button.help-incontent {
    border-radius: 2px;
    background: #FFCB00;
    border: 1px solid #C1C1C1;
    width: 30px;
    height: 30px;
}

button.help-incontent:hover {
    background: #F4C200;
}

button.help-incontent:active {
    background: #EABA00;
}

Comment 7

[Richard Marti (:Paenglab)]

Attachment: inContentHelp.patch v2

Now with the help-glyph. I still have a box-shadow on the help button, is this okay? This makes the button look more like a button.

I removed the label="?" completely, is this okay or should I leave a empty label?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8412779 [details] [diff] [review]
inContentHelp.patch v2

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

Super super sorry about the slow reviews here.

::: browser/components/preferences/in-content/advanced.xul
@@ +210,5 @@
>          </hbox>
>        </groupbox>
>  #endif
> +
> +      <hbox  pack="end">

nit, please only have one space between 'hbox' and 'pack'.

@@ +212,5 @@
>  #endif
> +
> +      <hbox  pack="end">
> +        <button class="help-button"
> +                oncommand="openHelpLink('prefs-advanced-general', '', 'tab');"/>

We should use an 'aria-label' attribute here with "Help" as the value (a localized string). See https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-label_attribute for more information.

::: browser/themes/shared/incontentprefs/preferences.css
@@ +188,5 @@
> +  min-width: 30px;
> +  border-radius: 2px;
> +  border-color: #C1C1C1;
> +  background-color: #FFCB00;
> +  background-image: url("chrome://browser/skin/preferences/in-content/help-glyph.png") !important;

Why is important needed here?

@@ +194,5 @@
> +}
> +
> +@media (min-resolution: 2dppx) {
> +  .help-button {
> +    background-size: cover;

This should be 'background-size: Xpx' where X is the width of the background-image in 1dppx resolution. This is more in line with our current convention of @2x images.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: inContentHelp.patch v3

Comments addressed.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8416872 [details] [diff] [review]
inContentHelp.patch v3

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

Hello Richard,

Have you considered having one help-button in preferences.xul that opens the help page for the selected category? I believe this is more like how the existing pref dialog works and it's cleaner than duplicating the markup in every category IMO.

Comment 11

[Richard Marti (:Paenglab)]

First, I wouldn't know how. My knowledge on this is to low.

But I think this isn't easily doable because this in-content prefs is a huge XUL file where all parts are hidden and only the needed parts are shown. The Dialog prefs do have a fixed dialog buttons area where the help button lies. On in-content prefs the help button is always at the end of each sub page. And this pages have also a different length, which would make the positioning hard.

If someone can point me to a example, I can try to do this. But again, my knowledge is limited here and someone other should take this bug, if it's doable this way.

Comment 12

[Matthew N. [:MattN]]

You can either put the help button at the bottom of the prefpane[1] and update the code so it shows on all applicable categories/subcategories (preferred) or you can place it directly below the </prefpane> but I'm not sure you will get the correct positioning.

[1] https://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.xul?rev=a1f0c9c0e7e2&mark=137#126

Comment 13

[Richard Marti (:Paenglab)]

Put on bottom the button is shown correctly but after switching the category it's hidden by this code: https://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js#54

As I wrote before, my JS knowledge is really low and I don't know how to remove the help button's hidden attribute with a clean code. The same is for exchanging the help button target page.

Comment 14

[Richard Marti (:Paenglab)]

Oh, I forgot to relate the bug for the help page merging.

Comment 15

[Matthew N. [:MattN]]

(In reply to Richard Marti (:Paenglab) from comment #13)
> Put on bottom the button is shown correctly but after switching the category
> it's hidden by this code:
> https://mxr.mozilla.org/mozilla-central/source/browser/components/
> preferences/in-content/preferences.js#54

Right, did you try my alternative approach which doesn't require this change? If it give the proper positioning, I think it would also be okay.
 
> As I wrote before, my JS knowledge is really low and I don't know how to
> remove the help button's hidden attribute with a clean code. The same is for
> exchanging the help button target page.

One way could be to use the attribute value "ALL" which makes it show on every page:
element.hidden = (attributeValue != aQuery && attributeValue != "ALL");

Perhaps a better solution would be to not hide elements lacking the attribute:
element.hidden = (attributeValue && attributeValue != aQuery);

Note that I haven't tested either of the above.

For showing the right help page, just have the help button always call the same function and that function would check categories.selectedItem and e.g. advancedPrefs.selectedTab.helpTopic (which already has the helpTopic IDs[1]).

[1] https://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/advanced.xul?rev=e2c046c72d6c#136

Comment 16

[Matthew N. [:MattN]]

Sorry, helpTopic may just be a normal attribute without a getter: advancedPrefs.selectedTab.getAttribute("helpTopic")

Comment 17

[Richard Marti (:Paenglab)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #15)
> (In reply to Richard Marti (:Paenglab) from comment #13)
> > Put on bottom the button is shown correctly but after switching the category
> > it's hidden by this code:
> > https://mxr.mozilla.org/mozilla-central/source/browser/components/
> > preferences/in-content/preferences.js#54
> 
> Right, did you try my alternative approach which doesn't require this
> change? If it give the proper positioning, I think it would also be okay.

Yes, I tried it, but it placed the button on the right of the panes.

> Perhaps a better solution would be to not hide elements lacking the
> attribute:
> element.hidden = (attributeValue && attributeValue != aQuery);

Great, this works.

> For showing the right help page, just have the help button always call the
> same function and that function would check categories.selectedItem and e.g.
> advancedPrefs.selectedTab.helpTopic (which already has the helpTopic IDs[1]).

Only advanced prefs is using tabs but I check if I can use the categories to select the topic. The different help-topics of advanced prefs are all showing the same page without scrolling to an anchor, thus the category should be enough.

Thanks a lot for your help.

Comment 18

[Richard Marti (:Paenglab)]

Attachment: inContentHelp.patch v4

Now with Matthew's proposal of one help button. I hope all in my new function openHelpPage() is correct.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8417023 [details] [diff] [review]
inContentHelp.patch v4

>+function openHelpPage() {

Can you rename this to helpButtonCommand or something like that? openHelpPage is confusingly close to openHelpLink.

>+  openHelpLink(helpLink, '', 'tab');

nit: use double quotes

Comment 20

[Richard Marti (:Paenglab)]

Attachment: inContentHelp.patch v5

Dão's comments applied.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8417253 [details] [diff] [review]
inContentHelp.patch v5

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

This is close, hopefully only one more pass and we'll be good-to-go. Thanks for sticking with this.

::: browser/themes/shared/incontentprefs/preferences.css
@@ +185,5 @@
>  }
>  
> +.help-button,
> +.help-button:not([disabled="true"]):hover,
> +.help-button:not([disabled="true"]):hover:active {

It stinks that we need these last two selectors here so that the background-image doesn't get overriden by the default button:not([disabled]):hover and :hover:active's linear-gradient background-image.

But I also see that as a something wrong with the design of this help button. The box-shadow that is added here has the wrong shadow location for the :hover:active state (the inner shadow should be adjusted), and it also doesn't match the look-and-feel of the other buttons within the in-content preferences.

So, this is what we should do:

1) Add border-width: 1px; to this rule, since it is missing and should be declared here explicitly.
2) Remove the box-shadow.
3) Remove the background-image.
4) Add the following ruleset:
.help-button > .button-box > .button-icon {
  width: 30px;
  height: 30px;
  background-image: url("chrome://browser/skin/preferences/in-content/help-glyph.png");
  background-position: center;
}
5) Add a similar ruleset for 2dppx.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8417253 [details] [diff] [review]
inContentHelp.patch v5

>+function helpButtonCommand() {
>+  let pane = history.state;
>+  let categories = document.getElementById("categories");
>+  let helpLink = categories.querySelector(".category[value=" + pane + "]")
>+                           .getAttribute("helpTopic");
>+  openHelpLink(helpLink, "", "tab");
>+}

openHelpLink's second argument (aCalledFromModal) is a boolean, not a string, and it's used to automatically determine whether the link should be opened in a new tab or in a new window. Your call should be equivalent to this one:

openHelpLink(helpLink);

another nit: rename helpLink to helpTopic

Comment 23

[Richard Marti (:Paenglab)]

Attachment: inContentHelp.patch v6

Moved the image now to .button-icon. I had to use some other dimensions to stay for the button 30px * 30px.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8417648 [details] [diff] [review]
inContentHelp.patch v6

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

::: browser/components/preferences/in-content/preferences.js
@@ +62,5 @@
> +function helpButtonCommand() {
> +  let pane = history.state;
> +  let categories = document.getElementById("categories");
> +  let helpTopic = categories.querySelector(".category[value=" + pane + "]")
> +                           .getAttribute("helpTopic");

nit, line up the period here to match the line above.

::: browser/locales/en-US/chrome/browser/preferences/preferences.dtd
@@ +23,5 @@
>  <!ENTITY  paneSync.title          "Sync">
>  <!ENTITY  buttonForward.tooltip   "Go forward one page">
> +<!ENTITY  buttonBack.tooltip      "Go back one page">
> +
> +<!ENTITY  helpButton.label          "Help">

nit, line up the "Help" value with the "Go back one page" value.

::: browser/themes/shared/incontentprefs/preferences.css
@@ +194,5 @@
> +}
> +
> +.help-button:not([disabled="true"]):hover {
> +  background-color: #F4C200;
> +  background-image: none;

Can you file a bug to move much of the styling from <button> to a class-based selector so that it won't apply to the help button?

Comment 25

[Richard Marti (:Paenglab)]

Attachment: patch for check-in

Fixed the nit.

Comment 26

[Richard Marti (:Paenglab)]

Attachment: patch for check-in

Oops, stll one nit.

Comment 27

[Carsten Book [:Tomcat]]

Hi Richard,

could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 28

[Richard Marti (:Paenglab)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=a0cd92d4921e

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/22347f9e52cd

Comment 30

[Richard Marti (:Paenglab)]

bc1 is failing.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_bug731866.js | Non-General elements should be hidden
etc.

I suppose it's because of this change in this patch to show always the help button:

-    element.hidden = (attributeValue != aQuery);
+    element.hidden = (attributeValue && attributeValue != aQuery);

I've never done something on tests, please can someone help me finding what needs to change in the tests to pass?

Comment 31

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #29)
> https://hg.mozilla.org/integration/fx-team/rev/22347f9e52cd

also backed out for this test failures mentioned in comment 30

Comment 32

[Carsten Book [:Tomcat]]

(In reply to Richard Marti (:Paenglab) from comment #30)
> I've never done something on tests, please can someone help me finding what
> needs to change in the tests to pass?

maybe dao or jaws could help you here.

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

This is the cset link for the backout, which should have been posted when the backout occurred:
https://hg.mozilla.org/integration/fx-team/rev/78bae968118e

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in (slightly adjusted)

Richard, what do you think about this change? I reverted the change that was made to search() and moved the help-button outside of the prefpane. I also had to change .main-content from a <box> to a <vbox> to get the button to display under the prefs (since <box> is a synonym for <hbox>). This fixes the test failure.

Comment 35

[Richard Marti (:Paenglab)]

Comment on attachment 8418806 [details] [diff] [review]
Patch for check-in (slightly adjusted)

Great, this looks good. Thanks a lot.

Do I need to run a new try to ask for check-in?

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

No, we should be good to go here. I ran the tests locally with no failures.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/be64fe402981

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 39

[Sören Hentzschel]

Attachment: Bildschirmfoto 2014-05-08 um 15.57.43.png

The bug is marked as fixed, but what's about the styling? It doesn't match the spec, at least on OS X.

Comment 40

[Richard Marti (:Paenglab)]

(In reply to Sören Hentzschel from comment #39)
> Created attachment 8419399 [details]
> Bildschirmfoto 2014-05-08 um 15.57.43.png
> 
> The bug is marked as fixed, but what's about the styling? It doesn't match
> the spec, at least on OS X.

Please can you file a bug and CC me?

Comment 41

[Francesca Ciceri [:madamezou]]

Hi, 

reproduced on Nightly 32.0a1 (20140503030204) on Debian Sid, and I can confirm this is fixed in the latest Nightly Firefox/32.0 ID:20140520030202. 

Cheers,
Francesca

Comment 42

[Francesca Ciceri [:madamezou]]

(In reply to Francesca Ciceri [:madamezou] from comment #41)
> Hi, 
> 
> reproduced on Nightly 32.0a1 (20140503030204) on Debian Sid, and I can
> confirm this is fixed in the latest Nightly Firefox/32.0 ID:20140520030202. 
> 
> Cheers,
> Francesca

Ops, just realized I haven't added the platform/os where I verified it: x86_64/Linux (Debian Sid).

And as this was filed against all/all, I'm removing the "verified" status I added with the above comment. Feel free to mark it again as Verified in case the verification against just one os/arch is enough (and sorry for the noise).

Comment 43

[juan becerra [:juanb]]

This will need a visual spot check, so qa+'ing. CC'ing Henrik in case this had been covered by a Mozmill test and will now need revision.

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to juan becerra [:juanb] from comment #43)
> This will need a visual spot check, so qa+'ing. CC'ing Henrik in case this
> had been covered by a Mozmill test and will now need revision.

No, we don't have a test for Mozmill. But if one is necessary this can perfectly become a Browser Chrome test, or?

Comment 45

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 32.0a1 (buildID: 20140521030200).