Closed Bug 448833 Opened 14 years ago Closed 14 years ago

Page Info > Security > More is confusing

Categories

(Firefox :: Page Info Window, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.1b1

People

(Reporter: tdowner, Assigned: tdowner)

Details

(Keywords: user-doc-complete)

Attachments

(1 file, 7 obsolete files)

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.
Flags: wanted-firefox3.1?
OS: Windows Vista → All
Hardware: PC → All
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → tyler
Status: NEW → ASSIGNED
Attachment #332069 - Flags: superreview?
Attachment #332069 - Flags: review?
Attached patch Patch (obsolete) — Splinter Review
Sorry, put the wrong reviewer in.
Attachment #332069 - Attachment is obsolete: true
Attachment #332070 - Flags: review?
Attachment #332069 - Flags: superreview?
Attachment #332069 - Flags: review?
Can Michael Connor or someone review this for Page Info?
Attached patch Patch (obsolete) — Splinter Review
SORRY. i am new at C++ and patches, even newer at FF code. Can someone please review this?
Attachment #332070 - Attachment is obsolete: true
Attachment #332070 - Flags: review?
Sorry, about that. I am going to make a small change, then will give you the diff.
Attached file Patch (obsolete) —
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.
Attachment #332077 - Attachment is obsolete: true
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.
Attachment #332431 - Attachment is obsolete: true
Attachment #332431 - Attachment is patch: false
Attachment #332431 - Attachment mime type: text/plain → application/zip
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.
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.
I wil be getting on this. Sorry for the delay.
Attached patch Patch (obsolete) — Splinter Review
I think this is the Diff. Thanks.
Attachment #336896 - Flags: review?(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.
Attachment #336896 - Flags: review?(steffen.wilberg)
Attached patch Patch V1.1 (obsolete) — Splinter Review
Correct format, as well as incorporating above suggestions.
Attachment #336896 - Attachment is obsolete: true
Attachment #337971 - Flags: review?(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).
Attachment #337971 - Flags: review?(steffen.wilberg) → review?(gavin.sharp)
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.
Attachment #337971 - Flags: review?(gavin.sharp) → review-
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?
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.
Attached patch Patch V1.2 (obsolete) — Splinter Review
Fix DD bug.
Attachment #337971 - Attachment is obsolete: true
Attachment #338153 - Flags: review?(gavin.sharp)
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.
OK, I can do that as well.
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.
Attachment #338153 - Attachment is obsolete: true
Attachment #338230 - Flags: review?(gavin.sharp)
Attachment #338153 - Flags: review?(gavin.sharp)
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.
Attachment #338230 - Flags: review?(gavin.sharp) → review+
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.
Keywords: checkin-needed
Comment on attachment 338230 [details] [diff] [review]
Patch V2
[Checkin: Comment 25]

http://hg.mozilla.org/mozilla-central/rev/e8824a809f0f
Attachment #338230 - Attachment description: Patch V2 → Patch V2 [Checkin: Comment 25]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b1
Thank you.
Status: RESOLVED → VERIFIED
Made the edit, waiting for review.
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)
DD again (I need a vacation desperately). Will make edit with release of 3.1.
Flags: wanted-firefox3.5?
You need to log in before you can comment on or make changes to this bug.