Site identity popup should link to explanation/support

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Address Bar
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: faaborg, Assigned: Minarto Margoliono)

Tracking

Trunk
Firefox 27
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=mconley][lang=js][lang=xul][lang=css], URL)

Attachments

(4 attachments, 11 obsolete attachments)

1.01 KB, image/png
Details
162.37 KB, image/png
Details
121.46 KB, image/png
Details
7.75 KB, patch
mconley
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
The site identity contextual dialog needs a 16x16 question icon.  I'll update this bug once it has landed.
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.
Flags: wanted-firefox3+
Flags: blocking-firefox3-
(Reporter)

Comment 3

10 years ago
Created attachment 329926 [details]
Tango help icon at 16x16
(Reporter)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Alex, I cannot find this icon within the contextual dialog. Can you tell me please, what is fixed?
I think Alex resolved this unintentionally?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 6

10 years ago
>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.
(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.
Assignee: nobody → johnath
Status: REOPENED → NEW
Morphing the bug.  Should be an easy fix, but I'm not working on it at the moment.
Assignee: johnath → nobody
Component: Theme → Location Bar and Autocomplete
OS: Windows XP → All
QA Contact: theme → location.bar
Summary: Windows icon: site identity contextual dialog needs a 16x16 question icon → Site identity popup should link to explanation/support
Whiteboard: [good first bug]
Where should this icon be placed? At the left side of the lock?
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

8 years ago
Is this still something that still needs to be implemented?

Comment 12

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

Comment 13

7 years ago
(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).
Keywords: uiwanted

Updated

7 years ago
Assignee: nobody → vikram.kmthdev
Status: NEW → ASSIGNED
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?
Flags: needinfo?(jaws)
Yeah, I can mentor this bug. Resetting the assignee of this bug since 1 year is far too long to pass with no movement.
Assignee: vikram.kmthdev → nobody
Flags: needinfo?(jaws)
Whiteboard: [good first bug] → [good first bug][mentor=jaws][lang=js][lang=xul][lang=css]
(Assignee)

Comment 16

5 years ago
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
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.
Assignee: nobody → lie.r.min.g
(Assignee)

Comment 18

5 years ago
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.
Attachment #764697 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #764697 - Flags: review? → review?(jaws)
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.
Attachment #764697 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 20

5 years ago
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
Attachment #764697 - Attachment is obsolete: true
Attachment #765272 - Flags: review?(jaws)
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.
Attachment #765272 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 22

5 years ago
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.
Attachment #765272 - Attachment is obsolete: true
Attachment #766242 - Flags: review?(jaws)
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
Attachment #766242 - Flags: ui-review?(ux-review)
(Assignee)

Comment 24

5 years ago
Created attachment 766244 [details] [diff] [review]
Add help button to the relevant support page for site identity pop-up
Attachment #766242 - Attachment is obsolete: true
Attachment #766242 - Flags: ui-review?(ux-review)
Attachment #766242 - Flags: review?(jaws)
Attachment #766244 - Flags: review?(jaws)
(Assignee)

Comment 25

5 years ago
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
Attachment #766244 - Attachment is obsolete: true
Attachment #766244 - Flags: review?(jaws)
Attachment #766254 - Flags: review?(jaws)

Updated

5 years ago
Attachment #766254 - Flags: ui-review?(ux-review)
I still can't get the patch to apply. Are you using the mozilla-central repo?
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.
Attachment #766254 - Flags: review?(jaws)
(Assignee)

Comment 29

5 years ago
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.
Attachment #766254 - Attachment is obsolete: true
Attachment #766254 - Flags: ui-review?(ux-review)
Attachment #768591 - Flags: review?(jaws)
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?
Attachment #768591 - Flags: review?(jaws)
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)?
Keywords: uiwanted
The patch in bug 885366 should help you out here. Can you rebase on top of that patch?
No longer depends on: 885366
(Assignee)

Comment 33

5 years ago
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.
Attachment #768591 - Attachment is obsolete: true
Attachment #770736 - Flags: review?(jaws)
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.
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:.
Flags: needinfo?(mverdi)
Michael, any update?

Updated

5 years ago
Flags: needinfo?(mverdi)

Comment 37

5 years ago
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 ?
Flags: needinfo?(rrosario)
(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
Flags: needinfo?(rrosario)
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");
(Assignee)

Comment 40

5 years ago
Created attachment 783101 [details] [diff] [review]
428943.patch

Changed the code to use openHelpLink and add the code to close the popup afterward
Attachment #770736 - Attachment is obsolete: true
Attachment #783101 - Flags: review?(jaws)
Comment on attachment 783101 [details] [diff] [review]
428943.patch

Redirecting review since I don't think I'll have time to get to this.
Attachment #783101 - Flags: review?(jaws) → review?(mconley)
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/Secure-Glyph@2x.png");
>      width: 24px;
>    }
> +  

Nit - trailing whitespace.
Attachment #783101 - Flags: review?(mconley) → review-
(Assignee)

Comment 43

5 years ago
Created attachment 789511 [details] [diff] [review]
428943.patch
Attachment #783101 - Attachment is obsolete: true
Attachment #789511 - Flags: review?(mconley)
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 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?
Attachment #789511 - Flags: review?(mconley)
(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.
Any update here?
Flags: needinfo?(lie.r.min.g)
(Assignee)

Comment 48

5 years ago
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.
Flags: needinfo?(lie.r.min.g)
(Assignee)

Comment 49

5 years ago
Created attachment 793425 [details] [diff] [review]
428943.patch
Attachment #793425 - Flags: review?(mconley)
Created attachment 796723 [details]
Applied patch

For reference, here's what the panel looks like with the patch.
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?
Flags: needinfo?(lie.r.min.g)
(Assignee)

Comment 52

5 years ago
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.
Flags: needinfo?(lie.r.min.g)
(Assignee)

Comment 53

5 years ago
Created attachment 798799 [details] [diff] [review]
428943.patch
Attachment #789511 - Attachment is obsolete: true
Attachment #793425 - Attachment is obsolete: true
Attachment #793425 - Flags: review?(mconley)
Attachment #798799 - Flags: review?(mconley)
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?
Flags: needinfo?(lie.r.min.g)
(Assignee)

Comment 55

5 years ago
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.
Flags: needinfo?(lie.r.min.g)
Ok, let me try fiddling with it for a bit.
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
Attachment #798799 - Flags: review?(mconley)
Whiteboard: [good first bug][mentor=jaws][lang=js][lang=xul][lang=css] → [good first bug][mentor=mconley][lang=js][lang=xul][lang=css]
(Assignee)

Comment 58

5 years ago
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.
(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.
(Assignee)

Comment 60

5 years ago
Created attachment 813839 [details] [diff] [review]
428943.patch
Attachment #813839 - Flags: review?(mconley)
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
Attachment #813839 - Flags: review?(mconley) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/4eae74b3965f
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=mconley][lang=js][lang=xul][lang=css] → [good first bug][mentor=mconley][lang=js][lang=xul][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4eae74b3965f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=mconley][lang=js][lang=xul][lang=css][fixed-in-fx-team] → [good first bug][mentor=mconley][lang=js][lang=xul][lang=css]
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.