448833 - Page Info > Security > More is confusing

Status: VERIFIED FIXED

(Firefox :: Page Info Window, enhancement)

Description

[Tyler Downer [He/Him]]

When Page Info is entered (opens in General Tab), in the "Security Information for this Page" box, there is a button labeled "More". This opens the Security Tab.

However, when you click the "More" button, you expect it will open a box in the same tab (based on the tradition More Information, Less, etc). When it opens in Security, it is rather disorienting to find that you are suddenly in "Security", not general. This results from the subtle color changes, and overall usability. 

Maybe the "More" should be "Security Info" or, "Go to Detailed Security". Just a thought.

Comment 1

[Tyler Downer [He/Him]]

Attachment: Patch

This changes the text of the button from "More" to "Details", stressing that it will appear in a new tab, instead of the implied same tab from "More".
This is my first patch, so I may be a bit rusty on this.

Comment 2

[Tyler Downer [He/Him]]

Attachment: Patch

Sorry, put the wrong reviewer in.

Comment 3

[Tyler Downer [He/Him]]

Can Michael Connor or someone review this for Page Info?

Comment 4

[Tyler Downer [He/Him]]

Attachment: Patch

SORRY. i am new at C++ and patches, even newer at FF code. Can someone please review this?

Comment 5

[Florian Quèze [:florian]]

Could you please attach a diff for your patch? See http://developer.mozilla.org/en/docs/Creating_a_patch

Comment 6

[Tyler Downer [He/Him]]

Sorry, about that. I am going to make a small change, then will give you the diff.

Comment 7

[Tyler Downer [He/Him]]

Attachment: Patch

Here is the new patch, I am still trying to figure out how to make a diff, is there anyone who can help me do this, as I do not have any merge stuff on my internet setup.

Comment 8

[Steffen Wilberg]

Comment on attachment 332431 [details]
Patch

A zip of the complete, modified files is not a diff.

A diff is a small text file which only contains the changes.

Did you use cvs or hg to get the source? Firefox 3.1 development happens on hg.

Use "hg diff -p -U8 FILE > mypatch.diff" to create a diff, where FILE is the file you want to diff and mypatch.diff is the output.

Comment 9

[Tyler Downer [He/Him]]

I downloaded a tarball onto my Visual Studio Machine (with no internet). If I can find recent instructions (the only ones are for Gecko 1.8.x) I will create a diff.

Comment 10

[Steffen Wilberg]

The latest and greatest source, version 3.1a2pre right now, is only available via Mercurial.

