In bug 339102, as part of a GSoC project, Firefox got a reworked page info dialog. We should probably port that to SeaMonkey (trunk, suiterunner). I'd personally prefer to still have tabs instead of the icon strip that Firefox uses, but I guess the rest looks fine for us as well.

(In reply to comment #0) > In bug 339102, as part of a GSoC project, Firefox got a reworked page info > dialog. We should probably port that to SeaMonkey (trunk, suiterunner). > There are a few things that still need to be fixed in the new Firefox page info. I think you will probably want to wait for bug 377364 and bug 378770 to be fixed. > I'd personally prefer to still have tabs instead of the icon strip that Firefox > uses, but I guess the rest looks fine for us as well. > You may also want to keep the forms and/or links tab.

I don't think that SeaMonkey has any feed handling code yet.

addresses several comments by Kairo

Comment on attachment 275423 [details] [diff] [review] 379183-2.diff >- * The Initial Developer of the Original Code is Daniel Brooks. Isn't it? >+textbox { > background: transparent !important; > border: none; > padding: 0px; >+ margin-top: 1px; > -moz-appearance: none; > } Why the margin change? [Why not use textbox class="plain"?] > textbox.header { >- margin-left: 0; >+ -moz-margin-start: 0; > } You don't have to change this yet if you don't want to ;-) >+/* General Tab */ >+groupbox.collapsable caption .caption-icon { >+ width: 9px; >+ height: 9px; >+ background-repeat: no-repeat; >+ background-position: center; >+ -moz-margin-start: 1px; >+ -moz-margin-end: 3px; >+ background-image: url("chrome://global/skin/tree/twisty-open.png"); >+} >+ >+groupbox.collapsable[closed="true"] { >+ border: none; >+} >+ >+groupbox.collapsable[closed="true"] caption .caption-icon { >+ background-image: url("chrome://global/skin/tree/twisty-clsd.png"); >+} >+ >+groupbox tree { >+ margin: 0; >+ border: none; >+} >+ >+groupbox.treebox .groupbox-body { >+ padding-top: 0; >+} Eww. Ditch collapsAble groupboxes completely? >+#securityBox description { >+ -moz-margin-start: 10px; >+} Child selectors? Or maybe <description class="indent">? >+#general-security-identity { >+ white-space: -moz-pre-wrap; >+ line-height: 2em; >+} We can't do this using two <description> elements? >+#feedtree { >+ margin-bottom: 0px; >+} Which tree is this? >+#feedListbox richlistitem { >+ padding-top: 6px; >+ padding-bottom: 6px; >+ -moz-padding-start: 7px; >+ -moz-padding-end: 7px; padding: 6px 7px; >+ border-bottom: 1px dotted #C0C0C0; Classic does not hardcode colours. >+#feedListbox richlistitem[selected="true"] { >+ background-color: -moz-Dialog; >+ color: -moz-DialogText; >+} Isn't this in richlistbox.css? >+#permList > vbox:hover { >+ background-color: -moz-dialog; >+} Hover looks weird. But I wouldn't mind if you switched to a richlistbox. >+/* Security Tab */ >+#securityPanel .caption-icon { >+ display: none; >+} >+ >+#securityPanel .header { >+ font-size: 120%; >+} >+ >+#securityPanel .fieldLabel { >+ margin: 2px 10px 3px 10px; >+} >+ >+#securityPanel .fieldValue { >+ font-weight: bold; >+ margin: 2px 10px 0px 10px; >+} More descendent selectors... >+textbox { > background: transparent !important; > border: none; > padding: 0px; >+ margin-top: 1px; >+ -moz-appearance: none; textbox is already -moz-appearance: none; in Modern. > textbox.header { >- margin-left: 0; >+ -moz-margin-start: 0; > } Modern has almost no rtl support... >+textbox[disabled] { >+ font-style: italic; >+} Still using these? I thought you went in for hiding them now. (Not that I actually like the effect, especially on the media tab.) >+treechildren::-moz-tree-cell-text(broken) { >+ font-style: italic; >+ color: graytext; > } #999999 (I assume this is on a white background) >+#feedListbox richlistitem { >+ padding-top: 6px; >+ padding-bottom: 6px; >+ -moz-padding-start: 7px; >+ -moz-padding-end: 7px; >+ min-height: 25px; >+ border-bottom: 1px dotted #C0C0C0; >+} #C0C0C0 is not a typical Modern colour. Try #C7D0D9. >+#feedListbox richlistitem[selected="true"] { >+ background-color: -moz-Dialog; >+ color: -moz-DialogText; >+} And these are definitely not Modern colours. >+ -moz-padding-start: 7px; >+ -moz-padding-end: 7px; See above. >+ background-color: -moz-dialog; Etc. >+/* Security Tab */ Etc. >+* content/navigator/pageinfo/feeds.js (pageinfo/feeds.js) >+* content/navigator/pageinfo/feeds.xml (pageinfo/feeds.xml) >+ content/navigator/pageinfo/pageInfo.css (pageinfo/pageInfo.css) >+* content/navigator/pageinfo/pageInfo.js (pageinfo/pageInfo.js) >+* content/navigator/pageinfo/pageInfo.xul (pageinfo/pageInfo.xul) >+* content/navigator/pageinfo/permissions.js (pageinfo/permissions.js) >+* content/navigator/pageinfo/security.js (pageinfo/security.js) Try to avoid the *s. Use real comments for a start. >+richlistitem[feed]:not([selected="true"]) .feed-subscribe { >+ display: none; >+} Etc. >+groupbox[closed="true"] > .groupbox-body { >+ visibility: collapse; >+} See above. >+#ifdef XP_MACOSX >+<?xul-overlay href="chrome://navigator/content/macBrowserOverlay.xul"?> >+#endif We don't have this. >+#ifdef XP_MACOSX >+ <key key="." modifiers="meta" command="cmd_close"/> >+#endif Just leave the key in for all platforms. >+ <tab id="feedTab" label="&feedTab;" accesskey="&feedTab.accesskey;" >+ oncommand="showTab('feed');" hidden="true"/> Unfortunately tabs don't take to being hidden and shown. Can you try disabling them instead? >+#ifdef XP_MACOSX >+#include ../browserMountPoints.inc >+#endif Etc. >+function realmHasPasswords(location) { >+ if (!location) >+ return false; >+ >+ try { >+ var passwordManager = Components.classes["@mozilla.org/login-manager;1"] >+ .getService(Components.interfaces.nsILoginManager); I hope we get to switch to login manager soon :-) >+<!ENTITY securityView.privacy.history "Have I visited this website before today?"> Where does this get used (or couldn't you port it?)

(In reply to comment #5) > (From update of attachment 275423 [details] [diff] [review]) > >- * The Initial Developer of the Original Code is Daniel Brooks. > Isn't it? Isn't it what? I am the original developer for some of these files. > > >+textbox { > > background: transparent !important; > > border: none; > > padding: 0px; > >+ margin-top: 1px; > > -moz-appearance: none; > > } > Why the margin change? [Why not use textbox class="plain"?] It was necessary at the time, but I don't know if it's still needed. At the time this particular rule was created, there was no class="plain" for textboxes. > > > textbox.header { > >- margin-left: 0; > >+ -moz-margin-start: 0; > > } > You don't have to change this yet if you don't want to ;-) > > >+/* General Tab */ > >+groupbox.collapsable caption .caption-icon { > >+ width: 9px; > >+ height: 9px; > >+ background-repeat: no-repeat; > >+ background-position: center; > >+ -moz-margin-start: 1px; > >+ -moz-margin-end: 3px; > >+ background-image: url("chrome://global/skin/tree/twisty-open.png"); > >+} > >+ > >+groupbox.collapsable[closed="true"] { > >+ border: none; > >+} > >+ > >+groupbox.collapsable[closed="true"] caption .caption-icon { > >+ background-image: url("chrome://global/skin/tree/twisty-clsd.png"); > >+} > >+ > >+groupbox tree { > >+ margin: 0; > >+ border: none; > >+} > >+ > >+groupbox.treebox .groupbox-body { > >+ padding-top: 0; > >+} > Eww. Ditch collapsAble groupboxes completely? sure. > > >+#securityBox description { > >+ -moz-margin-start: 10px; > >+} > Child selectors? Or maybe <description class="indent">? I like class="indent", personally. > > >+#general-security-identity { > >+ white-space: -moz-pre-wrap; > >+ line-height: 2em; > >+} > We can't do this using two <description> elements? dunno, I'll look in to it. > > >+#feedtree { > >+ margin-bottom: 0px; > >+} > Which tree is this? the one that used to be in the feeds tab, presumably > > >+#feedListbox richlistitem { > >+ padding-top: 6px; > >+ padding-bottom: 6px; > >+ -moz-padding-start: 7px; > >+ -moz-padding-end: 7px; > padding: 6px 7px; > > >+ border-bottom: 1px dotted #C0C0C0; > Classic does not hardcode colours. yea, what should I replace it with? > > >+#feedListbox richlistitem[selected="true"] { > >+ background-color: -moz-Dialog; > >+ color: -moz-DialogText; > >+} > Isn't this in richlistbox.css? yup > > >+#permList > vbox:hover { > >+ background-color: -moz-dialog; > >+} > Hover looks weird. But I wouldn't mind if you switched to a richlistbox. eh. that's a lot of work. > > >+/* Security Tab */ > >+#securityPanel .caption-icon { > >+ display: none; > >+} > >+ > >+#securityPanel .header { > >+ font-size: 120%; > >+} > >+ > >+#securityPanel .fieldLabel { > >+ margin: 2px 10px 3px 10px; > >+} > >+ > >+#securityPanel .fieldValue { > >+ font-weight: bold; > >+ margin: 2px 10px 0px 10px; > >+} > More descendent selectors... done. > > >+textbox { > > background: transparent !important; > > border: none; > > padding: 0px; > >+ margin-top: 1px; > >+ -moz-appearance: none; > textbox is already -moz-appearance: none; in Modern. removed > > > textbox.header { > >- margin-left: 0; > >+ -moz-margin-start: 0; > > } > Modern has almost no rtl support... > > >+textbox[disabled] { > >+ font-style: italic; > >+} > Still using these? I thought you went in for hiding them now. > (Not that I actually like the effect, especially on the media tab.) I'm going to make it go back to disabled instead of hidden. > > >+treechildren::-moz-tree-cell-text(broken) { > >+ font-style: italic; > >+ color: graytext; > > } > #999999 (I assume this is on a white background) yea > > >+#feedListbox richlistitem { > >+ padding-top: 6px; > >+ padding-bottom: 6px; > >+ -moz-padding-start: 7px; > >+ -moz-padding-end: 7px; > >+ min-height: 25px; > >+ border-bottom: 1px dotted #C0C0C0; > >+} > #C0C0C0 is not a typical Modern colour. Try #C7D0D9. done > > >+#feedListbox richlistitem[selected="true"] { > >+ background-color: -moz-Dialog; > >+ color: -moz-DialogText; > >+} > And these are definitely not Modern colours. removed as above > > >+ -moz-padding-start: 7px; > >+ -moz-padding-end: 7px; > See above. > > >+ background-color: -moz-dialog; > Etc. > > >+/* Security Tab */ > Etc. > done, done and done. > >+* content/navigator/pageinfo/feeds.js (pageinfo/feeds.js) > >+* content/navigator/pageinfo/feeds.xml (pageinfo/feeds.xml) > >+ content/navigator/pageinfo/pageInfo.css (pageinfo/pageInfo.css) > >+* content/navigator/pageinfo/pageInfo.js (pageinfo/pageInfo.js) > >+* content/navigator/pageinfo/pageInfo.xul (pageinfo/pageInfo.xul) > >+* content/navigator/pageinfo/permissions.js (pageinfo/permissions.js) > >+* content/navigator/pageinfo/security.js (pageinfo/security.js) > Try to avoid the *s. Use real comments for a start. I'll see what I can do. > > >+richlistitem[feed]:not([selected="true"]) .feed-subscribe { > >+ display: none; > >+} > Etc. > > >+groupbox[closed="true"] > .groupbox-body { > >+ visibility: collapse; > >+} > See above. done > > >+#ifdef XP_MACOSX > >+<?xul-overlay href="chrome://navigator/content/macBrowserOverlay.xul"?> > >+#endif > We don't have this. > removed > >+#ifdef XP_MACOSX > >+ <key key="." modifiers="meta" command="cmd_close"/> > >+#endif > Just leave the key in for all platforms. left in > > >+ <tab id="feedTab" label="&feedTab;" accesskey="&feedTab.accesskey;" > >+ oncommand="showTab('feed');" hidden="true"/> > Unfortunately tabs don't take to being hidden and shown. > Can you try disabling them instead? hmm, I'll see what that looks like. > > >+#ifdef XP_MACOSX > >+#include ../browserMountPoints.inc > >+#endif > Etc. > removed > >+function realmHasPasswords(location) { > >+ if (!location) > >+ return false; > >+ > >+ try { > >+ var passwordManager = Components.classes["@mozilla.org/login-manager;1"] > >+ .getService(Components.interfaces.nsILoginManager); > I hope we get to switch to login manager soon :-) > > >+<!ENTITY securityView.privacy.history "Have I visited this website before today?"> > Where does this get used (or couldn't you port it?) I had removed it, but on discussing it with KaiRo, I've added it back but ifdef'd it out. It relies on the places history backend for it's information, but he hopes to have that checked in for Seamonkey soon.

(In reply to comment #6) >(In reply to comment #5) >>(From update of attachment 275423 [details] [diff] [review]) >>>- * The Initial Developer of the Original Code is Daniel Brooks. >>Isn't it? >Isn't it what? I am the original developer for some of these files. Then why the -? >>>+textbox { >>> background: transparent !important; >>> border: none; >>> padding: 0px; >>>+ margin-top: 1px; >>> -moz-appearance: none; >>> } >>Why the margin change? [Why not use textbox class="plain"?] >It was necessary at the time, but I don't know if it's still needed. Well please consider removing it. >>>+ border-bottom: 1px dotted #C0C0C0; >>Classic does not hardcode colours. >yea, what should I replace it with? ThreeDFace (sorry I looked it up for Modern but forgot to check Classic) >>>+#permList > vbox:hover { >>>+ background-color: -moz-dialog; >>>+} >>Hover looks weird. But I wouldn't mind if you switched to a richlistbox. >eh. that's a lot of work. Then just get rid of the hover please. >>>+<!ENTITY securityView.privacy.history "Have I visited this website before today?"> >>Where does this get used (or couldn't you port it?) >I had removed it, but on discussing it with KaiRo, I've added it back but >ifdef'd it out. What's wrong with commenting it out?

Comment on attachment 275441 [details] [diff] [review] 379183-3.diff I notice that you keep requesting review from the wind - if you want a different reviewer you could do worse than IanN. In addition to comment #8: You removed the -moz-appearance: none; from the Classic pageInfo.css and not Modern when in fact you should have removed it from Modern, not Classic. The caption-icon class is not used and its styles can be removed. The feedListbox richlistitem is still a descendant selector, and the padding isn't specified using shorthand. You could retrive the general visit count from xpfe history if you feel like poking around in RDF ;-) but you omitted the definition of previousVisitCount. I understand why you #ifdef MOZ_PLACES and not commented it. Although you're showing all of the media textboxes you're not indicating those that have been left blank so you're still not making use of the disabled rule. You're still showing hidden tabs. As an alternative to my previous suggestion of disabling unused tabs you should be fine if you start off with all tabs showing and hide the ones you don't want.

H,, does this remove the link tab completely? I recently read a comments of someone who uses it a lot, and it's probably useful to web developers, etc. - see http://home.kairo.at/blog/2007-08/weekly_status_report_w31_2007#comments I understand why Firefox doesn't want it, I think we might still want to keep it.

(In reply to comment #9) > (From update of attachment 275441 [details] [diff] [review]) > I notice that you keep requesting review from the wind - if you want a > different reviewer you could do worse than IanN. Yea, dunno why that happened. Also, I don't know why I requested any reviews at all on that last patch, I had intended it only as a work-in-progress. Sorry to bother you with an extra review request, though you did catch some stuff I hadn't noticed. > > In addition to comment #8: > > You removed the -moz-appearance: none; from the Classic pageInfo.css and not > Modern when in fact you should have removed it from Modern, not Classic. oops :) > > The caption-icon class is not used and its styles can be removed. cool > > The feedListbox richlistitem is still a descendant selector, and the padding > isn't specified using shorthand. > > You could retrive the general visit count from xpfe history if you feel like > poking around in RDF ;-) but you omitted the definition of previousVisitCount. eww, no thanks. > > I understand why you #ifdef MOZ_PLACES and not commented it. > > Although you're showing all of the media textboxes you're not indicating those > that have been left blank so you're still not making use of the disabled rule. Yea, I left off for the day just on the point of looking up what the previous sm pageinfo used as it's text in that case. > > You're still showing hidden tabs. As an alternative to my previous suggestion > of disabling unused tabs you should be fine if you start off with all tabs > showing and hide the ones you don't want. > Yea, I'd prefer to have all of the tabs available all the time. I tried disabling them to see what it's like, but decided against it.

(In reply to comment #10) > H,, does this remove the link tab completely? I recently read a comments of > someone who uses it a lot, and it's probably useful to web developers, etc. - > see http://home.kairo.at/blog/2007-08/weekly_status_report_w31_2007#comments > I understand why Firefox doesn't want it, I think we might still want to keep > it. > Yes, I'm planning on adding them back in.

Oh... adding "Open links in new tab" and "Save as.." or "Save all..." on the links tab would be sooo nice.

now I think it's ready for prime time.

Comment on attachment 275762 [details] [diff] [review] 379183-4.diff >Index: suite/browser/pageinfo/feeds.js >=================================================================== >+function onSubscribeFeed() >+{ >+ var listbox = document.getElementById("feedListbox"); >+ openUILink(listbox.selectedItem.getAttribute("feedURL"), >+ null, false, true, false, null); >+} We probably want to use the general openURL function there that bug 384139 should make available...

>>+ openUILink(listbox.selectedItem.getAttribute("feedURL"), >>+ null, false, true, false, null); >>+} > >We probably want to use the general openURL function there that bug 384139 >should make available... Did Asrail's patch get checked in? I don't see openUILink in the latest SuiteRunner builds.

Comment on attachment 275807 [details] [diff] [review] 379183-5.diff >Index: suite/browser/pageinfo/pageInfo.xul >=================================================================== >+ <tabs> >+ <tab id="generalTab" label="&generalTab;" accesskey="&generalTab.accesskey;" >+ oncommand="showTab('general');"/> >+ <tab id="mediaTab" label="&mediaTab;" accesskey="&mediaTab.accesskey;" >+ oncommand="showTab('media'); ensureSelection(gImageView)"/> >+ <tab id="feedTab" label="&feedTab;" accesskey="&feedTab.accesskey;" >+ oncommand="showTab('feed');"/> >+ <tab id="permTab" label="&permTab;" accesskey="&permTab.accesskey;" >+ oncommand="showTab('perm');"/> >+ <tab id="formsTab" label="&formsTab;" accesskey="&formsTab.accesskey;" >+ oncommand="ensureSelection(gFormView)"/> >+ <tab id="linksTab" label="&linksTab;" accesskey="&linksTab.accesskey;" >+ oncommand="ensureSelection(gLinkView)"/> >+ <tab id="securityTab" label="&securityTab;" accesskey="&securityTab.accesskey;" >+ oncommand="showTab('security');"/> >+ <!-- Others added by overlay --> >+ </tabs> I can't find the definition of showTab()...

Oh, and regarding comment #15, the openURL() function is now available from the global contentAreaUtils.js, which you already include anyways, so that might be a good idea to use.

Comment on attachment 275807 [details] [diff] [review] 379183-5.diff >Index: suite/locales/en-US/chrome/browser/pageInfo.dtd >=================================================================== >-<!ENTITY pageInfoWindow.width "425"> >-<!ENTITY pageInfoWindow.height "470"> >+<!ENTITY pageInfoWindow.dimensions "width: 100ch; height: 30em;"> This looks good, but I think adding a L10n note that tells the localizers to only adapt the values and not translate the words "width" and "height" here would make sense (yes, we repeatedly have such cases).

(In reply to comment #18) >I can't find the definition of showTab()... In fact it shouldn't be necessary, as the tabs are in a tabbox.

Some issues noted while trying out the latest patch: 1. Except for the Media tree, the other trees should only be single selection, in particular the Forms tree, where it makes no sense to be multiple. 2. When switching forms, the form elements tree row count changes unexpectedly. 3. The text "This web site provides a certificate to verify its identity." has a spurious accesskey.

Comment on attachment 275807 [details] [diff] [review] 379183-5.diff >+ var feedListbox = document.getElementById("feedListbox"); >+} Very useful... >+function onSubscribeFeed() >+{ >+ var listbox = document.getElementById("feedListbox"); >+ openUILink(listbox.selectedItem.getAttribute("feedURL"), >+ null, false, true, false, null); >+} This is unlikely to work, whatever the definition of openUILink... >+ set rowCount(c) { throw "rowCount is a readonly property"; }, Simply omitting the setter has a similar effect... >+ return this.data[row][column.index] || ""; I see the original code had the || "", but you can remove it unless you have some JS callers that expect empty strings (PaintTreeCell doesn't care). >+ this.rows = this.data.push(row); >+ this.rowCountChanged(this.rows - 1, 1); push returns the new length? Neat. >+ if (this.tree) >+ this.tree.rowCountChanged(0, -this.rows); >+ this.rows = 0; >+ this.data = [ ]; You need to clear rows before calling rowCountChanged (you could pass -this.data.length instead). Nit: No space inside []; >+ return (row < 0 || this.copycol < 0) ? "" : (this.data[row][this.copycol] || ""); || "" again? >+ getParentIndex: function(index) { return 0; }, Nit: -1 (0 is certainly not its own parent!) >+ props.AppendElement(aserv.getAtom("broken")); Why not cache the atom at startup? >+function doHelpButton() I see no help button... >+ var deck = document.getElementById("mainDeck"); >+ var helpdoc = helpTopics[deck.selectedPanel.id] || "pageinfo_general"; This is wrong for a tabbox... >+ openHelp(helpdoc, 'chrome://browser/locale/help/help.rdf'); Wrong help file... and file a bug on updating suite help! >+function toggleGroupbox(id) Obsolete. >+function ensureSelection(view) I don't see this having any effect. >+ if (!gFormView.rowCount) return; >+ >+ if (gFormView.selection.count == 1) This implies !gFormView.rowCount so you're wasting a test. >+ window.open(url, "_blank", "chrome"); Should use openNewWindowWith (or some similar method). After reaching the copy code I now see why you allow multiple selections, but you need to come up with something that works with the forms tree.

(In reply to comment #21) > (In reply to comment #18) > >I can't find the definition of showTab()... > In fact it shouldn't be necessary, as the tabs are in a tabbox. > Yea, I removed the function, but not the callers that were in the xul file. oops. (In reply to comment #22) > 2. When switching forms, the form elements tree row count changes unexpectedly. Hmm, it had this problem before, but it was supposed to be fixed. > > 3. The text "This web site provides a certificate to verify its identity." has > a spurious accesskey. I can't decide whether that's a feature or a bug… <label id="security-view-text" class="fieldLabel" control="security-view-cert" flex="1"/> <button id="security-view-cert" label="&securityView.certView;" accesskey="&securityView.accesskey;" oncommand="security.viewCert();"/> The label has a control property, and the button it refers to has the access key; the access key is used on both. (In reply to comment #23) > (From update of attachment 275807 [details] [diff] [review]) > >+ var feedListbox = document.getElementById("feedListbox"); > >+} > Very useful... > heh > >+function onSubscribeFeed() > >+{ > >+ var listbox = document.getElementById("feedListbox"); > >+ openUILink(listbox.selectedItem.getAttribute("feedURL"), > >+ null, false, true, false, null); > >+} > This is unlikely to work, whatever the definition of openUILink... I know, but the subscribe button is hidden, so the code can't be called. I'll comment it out like the button. > >+ this.rows = this.data.push(row); > >+ this.rowCountChanged(this.rows - 1, 1); > push returns the new length? Neat. yea, that is neat. > >+ if (this.tree) > >+ this.tree.rowCountChanged(0, -this.rows); > >+ this.rows = 0; > >+ this.data = [ ]; > You need to clear rows before calling rowCountChanged (you could pass > -this.data.length instead). Nit: No space inside []; Hmm, that might be what's causing the unexpected row count change. > >+ getParentIndex: function(index) { return 0; }, > Nit: -1 (0 is certainly not its own parent!) heh. > >+ props.AppendElement(aserv.getAtom("broken")); > Why not cache the atom at startup? cached > >+function doHelpButton() > I see no help button... removed > >+function toggleGroupbox(id) > Obsolete. removed > >+function ensureSelection(view) > I don't see this having any effect. It selects the first item in the list, unless something is already selected or there's nothing in the list. This is called when you switch tabs, so the first time you see each tab, something is selected. (The image tab looks best that way, for example). > > >+ if (!gFormView.rowCount) return; > >+ > >+ if (gFormView.selection.count == 1) > This implies !gFormView.rowCount so you're wasting a test. true, removed the first test. > > >+ window.open(url, "_blank", "chrome"); > Should use openNewWindowWith (or some similar method). done > After reaching the copy code I now see why you allow multiple selections, but > you need to come up with something that works with the forms tree. Hmm, it seems to work just fine for me. How would you prefer it work for the forms tree?

Oops. This is the same patch, but this time I didn't forget the -N flag.

(In reply to comment #24) >The label has a control property, and the button it refers to has the access >key; the access key is used on both. I strongly suspect that buttons don't need controlling labels, as they already have their own text. Please remove it unless aaronlev says otherwise. >>After reaching the copy code I now see why you allow multiple selections, but >>you need to come up with something that works with the forms tree. >Hmm, it seems to work just fine for me. How would you prefer it work for the >forms tree? Well, I'd like it do something useful when you press Ctrl+A, and not just leave stale data in the bottom view - the Media tab gets this right ;-)

For the next versions of the patch, you may want to take the change suggested by bug 393986.

Comment on attachment 278708 [details] [diff] [review] 379183-6b.diff >+MOZ_PLACES=1 Oops ;-) >+* content/navigator/pageinfo/feeds.js (pageinfo/feeds.js) >+* content/navigator/pageinfo/feeds.xml (pageinfo/feeds.xml) >+ content/navigator/pageinfo/pageInfo.css (pageinfo/pageInfo.css) >+* content/navigator/pageinfo/pageInfo.js (pageinfo/pageInfo.js) >+* content/navigator/pageinfo/pageInfo.xul (pageinfo/pageInfo.xul) >+* content/navigator/pageinfo/permissions.js (pageinfo/permissions.js) >+* content/navigator/pageinfo/security.js (pageinfo/security.js) Only pageInfo.xul and security.js use #ifdef, right? >+# -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ Sigh. Just stick to JS comments throughout, please? >+<?xml version="1.0"?> >+# ***** BEGIN LICENSE BLOCK ***** And xml comments please :-P >+ <tab id="mediaTab" label="&mediaTab;" accesskey="&mediaTab.accesskey;" >+ oncommand="ensureSelection(gImageView)"/> oncommand doesn't work with Ctrl+Tab... >+ <label id="security-identity-verifier-label" >+ class="fieldLabel" >+ value="&securityView.identity.verifier;" >+ control="security-identity-verifier-value"/> So, after all that, aaronlev said to use description.

Comment on attachment 278708 [details] [diff] [review] 379183-6b.diff >Index: suite/locales/en-US/chrome/browser/pageInfo.properties >=================================================================== >@@ -69,18 +83,13 @@ > linkStylesheet=Stylesheet > linkRev=Reverse Link > linkX=Simple XLink >+yes=Yes Please don't add this one. You are already adding a "yes" property to the same file above, doubled properties are useless ;-)

Ok, this should be perfect, though I suppose it's possible that I forgot something along the way. Doesn't look like it though. I've fixed everything in comment 28, and removed the duplicate entry mentioned in comment 29.

Comment on attachment 280608 [details] [diff] [review] 379183-7.diff I still don't like the multiselection on the forms tree... >+ <description id="security-view-text" class="fieldLabel" >+ control="security-view-cert" flex="1"/> This description is correct but the other label changes were wrong. >Index: themes/classic/navigator/pageInfo.css >Index: themes/modern/navigator/pageInfo.css Note: these moved to suite/themes/*/navigator/pageInfo.css The other thought I had was that perhaps you should ensure the selection once processing is complete, rather than each time you switch tabs.

patch to be checked in

checked in

(In reply to comment #8) >(In reply to comment #6) >>(In reply to comment #5) >>>(From update of attachment 275423 [details] [diff] [review]) >>>>+ border-bottom: 1px dotted #C0C0C0; >>>Classic does not hardcode colours. >>yea, what should I replace it with? >ThreeDFace (sorry I looked it up for Modern but forgot to check Classic) This never got done :-(