428943 - Site identity popup should link to explanation/support

Status: RESOLVED FIXED

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

The site identity contextual dialog needs a 16x16 question icon.  I'll update this bug once it has landed.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Johnath: what's the status of actually having the code that would use this button, and the localized landing page?

This isn't blocking, it's a nice to have.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Johnath: here are the icons to use, in case you need them in the future for this UI:

http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/icons/question-16.png

http://mxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/global/icons/question-16-aero.png

http://mxr.mozilla.org/seamonkey/source/toolkit/themes/pinstripe/global/icons/question-mark.png

I'll attach the tango icon

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

Created attachment 329926 [details]
Tango help icon at 16x16

Comment 4

[Henrik Skupin (:whimboo) [away 07/08 - 07/22]]

Alex, I cannot find this icon within the contextual dialog. Can you tell me please, what is fixed?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

I think Alex resolved this unintentionally?

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

>Alex, I cannot find this icon within the contextual dialog. Can you tell me
>please, what is fixed?

I resolved since the icon now exists, I thought the bug was just for getting this icon created.  Either way, we can keep it open for johnath to drive adding the button to the dialog and getting it set up to link to the correct help pages.

Comment 7

[Henrik Skupin (:whimboo) [away 07/08 - 07/22]]

(In reply to comment #6)
> I resolved since the icon now exists, I thought the bug was just for getting
> this icon created.  Either way, we can keep it open for johnath to drive adding
> the button to the dialog and getting it set up to link to the correct help
> pages.

Thanks Alex. That sounds like a good idea. I'll assign this bug to him.

Comment 8

[Johnathan Nightingale [:johnath]]

Morphing the bug.  Should be an easy fix, but I'm not working on it at the moment.

Comment 9

[Henrik Skupin (:whimboo) [away 07/08 - 07/22]]

Where should this icon be placed? At the left side of the lock?

Comment 10

[Johnathan Nightingale [:johnath]]

I think probably floating in the top right or bottom left corner.  Top right may get confused (windows) people clicking it as a close button before realizing what it is, so bottom left is probably better placement. 

Comment 11

[Mark Becwar]

Is this still something that still needs to be implemented?

Comment 12

[Vikram Kamath]

I'm sorry, I'm kinda confused, has this been resolved or is there work still pending?

Comment 13

[:Margaret Leibovic]

(In reply to Vikram Kamath from comment #12)
> I'm sorry, I'm kinda confused, has this been resolved or is there work still
> pending?

No one ever wrote a patch for this bug, but this icon is really old, and probably isn't what we want anymore. We probably just want to use the question mark icons available in toolkit (toolkit/themes/.../global/icons/question-16.png).

Comment 14

[Tiziana Sellitto [:tiziana]]

Vikram, are you still working on this bug? 
I'm looking at old "good first bugs" not touched for a while. 
Jared, do you think this bug could become a mentored bug? is this something you can help Vikram to update?

Comment 15

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

Yeah, I can mentor this bug. Resetting the assignee of this bug since 1 year is far too long to pass with no movement.

Comment 16

[Minarto Margoliono]

Hi may i take this bug? 

If I understand correctly, the task is about placing a linked icon (taken from the toolkit/themes...) on bottom left of site identity popup. Site identity popup is the one shown when we click the padlock/globe in the left side of address bar right?

Should the icon linked to this page (based on localization)
http://support.mozilla.org/en-US/kb/how-do-i-tell-if-my-connection-is-secure

Comment 17

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

Yes that is correct. To get the localization link to work correctly, you can just remove the "en-US/" part of the link. The support.mozilla.org server will redirect to the correct locale.

Comment 18

[Minarto Margoliono]

Created attachment 764697 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

I put the icon on the right as the padlock icon is not always shown. I probably need to add tooltip as well, but I don't know what text i should put there.

Comment 19

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

Comment on attachment 764697 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

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

We'll also need changes for windows/browser.css and osx/browser.css.

::: browser/base/content/browser.xul
@@ +337,5 @@
>            </hbox>
>          </vbox>
> +        <image id="identity-popup-help-icon"
> +                     onclick="gIdentityHandler.handleHelpClick(event);"
> +                     />

The identity-popup-help-icon should be at the beginning of the same line as the identity-popup-more-info-button. Looking at this markup it appears to me that it will be below the identity-popup-more-info-button.

The indentation here can be fixed up. Line up the onclick attribute with the id attribute, and place the /> immediately after the onclick attribute.

For the tooltip, we can use the title of the SUMO documentation page, "How do I tell if my connection to a website is secure?". When you add the tooltip to the DTD file, add a localization note that says that the tooltip value should be the translated title of the SUMO article.

Comment 20

[Minarto Margoliono]

Created attachment 765272 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

Moved the button position and added tooltip text

Comment 21

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

Comment on attachment 765272 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

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

This patch doesn't apply for me on my latest mozilla-central repo. Can you update your tree and rebase your patch?

::: browser/base/content/browser.xul
@@ +333,5 @@
> +            <vbox>
> +              <image id="identity-popup-help-icon"
> +                     onclick="gIdentityHandler.handleHelpClick(event);"
> +                     tooltiptext="&identity.helpText;" />
> +            </vbox>

What is this <vbox> needed for?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +613,5 @@
>  
>  <!ENTITY identity.moreInfoLinkText "More Information…">
>  
> +<!-- Localization note (identity.helpText) : This string should be the 
> +localized title of this SUMO article  

nit, please remove the trailing whitespace and indent the second line of text to line up with Localization.

::: browser/themes/osx/browser.css
@@ +3010,5 @@
>    list-style-image: url("chrome://browser/skin/Secure-Glyph.png");
>  }
>  
> +#identity-popup-help-icon{
> +  list-style-image: url("chrome://global/skin/icons/question-16.png");

You'll need to include a HiDPI version of this graphic for Retina display Macs. See below for how it is used for other icons.

I'm not sure if we have a 32px by 32px help icon though, we may need one created.

Comment 22

[Minarto Margoliono]

Created attachment 766242 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

Let me know if the patch can be applied now. I updated my local with 'hg pull -u'

The vbox was used to prevent vertical streching of the image (the button is taller). I changed it to use align instead.

Fixed the spacing of localization.

Added rule for retina mac to use 32px icon.

Comment 23

[Dão Gottwald [::dao]]

Comment on attachment 766242 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

>+<!ENTITY identity.helpText "How do I tell if my connection to a website is secure?">

please rename this to identity.help.tooltip

>+#identity-popup-help-icon{

add a space before {

>+  list-style-image: url("chrome://global/skin/icons/question-16.png");
>+  -moz-padding-start: 15px;
>+  cursor:pointer;

and a space after the colon

Comment 24

[Minarto Margoliono]

Created attachment 766244 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

Comment 25

[Minarto Margoliono]

Created attachment 766254 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

Updated the text on browser.xul

Comment 26

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

I still can't get the patch to apply. Are you using the mozilla-central repo?

Comment 27

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

You can follow the steps here to generate the patch, https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Maybe this will help?

Comment 28

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

Comment on attachment 766254 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

Clearing review until the patch can get applied.

Comment 29

[Minarto Margoliono]

Created attachment 768591 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

Looks like my tree wasn't up to date, the popup part of browser.xul is splited to a new file. Sorry for the trouble.

Comment 30

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

Comment on attachment 768591 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up

I'm sorry, but I can't figure out what is going wrong here. I updated to the parent revision that is referenced in your diff file, and applied your patch but all hunks failed still.

Have you been modifying this diff file by hand?

Comment 31

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

Ok, the only issues with the patches were the line endings which got changed when the attachment was created by copy-pasting the text in to the Add Attachment As Text option. Sorry for the run-around.

I built the patch and I think we should make sure that the help icon is in fact in the bottom left corner. See this screenshot, http://screencast.com/t/Vu8FvrnrsWu. It should end up looking like this, http://screencast.com/t/x50XIR9Q4sN7

Can you make the "help button and more information line" have full width and be placed below the identity image (larry)?

Comment 32

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

The patch in bug 885366 should help you out here. Can you rebase on top of that patch?

Comment 33

[Minarto Margoliono]

Created attachment 770736 [details] [diff] [review]
Add a help icon on site identity popup and link it to mozilla support page

Rebase patch based on the related bug.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 770736 [details] [diff] [review]
Add a help icon on site identity popup and link it to mozilla support page

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+  handleHelpClick : function(event) {
>+    openUILinkIn("http://support.mozilla.org/kb/how-do-i-tell-if-my-connection-is-secure", "tab")

Hmm, this should probably be an app.support.baseURL-relative link? Someone from the sumo team (mverdi?) should be able to set one up for you.

Comment 35

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

This query, http://mxr.mozilla.org/mozilla-central/search?string=support.mozilla.org%2Fkb%2F&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central, shows many other places that have embedded SUMO links. We'll need to fix those too.

Michael, can you set one up for https://support.mozilla.org/kb/how-do-i-tell-if-my-connection-is-secure

Also, this made me notice that the link in the patch is to http:, when it should be to https:.

Comment 36

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

Michael, any update?

Comment 37

[Verdi [:verdi]]

Sorry - just saw this. 

Ricky can we set one of those in-product redirects to https://support.mozilla.org/kb/how-do-i-tell-if-my-connection-is-secure ?

Comment 38

[Ricky Rosario [:rrosario, :r1cky]]

(In reply to Verdi [:verdi] from comment #37)
> Sorry - just saw this. 
> 
> Ricky can we set one of those in-product redirects to
> https://support.mozilla.org/kb/how-do-i-tell-if-my-connection-is-secure ?

I just created a redirect. The "topic" is secure-connection. So the URL from Firefox should look like:
http://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/secure-connection

Comment 39

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

Comment on attachment 770736 [details] [diff] [review]
Add a help icon on site identity popup and link it to mozilla support page

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

::: browser/base/content/browser.js
@@ +6521,5 @@
> +   * Handler for mouseclicks on the help button in the
> +   * "identity-popup" panel.
> +   */
> +  handleHelpClick : function(event) {
> +    openUILinkIn("http://support.mozilla.org/kb/how-do-i-tell-if-my-connection-is-secure", "tab")

openUILinkIn can now be replaced with openHelpLink("secure-connection");

Comment 40

[Minarto Margoliono]

Created attachment 783101 [details] [diff] [review]
428943.patch

Changed the code to use openHelpLink and add the code to close the popup afterward

Comment 41

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

Comment on attachment 783101 [details] [diff] [review]
428943.patch

Redirecting review since I don't think I'll have time to get to this.

Comment 42

[Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th))]

Comment on attachment 783101 [details] [diff] [review]
428943.patch

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

The code looks OK, but there are some issues with the icons on (at least) OSX. I haven't tried Windows or Linux GTK yet, but here's what I'm seeing:

Vertical alignment issue on non-Retina:  http://i.imgur.com/5A7SXcx.png (lines added for illustration).

This first issue can likely be fixed by matching the 10px padding that's at the top of the "More Information" button.

For Retina, I'm seeing this:  http://i.imgur.com/ydYH00U.png. That is one big icon there - and looks kinda pixel-y. Might be better by setting a height of 16px on it in the @media (min-resolution: 2dppx) rule you've got going on there.

When you've got a new patch up, I'll try Windows and Linux.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +598,5 @@
>  <!ENTITY identity.moreInfoLinkText "More Information…">
>  
>  <!ENTITY identity.permissions "Permissions">
>  
> +<!-- Localization note (identity.help.tooltip) : This string should be the

"Localization note" is generally all-caps'd.

::: browser/themes/osx/browser.css
@@ +3033,5 @@
>    #identity-popup-content-box.verifiedDomain > #identity-popup-encryption > vbox > #identity-popup-encryption-icon {
>      list-style-image: url("chrome://browser/skin/[e-mail]");
>      width: 24px;
>    }
> +  

Nit - trailing whitespace.

Comment 43

[Minarto Margoliono]

Created attachment 789511 [details] [diff] [review]
428943.patch

Comment 44

[Dão Gottwald [::dao]]

Comment on attachment 789511 [details] [diff] [review]
428943.patch

>+        <image id="identity-popup-help-icon"
>+               onclick="gIdentityHandler.handleHelpClick(event);"
>+               tooltiptext="&identity.help.tooltip;"/>

Is this keyboard- and screen-reader-accessible?

Comment 45

[Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th))]