See http://developer.mozilla.org/en/docs/Mozilla_Source_Code_(Mercurial).
You need to
1. Download, install and configure MozillaBuild (http://developer.mozilla.org/en/docs/Installing_Mercurial)
2. Get the source tree (http://developer.mozilla.org/en/docs/Mozilla_Source_Code_(Mercurial)#Checking_out_a_source_tree).
3. Use hg diff as I said in comment 8.

Comment 11

[Tyler Downer [He/Him]]

I wil be getting on this. Sorry for the delay.

Comment 12

[Tyler Downer [He/Him]]

Attachment: Patch

I think this is the Diff. Thanks.

Comment 13

[Steffen Wilberg]

That's a diff, but it contains the entire files instead of just the changed lines and some context before and afterwards.

That might be an issue with line-endings. Mozilla files use unix-style line-endings, you need to use an editor which doesn't convert them to windows-style. I like http://www.jedit.org/.

"I" is a bad accesskey for "Details". The underline for the letter "i" is very narrow and hard to notice. Use "D" instead.

Comment 14

[Tyler Downer [He/Him]]

Attachment: Patch V1.1

Correct format, as well as incorporating above suggestions.

Comment 15

[Steffen Wilberg]

Now that looks so much better!
I can't grant review, please ask mano@moz or gavin.sharp@gmail or mconnor@moz instead (as per http://www.mozilla.org/projects/firefox/review.html).

Comment 16

[Tyler Downer [He/Him]]

Thanks, I will get Gavin in on this (I think I finally have the bug process down better now, so I can start working on others). Thank you for the help.

Comment 17

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

Comment on attachment 337971 [details] [diff] [review]
Patch V1.1

>diff --git a/browser/locales/en-US/chrome/browser/pageInfo.dtd b/browser/locales/en-US/chrome/browser/pageInfo.dtd

>+<!ENTITY  generalSecurityMore   "Details">
>+<!ENTITY  generalSecurityMore.accesskey "D">

Looks like you meant to change these to be generalSecurityDetails instead of generalSecurityMore, to match the XUL change?

Why change the ID and function name?

Comment 18

[Tyler Downer [He/Him]]

Yes I do mean to change those and will look at those. I changed them to reflect the change from More to Details. This should avoid confusion later. Let me throw that up in a bit. That was a DD mistake by me. Sorry.

Comment 19

[Tyler Downer [He/Him]]

Attachment: Patch V1.2

Fix DD bug.

Comment 20

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

I'm fine with changing the strings, but changing the ID and function name can affect extension compatibility, so you have to weigh that cost against the cost of confusion down the line. In this case, I don't think the name of the function not matching the button's label is particularly confusing, so in the interests of minimizing change, I'd prefer to just leave it as is.

Comment 21

[Tyler Downer [He/Him]]

OK, I can do that as well.

Comment 22

[Tyler Downer [He/Him]]

Attachment: Patch V2 [Checkin: Comment 25]

I have changed the id and labels to "Details", but we still have OnClickMore().
I think that we should just go ahead and change the label as I have it to avoid any confusion say, in two years with 4.2. i don't think many extensions will be affected by this, but have kept what I said above. just the id and label is different. If you want just the DTD file to say:
>diff --git a/browser/locales/en-US/chrome/browser/pageInfo.dtd b/browser/locales/en-US/chrome/browser/pageInfo.dtd

>+<!ENTITY  generalSecurityMore   "Details">
>+<!ENTITY  generalSecurityMore.accesskey "D">

I can, but that is most likely not the best long-term idea.
My 2c.

Comment 23

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

Comment on attachment 338230 [details] [diff] [review]
Patch V2
[Checkin: Comment 25]

I'm not sure exactly why you're reluctant to leave the ID as-is - there's nothing that requires that an ID match the entity name (they rarely do!), and having them contain a common substring doesn't really help code clarity at all. I don't want to argue for the sake of arguing - you're right that it likely matters very little as far as extension compat goes - but I'm a big fan of minimizing unnecessary code churn.

Comment 24

[Tyler Downer [He/Him]]

Like I said, if you want me to leave it the way it is, that is no problem. I will mark as checkin-needed, but let me know if I should change it. i agree it is not terribly important, but I am something of a neat-freak with my code.

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 338230 [details] [diff] [review]
Patch V2
[Checkin: Comment 25]

http://hg.mozilla.org/mozilla-central/rev/e8824a809f0f

Comment 26

[Tyler Downer [He/Him]]

Thank you.

Comment 27

[Steffen Wilberg]

Replace "More" with "Details" on http://support.mozilla.com/en-US/kb/Page+Info+window?style_mode=inproduct#Security_Information_for_this_page.

Comment 28

[Tyler Downer [He/Him]]

Made the edit, waiting for review.

Comment 29

[Chris Ilias [:cilias]]

Thanks for the edit Tyler; but support.mozilla.com is not covering Firefox 3.1 yet. (but keep the user-doc-needed keyword on this bug)

Comment 30

[Tyler Downer [He/Him]]

DD again (I need a vacation desperately). Will make edit with release of 3.1.

Comment 31

[Chris Ilias [:cilias]]

user-doc-complete
http://support.mozilla.com/kb/Page+Info+window?bl=n#Security_Information_for_this_page