Closed
Bug 428943
Opened 13 years ago
Closed 8 years ago
Site identity popup should link to explanation/support
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 27
People
(Reporter: faaborg, Assigned: lie.r.min.g)
References
()
Details
(Whiteboard: [good first bug][mentor=mconley][lang=js][lang=xul][lang=css])
Attachments
(4 files, 11 obsolete files)
The site identity contextual dialog needs a 16x16 question icon. I'll update this bug once it has landed.
Comment 1•13 years ago
|
||
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 2•13 years ago
|
||
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
Reporter | ||
Comment 3•13 years ago
|
||
Reporter | ||
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 4•13 years ago
|
||
Alex, I cannot find this icon within the contextual dialog. Can you tell me please, what is fixed?
Comment 5•13 years ago
|
||
I think Alex resolved this unintentionally?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 6•13 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.
Comment 7•13 years ago
|
||
(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
Comment 8•13 years ago
|
||
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]
Comment 9•13 years ago
|
||
Where should this icon be placed? At the left side of the lock?
Comment 10•13 years ago
|
||
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•11 years ago
|
||
Is this still something that still needs to be implemented?
Comment 12•9 years ago
|
||
I'm sorry, I'm kinda confused, has this been resolved or is there work still pending?
Comment 13•9 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•9 years ago
|
Assignee: nobody → vikram.kmthdev
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 14•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
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•8 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
Comment 17•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
Attachment #764697 -
Flags: review? → review?(jaws)
Comment 19•8 years ago
|
||
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•8 years ago
|
||
Moved the button position and added tooltip text
Attachment #764697 -
Attachment is obsolete: true
Attachment #765272 -
Flags: review?(jaws)
Comment 21•8 years ago
|
||
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•8 years ago
|
||
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 23•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Updated the text on browser.xul
Attachment #766244 -
Attachment is obsolete: true
Attachment #766244 -
Flags: review?(jaws)
Attachment #766254 -
Flags: review?(jaws)
Updated•8 years ago
|
Attachment #766254 -
Flags: ui-review?(ux-review)
Comment 26•8 years ago
|
||
I still can't get the patch to apply. Are you using the mozilla-central repo?
Comment 27•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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 30•8 years ago
|
||
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)
Comment 31•8 years ago
|
||
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•8 years ago
|
||
The patch in bug 885366 should help you out here. Can you rebase on top of that patch?
Assignee | ||
Comment 33•8 years ago
|
||
Rebase patch based on the related bug.
Attachment #768591 -
Attachment is obsolete: true
Attachment #770736 -
Flags: review?(jaws)
Comment 34•8 years ago
|
||
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•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #770736 -
Flags: review?(jaws)
Comment 36•8 years ago
|
||
Michael, any update?
Updated•8 years ago
|
Flags: needinfo?(mverdi)
Comment 37•8 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)
Comment 38•8 years ago
|
||
(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 39•8 years ago
|
||
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•8 years ago
|
||
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 41•8 years ago
|
||
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•8 years ago
|
||
Attachment #783101 -
Attachment is obsolete: true
Attachment #789511 -
Flags: review?(mconley)
Comment 44•8 years ago
|
||
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.
Assignee | ||
Comment 48•8 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•8 years ago
|
||
Attachment #793425 -
Flags: review?(mconley)
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•8 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•8 years ago
|
||
Attachment #789511 -
Attachment is obsolete: true
Attachment #793425 -
Attachment is obsolete: true
Attachment #793425 -
Flags: review?(mconley)
Attachment #798799 -
Flags: review?(mconley)
The code looks OK, but on Retina, the icon is stretched horizontally. Any idea why?
Flags: needinfo?(lie.r.min.g)
Assignee | ||
Comment 55•8 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•8 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•8 years ago
|
||
Attachment #813839 -
Flags: review?(mconley)
Attachment #798799 -
Attachment is obsolete: true
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
Depends on: 924532
Comment 62•8 years ago
|
||
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]
Comment 63•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4eae74b3965f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 8 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.
Description
•