Comment on attachment 789511 [details] [diff] [review]
428943.patch

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

This looks OK to me. Just update the casing of Localization note, and answer my question about the margin for osx/browser.css.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +602,5 @@
>  <!ENTITY identity.moreInfoLinkText "More Information…">
>  
>  <!ENTITY identity.permissions "Permissions">
>  
> +<!-- Localization note (identity.help.tooltip) : This string should be the

I believe LOCALIZATION NOTE, in all upper-case, is the convention.

::: browser/themes/osx/browser.css
@@ +3027,5 @@
>    list-style-image: url("chrome://browser/skin/Secure-Glyph.png");
>  }
>  
> +#identity-popup-help-icon {
> +  margin: 10px 0 0;

Why not just margin-top: 10px? You have to eliminate other margins? From where?

Comment 46

[Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th))]

(In reply to Dão Gottwald [:dao] from comment #44)
> Comment on attachment 789511 [details] [diff] [review]
> 428943.patch
> 
> >+        <image id="identity-popup-help-icon"
> >+               onclick="gIdentityHandler.handleHelpClick(event);"
> >+               tooltiptext="&identity.help.tooltip;"/>
> 
> Is this keyboard- and screen-reader-accessible?

If I'm reading https://developer.mozilla.org/en/docs/XUL_accessibility_guidelines#Alternative_text correctly, I think the tooltiptext attribute makes this screen-reader-accessible.

As for keyboard accessibility, yeah this patch is lacking it. I can't seem to focus the image with the keyboard.

The solution here might be to use a button instead with -moz-appearance: none on it, and set the list-style-image on it with the icon. Then use oncommand instead of onclick to trigger handleHelpClick (which could probably be renamed to handleHelpCommand).

If we're going this far, we might as well put a focus glow around the button too when keyboard focused - identity-popup-more-info-button has a :focus rule on it that you can apply to your button as well.

Sorry for the slight shift in direction, lie.r.min.g - but I think Dao is right - this should be keyboard accessible.

Comment 47

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

Any update here?

Comment 48

[Minarto Margoliono]

I have changed the image into a button and use oncommand instead. I tested in on linux, and currently work on windows (xp) styling. After that done I will do the osx styling.

Comment 49

[Minarto Margoliono]

Created attachment 793425 [details] [diff] [review]
428943.patch

Comment 50

[Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th))]

Created attachment 796723 [details]
Applied patch

For reference, here's what the panel looks like with the patch.

Comment 51

[Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th))]

So the code looks great - thumbs up.

Looking at the applied patch, I'm noticing alignment inconsistencies - for example, the ? icon's left edge is not lined up with the left edge of "Larry" the passport guy. On Windows and Linux, it seems the ? is indented too much - and on OSX, not enough.

I've also noticed that when I keyboard focus the ? button, I get a black box around the button. It should be a single dotted border, I think.

Also, any idea why I can't keyboard focus this on OSX?

Comment 52

[Minarto Margoliono]

On OSX have you enabled full keyboard access? By default OSX doesn't allow tabbing between buttons. The shortcut is ctrl + F7 if I remember correctly. If it is not the issue, let me know. 

I didn't think of the vertical allignment with 'Larry'. I will try to align it and submit updated patch later.

Comment 53

[Minarto Margoliono]

Created attachment 798799 [details] [diff] [review]
428943.patch

Comment 54

[Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th))]

Created attachment 806220 [details]
Screen Shot 2013-09-17 at 4.35.41 PM.png

The code looks OK, but on Retina, the icon is stretched horizontally. Any idea why?

Comment 55

[Minarto Margoliono]

To be honest I do not know what might be wrong. I compared the latest patch (which strech the icon horizontally) and the one before (which has alignment problem). The only osx specific difference is this

+  margin: 10px 0 0 2px;
-  margin-top: 10px;

Adding 2 px margin-left should not stretch the icon. I do not have access to retina mac to try whether that is really the cause.

Comment 56

[Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th))]

Ok, let me try fiddling with it for a bit.

Comment 57

[Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th))]

Comment on attachment 798799 [details] [diff] [review]
428943.patch

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

Ok, I figured it out.

There are two things I'd like you to do here:

1. Set display: none on the button-text that is a child of the button, via:

#identity-popup-help-icon > .button-box > .button-text {
  display: none;
}

2. Set the height and width of the of the .button-icon, instead of the button itself, via:

#identity-popup-help-icon > .button-box > .button-icon {
  height: 16px;
  width: 16px;
}

3. Do a quick audit of the rules you've put in, because I'm pretty sure there's some unnecessary rules in there. I don't believe the border: none and padding rules are needed, for example...same with background. Can you confirm?

Thanks so much for your patience with this!

-Mike

Comment 58

[Minarto Margoliono]

those override are needed because og this rule (taken from linux css, but should bu similar to others)

button {
  ...
  min-width: 6.3em;
  border: 3px solid;
  background-color: ThreeDFace;
  ...
}

cursor is needed as -moz-appearance:none will make the button have a normal cursor instead of pointer which button usually has. 

For padding, I think I can safefly remove it. Initially I mistook the width of button was caused by padding, but apparently it is caused by min-width rule above.

I have some questions:
- Do I need to add those only for osx, or for all browser?
- Do I need to move the list-image-style to .button-icon or keep it at the old location.

Comment 59

[Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th))]

(In reply to lie.r.min.g from comment #58)
> those override are needed because og this rule (taken from linux css, but
> should bu similar to others)
> 
> button {
>   ...
>   min-width: 6.3em;
>   border: 3px solid;
>   background-color: ThreeDFace;
>   ...
> }
> 
> cursor is needed as -moz-appearance:none will make the button have a normal
> cursor instead of pointer which button usually has. 
> 
> For padding, I think I can safefly remove it. Initially I mistook the width
> of button was caused by padding, but apparently it is caused by min-width
> rule above.

Ok, cool. Thanks for looking over those.

> 
> I have some questions:
> - Do I need to add those only for osx, or for all browser?

Might as well add them for all themes.

> - Do I need to move the list-image-style to .button-icon or keep it at the
> old location.

Keeping it at the old location is fine.

Comment 60

[Minarto Margoliono]

Created attachment 813839 [details] [diff] [review]
428943.patch

Comment 61

[Mike Conley (:mconley) (:⚙️) (PTO Jul 24th-Aug 6th))]

Comment on attachment 813839 [details] [diff] [review]
428943.patch

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

This looks really really good!

I just have one nit - when keyboard focusing the button on Windows, I get a double dotted outline: http://i.imgur.com/n4vDNzK.png

But I'm willing to get that fixed that in a follow-up, since I think we've iterated enough on this patch. :) Let's get it landed!

Thanks!

-Mike

Comment 62

[Ryan VanderMeulen [:RyanVM]]

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

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4eae74b3965f