Closed Bug 195492 Opened 17 years ago Closed 15 years ago

Tidy up media tab in page info

Categories

(SeaMonkey :: Page Info, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: piers, Assigned: db48x)

References

Details

Attachments

(5 files, 13 obsolete files)

31.23 KB, image/png
Details
15.90 KB, image/png
Details
12.71 KB, image/png
Details
111.43 KB, patch
neil
: review+
Details | Diff | Splinter Review
1.87 KB, patch
neil
: review+
Details | Diff | Splinter Review
Some minor issues in the page info -> media tab.

* "Title Attribute" should be just "Title";
* Physical size label shouldn't make interface move about;
* Add groupbox;
* "Source" -> "Cache Source" (is this right?);
* Move Save As button down;
* Change Save As accesskey to "A" for consistancy;
* Standardise format of "Not Specified", "Unknown", "Not Cached", etc.

Daniel, Neil: let me know which, if any of these you think are good.

Perhaps we could also have tooltips for each attribute to describe what they
refer to?
Attached image Screenshot of changes
* Possibly also add "chrome" as an allowed protocol.
I think those changes are all pretty good.

The groupbox might be a bit over the top, but I really don't know.

Just noticed the dimensions though, put those back up on one line like it was.

you know, maybe one way to get more on the screen, especially at low
resolutions, is to use a smaller font size for the image details. try doing
font-size: smaller; in the css and see what happens.
allowing the chrome format would be fine, imho, but not a part of this bug.
besides, I'd want to double check first. There might even be a different bug
about it already.

the not specified/unknown stuff could go for all the tabs.

As for the physical size stuff, how do you propose to do it?

oh, and it's not so bad at 640x480 anymore, now that the help button is gone
there's some extra room. :)
Blocks: 181057
Besides the proposed changes, which I approve of, I would also like to propose
that each line in the list be numbered, which would be very helpful on pages
that have hundreds of images. 

Making the list sortable by column would also be very useful.
you liked those numbers? hmm. I guess I can add them back. File a seperate bug
for it and assign it to me.
Per, i don't think the numbers are a good idea: the number wouldn't mean anything
at all - would the number be locked to an entry so that when resorted 
(once sorting is implemented) the numbers would go out of order?! If not, then the
numbers would mean absolutely nothing (well, they wouldn't mean anything 
anyway...)

Sortable columns is bug 76170.
Re comment #7: At the very least, numbers tell you how many images (or
occurrences, to be more precise) there are on a page. In itself, the number
would say something about the relative position of the image, so it's not
completely meaningless unless the list is populated in a non-obvious order.

I think I wouldn't want the list to be renumbered when sorted - I agree that
that would be meaningless.

I think image numbers are as useful as line numbers in an editor: you may not
need them most of the time, but sometimes they are very useful, especially if
you need to find your way among hundreds of images/lines.

I probably will open a bug report for this. We may then continue the discussion
over there.
Blocks: 195546
Attached patch Page info meda tab (obsolete) — Splinter Review
Attachment #116029 - Flags: review?(db48x)
Comment on attachment 116029 [details] [diff] [review]
Page info meda tab

>Index: xpfe/browser/resources/content/pageInfo.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/pageInfo.js,v
>retrieving revision 1.51
>diff -u -r1.51 pageInfo.js
>--- xpfe/browser/resources/content/pageInfo.js	28 Feb 2003 02:34:01 -0000	1.51
>+++ xpfe/browser/resources/content/pageInfo.js	2 Mar 2003 14:04:36 -0000
>@@ -782,8 +782,29 @@
>   var url = imageView.getCellText(row, "image-address");
>   var isBG = imageView.getCellText(row, "image-bg");
> 
>-  document.getElementById("imageurltext").value = url;
>-  document.getElementById("imagetitletext").value = item.title || gStrings.notSet;
>+  // set title attribute  

comment should say url attribute, I guess

>+  if (url) 
>+  {  
>+    document.getElementById("imageurltext").value = url;
>+    document.getElementById("imageurltext").setAttribute("class", "plain");    
>+  }
>+  else 
>+  {
>+    document.getElementById("imageurltext").value = gStrings.notSet;
>+    document.getElementById("imageurltext").setAttribute("class", "plain unset");
>+  }
>+
>+  // set title attribute  
>+  if (item.hasAttribute("title") && ("title" in item))

I don't remember why it needed both those checks.

>+  {  
>+    document.getElementById("imagetitletext").value = ("title" in item && item.title);
>+    document.getElementById("imagetitletext").setAttribute("class", "plain");    
>+  }
>+  else 
>+  {
>+    document.getElementById("imagetitletext").value = gStrings.notSet;
>+    document.getElementById("imagetitletext").setAttribute("class", "plain unset");
>+  }
> 
>   var altText = null;
>   if (item.hasAttribute("alt") && ("alt" in item))
>@@ -793,35 +814,51 @@
>   if (altText == null)
>     altText = gStrings.notSet;
>   var textbox=document.getElementById("imagealttext");
>-  
>-  // IMO all text that is not really the value text should go in italics
>-  // What if somebody has <img alt="Not specified">? =)
>+
>+  // Set alt attribute
>   // We can't use textbox.style because of bug 7639
>-  if (!altText) {
>-      textbox.value = gStrings.emptyString;
>-      textbox.setAttribute("style","font-style:italic");
>-  } else {
>-      textbox.value = altText;
>-      textbox.setAttribute("style","font-style:inherit");
>+  if (!altText) 

All of these tests should be the same, so remvoe the negation.

>+  {
>+    textbox.value = gStrings.notSet;
>+    textbox.setAttribute("class", "plain unset");
>+  } 
>+  else 
>+  {
>+    textbox.value = altText;
>+    textbox.setAttribute("class", "plain");
>+  }
>+  
>+  // set longdesc attribute
>+  if (item.hasAttribute("longdesc")) 

Here's one with just one of the two tests I mentioned.

>+  {
>+    document.getElementById("imagelongdesctext").value = ("longDesc" in item && item.longDesc);
>+    document.getElementById("imagelongdesctext").setAttribute("class", "plain");
>+  }
>+  else 
>+  {
>+    document.getElementById("imagelongdesctext").value = gStrings.notSet;
>+    document.getElementById("imagelongdesctext").setAttribute("class", "plain unset");
>   }
>-  document.getElementById("imagelongdesctext").value = ("longDesc" in item && item.longDesc) || gStrings.notSet;
> 
>   // get cache info
>   var sourceText = theBundle.getString("generalNotCached");
>-  var expirationText = gStrings.unknown;
>+  var expirationText;
>   var sizeText = gStrings.unknown;
> 
>-  var pageSize = 0; 
>-  var kbSize = 0;
>+  var pageSize; 
>+  var kbSize;
>   var expirationTime = 0;
>   var expirationDate = null;
> 
>-  try
>+  document.getElementById("imagesourcetext").setAttribute("class", "plain"); 
>+
>+  // set cache source text
>+  try 
>   {
>     var cacheEntryDescriptor = httpCacheSession.openCacheEntry(url, Components.interfaces.nsICache.ACCESS_READ, false);   // open for READ, in non-blocking mode
>-    if (cacheEntryDescriptor)
>+    if (cacheEntryDescriptor) 

please don't add extra whitespace.

>     {
>-      switch(cacheEntryDescriptor.deviceID)
>+      switch(cacheEntryDescriptor.deviceID) 
>       {
>         case "disk":
>           sourceText = theBundle.getString("generalDiskCache");
>@@ -835,14 +872,13 @@
>       }
>     }
>   }
>-  catch(ex)
>+  catch(ex) 
>   {
>-    try
>+    try 
>     {
>       cacheEntryDescriptor = ftpCacheSession.openCacheEntry(url, Components.interfaces.nsICache.ACCESS_READ, false);   // open for READ, in non-blocking mode
>-      if (cacheEntryDescriptor)
>-      {
>-        switch(cacheEntryDescriptor.deviceID)
>+      if (cacheEntryDescriptor) {
>+        switch(cacheEntryDescriptor.deviceID) 
>         {
>           case "disk":
>             sourceText = theBundle.getString("generalDiskCache");
>@@ -856,23 +892,31 @@
>         }
>       }
>     }
>-    catch(ex2)
>+    catch(ex2) 
>     {
>       sourceText = theBundle.getString("generalNotCached");
>+      document.getElementById("imagesourcetext").setAttribute("class", "plain unset"); 
>     }
>   }
> 
>   // find out the mime type, file size and expiration date
>   var mimeType = gStrings.unknown, httpType;
>-  if (cacheEntryDescriptor)
>+  if (cacheEntryDescriptor) 
>   {
>     var headers, match;
> 
>     pageSize = cacheEntryDescriptor.dataSize;
>     kbSize = pageSize / 1024;
>-    sizeText = theBundle.getFormattedString("generalSize", [Math.round(kbSize*100)/100, pageSize]);
>+    if (pageSize < 1000) 
>+    {
>+      sizeText = theBundle.getFormattedString("byteSize",[pageSize]);
>+    }
>+    else 
>+    {
>+      sizeText = theBundle.getFormattedString("generalSize", [Math.round(kbSize*100)/100, pageSize]);
>+    }
> 
>-    expirationText = formatDate(cacheEntryDescriptor.expirationTime*1000, gStrings.notSet);
>+    expirationText = formatDate(cacheEntryDescriptor.expirationTime*1000, null);
> 
>     headers = cacheEntryDescriptor.getMetaDataElement("response-head");
> 
>@@ -881,11 +925,41 @@
>       httpType = match[1];
>   }
> 
>+  if (!expirationText) 
>+  {
>+    expirationText = gStrings.unknown;
>+    document.getElementById("imageexpirestext").setAttribute("class", "plain unset");
>+  }
>+  else 
>+  {
>+    document.getElementById("imageexpirestext").setAttribute("class", "plain");
>+  }
>+  
>+  if (!pageSize) 
>+  {
>+    sizeText = gStrings.unknown;
>+    document.getElementById("imagesizetext").setAttribute("class", "plain unset");
>+  }
>+  else 
>+  {
>+    document.getElementById("imagesizetext").setAttribute("class", "plain");
>+  }
>+
>   if (!(item instanceof nsIInputElement))
>     mimeType = ("type" in item && item.type) ||
>                ("codeType" in item && item.codeType) ||
>                ("contentType" in item && item.contentType) ||
>-               httpType || gStrings.unknown;
>+               httpType;
>+  
>+  if (!mimeType) 
>+  {
>+    mimeType = gStrings.unknown;
>+    document.getElementById("imagetypetext").setAttribute("class", "plain unset");
>+  }
>+  else 
>+  {
>+    document.getElementById("imagetypetext").setAttribute("class", "plain");
>+  }
> 
>   document.getElementById("imagetypetext").value = mimeType;
>   document.getElementById("imagesourcetext").value = sourceText;
>@@ -919,23 +993,24 @@
>     // fallback image for protocols not allowed (e.g., data: or javascript:) 
>     // or elements not [yet] handled (e.g., object, embed). XXX blank??
>     newImage.src = "resource:///res/loading-image.gif";
>-    newImage.width = 40;
>-    newImage.height = 40;

any particular reason for removing these?

>   }
> 
>   var width = ("width" in item && item.width) || ("width" in newImage && newImage.width) || "0";
>   var height = ("height" in item && item.height) || ("height" in newImage && newImage.height) || "0";
> 
>-  document.getElementById("imageSize").value = theBundle.getFormattedString("mediaSize", [width, height]);
> 
>-  if (width != physWidth || height != physHeight)
>+  if (width != physWidth || height != physHeight)  
>   {
>-    document.getElementById("physSize").removeAttribute("hidden");
>-    document.getElementById("physSize").value = theBundle.getFormattedString("mediaPhysSize", [physWidth, physHeight]);
>+    document.getElementById("imageWidth").value = theBundle.getFormattedString("mediaSize", [width]);
>+    document.getElementById("imageHeight").value = theBundle.getFormattedString("mediaSize", [height]);
>+    document.getElementById("imageWidth").value += " " + theBundle.getFormattedString("mediaPhysSize", [physWidth]);
>+    document.getElementById("imageHeight").value += " " + theBundle.getFormattedString("mediaPhysSize", [physHeight]);

do both of these all in one expression (they don't have to be a single like
though)

document.getElementById("imageWidth").value = 
  theBundle.getFormattedString("mediaSize", [width]) +
  theBundle.getFormattedString("mediaPhysSize", [physWidth]);

>+  }
>+  else 
>+  {
>+    document.getElementById("imageWidth").value = theBundle.getFormattedString("mediaSize", [width]);
>+    document.getElementById("imageHeight").value = theBundle.getFormattedString("mediaSize", [height]);
>   }
>-  else
>-    document.getElementById("physSize").setAttribute("hidden", "true");
>-
> 
>   imageContainer.removeChild(oldImage);
>   imageContainer.appendChild(newImage);
>@@ -945,6 +1020,7 @@
> //******** Other Misc Stuff
> // Modified from the Links Panel v2.3, http://segment7.net/mozilla/links/links.html
> // parse a node to extract the contents of the node
>+// linkNode doesn't really _have_ to be link

don't add that back in, there is no varible called linkNode ;)

> function getValueText(node)
> {
>   var valueText = "";
>Index: xpfe/browser/resources/content/pageInfo.xul
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/pageInfo.xul,v
>retrieving revision 1.44
>diff -u -r1.44 pageInfo.xul
>--- xpfe/browser/resources/content/pageInfo.xul	28 Feb 2003 11:20:14 -0000	1.44
>+++ xpfe/browser/resources/content/pageInfo.xul	2 Mar 2003 14:04:38 -0000
>@@ -241,74 +241,79 @@
>           </treecols>
>           <treechildren flex="1"/>
>         </tree>
>-        <splitter collapse="after" orient="vertical"/>
>-        <vbox flex="1">
>-          <grid>
>-            <columns>
>-              <column/>
>-              <column style="width: .5em;"/>
>-              <column flex="1"/>
>-            </columns>
>-            <rows>
>-              <row>
>-                <label value="&mediaURL;"/>
>-                <separator/>
>
>-                <textbox readonly="true" crop="right" id="imageurltext"/>
>-              </row>
>-              <row>
>-                <label value="&mediaTitle;"/>
>-                <separator/>
>-                <textbox readonly="true" crop="right" id="imagetitletext"/>
>-              </row>
>-              <row>
>-                <label value="&mediaAlt;"/>
>-                <separator/>
>-                <textbox readonly="true" crop="right" id="imagealttext"/>
>-              </row>
>-              <row>
>-                <label value="&mediaLongdesc;"/>
>-                <separator/>
>-                <textbox readonly="true" crop="right" id="imagelongdesctext"/>
>-              </row>
>-              <row>
>-                <label value="&generalType;"/>
>-                <separator/>
>-                <textbox readonly="true" crop="right" id="imagetypetext"/>
>-              </row>
>-              <row>
>-                <label value="&generalSource;"/>
>-                <separator/>
>-                <textbox readonly="true" crop="right" id="imagesourcetext"/>
>-              </row>
>-              <row>
>-                <label value="&generalSize;"/>
>-                <separator/>
>-                <textbox readonly="true" crop="right" id="imagesizetext"/>
>-              </row>
>-              <row>
>-                <label value="&generalExpires;"/>
>-                <separator/>
>-                <textbox readonly="true" crop="right" id="imageexpirestext"/>
>-              </row>
>-              <row>
>-                <label value="&mediaDimensions;"/>
>-                <separator/>
>-                <vbox>
>-                  <textbox readonly="true" crop="right" id="imageSize"/>
>-                  <textbox readonly="true" crop="right" id="physSize"/>
>-                </vbox>
>-              </row>
>-            </rows>
>-          </grid>
>-          <hbox>
>-            <button label="&mediaSaveAs;" accesskey="&mediaSaveAsAccesskey;" id="imagesaveasbutton" disabled="true" oncommand="saveMedia();"/>
>-          </hbox>
>-          <vbox class="inset iframe" flex="1" pack="center">
>-            <hbox id="theimagecontainer" pack="center">
>-              <image id="thepreviewimage"/>
>-            </hbox>
>-          </vbox>
>-        </vbox>
>+         <splitter collapse="after" orient="vertical"/>
>+         <vbox flex="1">
>+         <groupbox>
>+         <caption label="Media Info"/>
>+       

The patch just seems to end here, which can't be right :)
Attachment #116029 - Flags: review?(db48x) → review-
whoops, meant to trim the length a bit before I submitted :)
And now for the rest of the patch:

+         <vbox flex="1">
+         <groupbox>
+         <caption label="Media Info"/>
+           <grid>
+             <columns>
...
+               <row>
+ 	        <label value="Height:"/>
+ 	        <separator/>
+ 	        <textbox readonly="true" class="plain" id="imageHeight"/>
+               </row>

keep the indentation going, it makes it easier to read

The rest of the changes look ok.

On the whole, however, I don't think I like the change to the why the dimensions
are given. I think I prefer the width and height to be on a single line.
Something I noticed which you could fix at the same time:
Page Info on about: won't display the logo in the media tab (it's in chrome).
Instead, it displays "loading-image.gif" at 40x40, which is silly because
a) it should be "broken-image.gif"
b) the natural size is 14x16 so 40x40 isn't even 1:1 scaling
Hmm... perhaps resource: should also be an allowed protocol...
Doh, must remember to read the bug before commenting :-/
I think that the Not Specified should be its own <label> rather than juggling
styles on the <input>. (Use a <deck> to flip between the two?)
>-    newImage.width = 40;
>-    newImage.height = 40;

Why scale this icon (especially if we're changing the ratio)? The icon is fine in
its native size. Neil, i think it's better to use loading-image because the image
isn't broken, we just haven't loaded it in page info.

I'll have a look on XUL Planet to see what deck is, i've never used it before.
Piers, thanks for pointing out my silliness, I'll amend my comment to:

Doh, must remember to read the bug and all patches before commenting :-/

Don't worry about the deck, I've changed my mind, disable the textbox instead.
Piers: ah, ok. I hadn't realized we were changing the ratio of the picture.
Neil, why do you want the textboxes disabled? - it doesn't really matter if you 
can select the text in them does it? (and it seems to me more natural if you're
tabbing through them to tab through each one)

I'll show the broken-image.gif when there's no "src". :)
Piers: enable the text box when there is real alt (or whatever) text.
      disable the text box when the text is your "None Specified" default.
Attached patch Page info meda tab (obsolete) — Splinter Review
Ok, change the "unset" indicator to just disable the textbox per comment 21.
Attachment #116029 - Attachment is obsolete: true
Attachment #116318 - Flags: review?(db48x)
Adding depency, which request adding a count to the media lines indicating the
number of times a image is used.
Depends on: 201264
Please add a title to the preview pane as part of the fixed window section.
Currently if the window happens to be smaller than needed to shwo the preview
image but not to small to cut off anything else on the pane you have no idea
that the preview pane exists! A title "Image Preview" above or below (or instead
of) the horizontal line will indicate there is more that can be seen if you
resize. See my screenshot for example:
Comment on attachment 123876 [details]
Screenshot of Page Info -> Media Tab under certain resize

Perhaps wrapping the preview box in a groupbox would make it more obvious?
i'm not sure but I definately think it needs a label
Attached patch patch (obsolete) — Splinter Review
clean up, account for some bit rot, touch up a few other things, same changes
to firefox.
Assignee: piers → db48x
Attachment #116318 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #116318 - Flags: review?(db48x)
Attachment #148635 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 148635 [details] [diff] [review]
patch

>+  var tmp = new String(url.toString);
Did you mean url.toString() which you might want to put in the initializer i.e.
var url = theDocument.location.toString();

>+  var cacheKey = tmp.substring(0, tmp.search('#'));
This won't work if tmp doesn't contain a # - you could try tmp.replace(/#.*$/,
"");

>+    // stop the spinner
>+    document.getElementById("tabs").setAttribute("finished", "true");
Consider using window.setCursor("spinning"); (and "auto") instead.

>+  if (item.hasAttribute("title") && ("title" in item)) 
>+  {  
>+    document.getElementById("imagetitletext").value = ("title" in item && item.title);
Duplicate in check.

>+    document.getElementById("imagelongdesctext").value = ("longDesc" in item && item.longDesc);
I think this might display "false" if the in test fails...

>-  var regex = new RegExp("^(https?|ftp|file|gopher)://");
>+  var regex = new RegExp("(https?|ftp|file|gopher|about|chrome|resource):");
Consider changing to const regex = /<foo>/; (the old one may have used new
RegExp to save quoting the /s?)

>+    document.getElementById("theimagecontainer").removeAttribute("collapsed");
>+    document.getElementById("brokenimagecontainer").setAttribute("collapsed", "true");

>+    document.getElementById("physRow").removeAttribute("hidden");
collapsed and hidden are best set via the property rather than the attribute.

>-<dialog id="main-window"
>+<window id="main-window"
Please change the DOCTYPE to match.

>+  button="none">
You don't need this attribute in a window.

>-                <textbox readonly="true" crop="right" id="imageexpirestext"/>
>+                <textbox readonly="true" crop="right" id="imageexpirestext"/> 
This line gained a trailing space :-(

>             <hbox id="theimagecontainer" pack="center">
>               <image id="thepreviewimage"/>
>             </hbox>
>+            <hbox id="brokenimagecontainer" pack="center" collapsed="true">
>+              <image id="brokenimage" src="resource:///res/broken-image.gif"/>
>+            </hbox>
Hmmm... if the url isn't permitted as the img src why not set the src to
broken-image rather than hiding that image and showing this one?

If you stick with the new .xml file it will need a licence header.
Attachment #148635 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #148635 - Attachment is obsolete: true
Attachment #153757 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch patch (obsolete) — Splinter Review
same changes in firefox, a few other minor tweaks.
Attachment #153757 - Attachment is obsolete: true
Attachment #153757 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch patch (obsolete) — Splinter Review
this should do it, but with the same changes to firefox as well. I think I got
everything Neil mentioned in his last review.
Attachment #153777 - Attachment is obsolete: true
Comment on attachment 164112 [details] [diff] [review]
patch

request r=
Attachment #164112 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch 195492-7.diff (obsolete) — Splinter Review
slight update. Not calling this one 'patch' for Neil's sake
Attachment #164112 - Attachment is obsolete: true
Attachment #164143 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 164112 [details] [diff] [review]
patch

request r= on newer patch
Attachment #164112 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 164143 [details] [diff] [review]
195492-7.diff

I've just had a quick flick through...

>+var threadID = null;
Unused?

>+  while (i < n && iterator.nextNode())
>+    ++i;
> 
>+  if (iterator.nextNode())
Is it safe to call nextNode after it has returned null?

>+  const regex = /^(https?|ftp|file|gopher|about|chrome|resource):/;
>+  var isProtocolAllowed = regex.test(url);
>+  if (/^data:/.test(url) && /^image\//.test(mimeType))
>+    isProtocolAllowed = 1;
What's wrong with true?

>+  number -= 0;  // coerce any strings to numbers
I discovered recently that (+number) works ;-)

>+function doSelectAll(event)
> {
>+  var elem;
>+  if (event instanceof Components.interfaces.nsIDOMMouseEvent)
>+    elem = document.popupNode.parentNode;
>+  else
>+    elem = event.target;
Surely Copy and Select All always work on the focused element?

>+  class="unfinished">
(How) does this work? What was wrong with window.setCursor?

>+    <key id="copykey" key="&copy.key;" modifiers="accel" command="cmd_copy"/>
>+    <key id="selectkey" key="&selectall.key;" modifiers="accel" command="cmd_selectall"/>
Now this is going to give you a bit of a headache. You see, select all is Alt+A
on linux. Furthermore, you'll block Ctrl+A and Ctrl+C in all text boxes. You
could of course implement a command controller, see mail3PaneWindowCommands.js
for how to implement a select all controller on a tree.

>+  <menupopup id="picontext">
>+    <menuitem label="&selectall.label;" command="cmd_selectall" accesskey="&selectall.key;" key="selectkey"/>
>+    <menuitem label="&copy.label;" command="cmd_copy" accesskey="&copy.key;" key="copykey"/>
>+  </menupopup>
Context menus don't display shortcut keys.

>+          <tree id="formpreview" flex="1"  contextmenu="picontext">
Nit: doubled space

>-<!ENTITY  mediaSaveAs           "Save As...">
>+<!ENTITY  mediaSaveAs           "Save As…">
Sorry, I don't like this change.

>+mediaSize=%Spx × %Spx
\u00d7 please.
Comment on attachment 164143 [details] [diff] [review]
195492-7.diff

>+  if (cacheEntryDescriptor)
>+  {
>+    pageSize = cacheEntryDescriptor.dataSize;
>+    kbSize = pageSize / 1024;
>+    sizeText = theBundle.getFormattedString("generalSize", [formatNumber(Math.round(kbSize*100)/100), formatNumber(pageSize)]);
>+
>+    expirationText = formatDate(cacheEntryDescriptor.expirationTime*1000, gStrings.notSet);
>+  }
>+
>+  setItemValue("sourcetext", sourceText, theBundle.getString("generalNotCached"));
>+  setItemValue("expirestext", expirationText, theBundle.getString("generalNoExpiration"));
>+  setItemValue("sizetext", sizeText, gStrings.unknown);
When is the expiration going to use no expiration and when is it going to use
not set?

>+  var i = 0;
>+  var n = 50;
>+  var s = new Date();
>+  while (i < n && iterator.nextNode())
>+    ++i;
I think a for loop would look neater here, e.g.
for (var i = 0; i < 50; ++i)
  if (!iterator.nextNode())
    return;

>+  //addlog("segment time: " + (new Date() - s) + " (" + (i+1) + " nodes)");
//prefer Date.now() if you are just subtracting timestamps

>+function addlog(entry)
This debugging stuff will go, right?

>+    switch (elem.type.toLowerCase())
IIRC type is guaranteed to be in lower case.

>     if (elem.hasAttributeNS(XLinkNS, "href"))
>-    {
>-      linktext = getValueText(elem);
>-      linkView.addRow([linktext, getAbsoluteURL(elem.href, elem), gStrings.linkX, ""]);
>-    }
>+      linkView.addRow([getValueText(elem), elem.href, gStrings.linkX, ""]);
I did't think we supported the href property on xlink elements. Does anyone
know of any example documents?

>+  setItemValue("imageexpirestext", expirationText);
>+  setItemValue("imagesizetext", sizeText);
I see this is slightly different to the page properties...

>+    document.getElementById("theimagecontainer").collapsed = undefined
>+    document.getElementById("brokenimagecontainer").collapsed = "true";
Actually hidden and collapsed are boolean properties, so you want true and
false (you probably noticed that "false" didn't work, that's because it
converts to true!)

>+  var imageSize = document.getElementById("imageSize");
>+  if (url)
>+  {
>+    //    alert([width, height]);
>+    imageSize.value = theBundle.getFormattedString("mediaSize", [formatNumber(width), formatNumber(height)]);
>+    imageSize.removeAttribute("disabled");
>+  }
>+  else
>+  {
>+    imageSize.value = gStrings.notSet;
>+    imageSize.setAttribute("disabled", "true");
>+  }
OK, it looks as if you couldn't use setItemValue.

>+        tmp = elem.getAttribute("copybuffer");
>+        if (tmp)
>+          text.push(tmp);
Use hasAttribute perhaps? Or at least declare tmp here.

As for the copy stuff, I think that Alt+A and Ctrl+C might work fine in text
fields after all, but it would still be better to get Alt+A for the trees too.
In SeaMonkey, platformCommunicatorOverlay.xul has a suitable definition for
key_selectAll.
(In reply to comment #36)
> >+  while (i < n && iterator.nextNode())
> >+    ++i;
> > 
> >+  if (iterator.nextNode())
> Is it safe to call nextNode after it has returned null?

It should just continue to return null, but I'll test this explicitly to make sure.

> >+function doSelectAll(event)
> > {
> >+  var elem;
> >+  if (event instanceof Components.interfaces.nsIDOMMouseEvent)
> >+    elem = document.popupNode.parentNode;
> >+  else
> >+    elem = event.target;
> Surely Copy and Select All always work on the focused element?
> 

Yes, but I'm not sure how that changes things. Is there a way to just get the
currently focused element?

> 
> >+    <key id="copykey" key="&copy.key;" modifiers="accel" command="cmd_copy"/>
> >+    <key id="selectkey" key="&selectall.key;" modifiers="accel"
command="cmd_selectall"/>
> Now this is going to give you a bit of a headache. You see, select all is Alt+A
> on linux. Furthermore, you'll block Ctrl+A and Ctrl+C in all text boxes. You
> could of course implement a command controller, see mail3PaneWindowCommands.js
> for how to implement a select all controller on a tree.

The textfields work just fine with this, and I have added a key for alt+A.

(In reply to comment #37)
> (From update of attachment 164143 [details] [diff] [review])
> >+  if (cacheEntryDescriptor)
> >+  {
> >+    pageSize = cacheEntryDescriptor.dataSize;
> >+    kbSize = pageSize / 1024;
> >+    sizeText = theBundle.getFormattedString("generalSize",
[formatNumber(Math.round(kbSize*100)/100), formatNumber(pageSize)]);
> >+
> >+    expirationText = formatDate(cacheEntryDescriptor.expirationTime*1000,
gStrings.notSet);
> >+  }
> >+
> >+  setItemValue("sourcetext", sourceText,
theBundle.getString("generalNotCached"));
> >+  setItemValue("expirestext", expirationText,
theBundle.getString("generalNoExpiration"));
> >+  setItemValue("sizetext", sizeText, gStrings.unknown);
> When is the expiration going to use no expiration and when is it going to use
> not set?
> 

Good call, I can omit the second arg to setItemValue here.

> >+  var i = 0;
> >+  var n = 50;
> >+  var s = new Date();
> >+  while (i < n && iterator.nextNode())
> >+    ++i;
> I think a for loop would look neater here, e.g.
> for (var i = 0; i < 50; ++i)
>   if (!iterator.nextNode())
>     return;

Perhaps. Or I could do | while (i++ < n && iterator.nextNode()); |
;)

> 
> >+  //addlog("segment time: " + (new Date() - s) + " (" + (i+1) + " nodes)");
> //prefer Date.now() if you are just subtracting timestamps
> 
> >+function addlog(entry)
> This debugging stuff will go, right?

yep.

> 
> >+    switch (elem.type.toLowerCase())
> IIRC type is guaranteed to be in lower case.
> 
> >     if (elem.hasAttributeNS(XLinkNS, "href"))
> >-    {
> >-      linktext = getValueText(elem);
> >-      linkView.addRow([linktext, getAbsoluteURL(elem.href, elem),
gStrings.linkX, ""]);
> >-    }
> >+      linkView.addRow([getValueText(elem), elem.href, gStrings.linkX, ""]);
> I did't think we supported the href property on xlink elements. Does anyone
> know of any example documents?

It worked when I wrote it, but I no longer have the test pages I had then. I'll
write another test for this.

> 
> >+  setItemValue("imageexpirestext", expirationText);
> >+  setItemValue("imagesizetext", sizeText);
> I see this is slightly different to the page properties...

right, this one is correct.

> 
> >+    document.getElementById("theimagecontainer").collapsed = undefined
> >+    document.getElementById("brokenimagecontainer").collapsed = "true";
> Actually hidden and collapsed are boolean properties, so you want true and
> false (you probably noticed that "false" didn't work, that's because it
> converts to true!)

Doh, I thought something was funny there. thanks :)

My next patch will include the firefox files as well. I'm also going to update
the help files to include information about keyboard shortcuts for copy and so on.
Attached patch 195492-8.diff (obsolete) — Splinter Review
addresses Neil's comments
Attachment #164143 - Attachment is obsolete: true
Attachment #164687 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #164143 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #164687 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch 195492-9.diff (obsolete) — Splinter Review
updated just a tad, uses setItemValue in onFormSelect(). I don't think there
are any other places to use it, but another pair of eyes would be good.
Attachment #164687 - Attachment is obsolete: true
Attachment #164767 - Flags: review?(neil.parkwaycc.co.uk)
this patch fixes bugs 195492, 201741, 203184, 244044, 261666, 252043, 135902,
181057, 182749, 239979, 158425, 188049 and 136532
Attachment #164767 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch 195492-10.diff (obsolete) — Splinter Review
another patch. fixes two other small bugs (not that I remeber their bug
numbers) that I promised Florian I'd fix. Sorry for the moving target Neil.

It may just be because I'm very tired, but looking at each bit of makePreview()
critically makes me wonder why it does the things it does. Was there really
even a readson to exclude some protocols? There's a bug out there about images
with imap: urls not showing in page info, though they do show in regular
webpages, etc. Are there any protocols we know we cannot show images from in
page info?

Also, with these changes, does an object with a type attribute of image/foo but
a data attribute that points to a java applet or flash or something show the
applet? If so, I think that change would have to go.

later.
Attachment #164767 - Attachment is obsolete: true
also fixes 249726, 252213, 253528, 256710, 235447 and 200025
No longer blocks: 181057, 195546
No longer depends on: 201264
Comment on attachment 164842 [details] [diff] [review]
195492-10.diff

>+  else if (item instanceof nsIImageElement || isBG)
>+    setItemValue("imagealttext", null);
>   else if (!isBG)
Well !isBG must be true at this point, no point repeating the test...

>+  if (item instanceof nsIObjectElement || item instanceof nsIEmbedElement || item instanceof nsILinkElement)
>+    mimeType = "type" in item && item.type;
If the item really is one of these classes then it must have a type property.

r=me with these nits fixed.

So, you didn't fancy window.setCursor("spinning"); then?
The suite, including page info, has help documentation so why are we planning on
getting rid of the help button without some other way of accessing it?
Why are you adding a second <script type="application/x-javascript"
src="chrome://global/content/XPCNativeWrapper.js"/> to the suite's pageInfo.xul?
Attached patch 195492-11.diff (obsolete) — Splinter Review
fix Neil's nits
Neil: no, I didn't really care for it. might still be the best way to do it
though
IanN: good call on both points. I've added access to the help via F1 (which I
had thought we already had, but I guess not), and removed the extra <script>
Attachment #164842 - Attachment is obsolete: true
Comment on attachment 165042 [details] [diff] [review]
195492-11.diff

request r= and sr=
Attachment #165042 - Flags: superreview?(bzbarsky)
Attachment #165042 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 165042 [details] [diff] [review]
195492-11.diff

I'm really not sure when I'll get to this (especially because I'm really not
doing srs in modules I'm not peer for at the moment....).  If you can get
someone else to do the sr, that would be much appreciated.
Attachment #165042 - Flags: superreview?(bzbarsky) → superreview?(jag)
Attached patch 195492-12.diff (obsolete) — Splinter Review
Attachment #165042 - Attachment is obsolete: true
Attachment #165042 - Flags: superreview?(jag)
Attachment #165042 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #165680 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 165680 [details] [diff] [review]
195492-12.diff

> <!ENTITY  linksTab              "Links">
>+<!ENTITY  linksAccesskey        "L">
Unfortunately the Policy button also has an access key of L :-/

>- * Portions created by the Initial Developer are Copyright (C) 2001
>+ * Portions created by the Initial Developer are Copyright (C) 2001-2004
This was the only change in classic pageInfo.css??

>+/* poke */
I don't think you meant to have this ;-)

>+window {
>+  background: transparent;
>+}
This doesn't look right (and it crashes my GTK1 for some reason :-( )
Comment on attachment 165680 [details] [diff] [review]
195492-12.diff

>     if (elem.hasAttributeNS(XLinkNS, "href"))
>-    {
>-      linktext = getValueText(elem);
>-      linkView.addRow([linktext, getAbsoluteURL(elem.href, elem), gStrings.linkX, ""]);
>-    }
>+      linkView.addRow([getValueText(elem), elem.href, gStrings.linkX, ""]);
This didn't work before, and it still doesn't work; the version in metaData.js
would work for absolute links but its version of getAbsoluteURL is busted (see
bug 175074). However since fairly recently the DOM3 baseURI property has been
added you can use a simplified version of getAbsoluteURL:
ioService.newURI(elem.getAttributeNS(XLinkNS, "href"), null, elem.baseURI).spec
Product: Browser → Seamonkey
Attached patch 195492-13.diff (obsolete) — Splinter Review
Attachment #165680 - Attachment is obsolete: true
Attachment #165680 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 167814 [details] [diff] [review]
195492-13.diff

request r= on new patch
Attachment #167814 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 167814 [details] [diff] [review]
195492-13.diff

Looks like "c" is the best access key for "Policy" - "l" was a bad choice
anyway, as the underline may only render as one pixel in some fonts.
Attachment #167814 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #167814 - Flags: review+
Attached patch 195492-14.diffSplinter Review
change accesskey
Attachment #167814 - Attachment is obsolete: true
Comment on attachment 167883 [details] [diff] [review]
195492-14.diff

request r= and sr=
Attachment #167883 - Flags: superreview?(rbs)
Attachment #167883 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #167883 - Flags: review?(neil.parkwaycc.co.uk) → review+
Comment on attachment 167883 [details] [diff] [review]
195492-14.diff

sr=rbs
Attachment #167883 - Flags: superreview?(rbs) → superreview+
just chekcked in, marking bug 19492 and it's dependants fixed
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 167883 [details] [diff] [review]
195492-14.diff

>Index: browser/base/content/pageInfo.xul
>===================================================================
>+  <stringbundle id="pageinfobundle" src="chrome://navigator/locale/pageInfo.properties"/>
>-  <stringbundle id="pageinfobundle" src="chrome://browser/locale/pageInfo.properties"/>
Bad idea, this broke Page Info on Firefox.

>       <!-- General page information -->
>-              <textbox readonly="true" crop="right" id="urltext"/>
>+              <textbox readonly="true" crop="right" id="urltext" class="text-link"/>
This reintroduced bug 238186: The Address of the page appears blue, and the
context menu items for the address are underlined in blue.
Blocks: 274440
Blocks: 238186
This appears to have completely broken page info for Firefox.
I would do something like that, wouldn't I?
Attached patch 195492-15.diffSplinter Review
Attachment #168642 - Flags: review?(neil.parkwaycc.co.uk)
*** Bug 274484 has been marked as a duplicate of this bug. ***
Blocks: 274478
Comment on attachment 168642 [details] [diff] [review]
195492-15.diff

sr=rbs
Attachment #168642 - Flags: superreview+
Comment on attachment 168642 [details] [diff] [review]
195492-15.diff

Actually can't you get rid of the crop="right" too?
Attachment #168642 - Flags: review?(neil.parkwaycc.co.uk) → review+
This patch added two non-existent CSS properties to the Firefox Page Info
stylesheets, "text-syle" (sic) in pinstripe and "text-style" in winstripe. I
assume those were supposed to be font-style?
Daniel:  I was about to mark bug 154692 WFM since the crash seems long gone, but
since you've been around the code recently, can you take a quick look to see if
there is anything we need to really "fix" there.  If there isn't please go ahead
and mark it WFM.  Thanks!
this caused bug 275096
*** Bug 160694 has been marked as a duplicate of this bug. ***
Comment on attachment 167883 [details] [diff] [review]
195492-14.diff

>+      linkView.addRow([getValueText(elem),
>+                       ioService.newURI(elem.getAttributeNS(XLinkNS, "href"), null, elem.baseURI).spec,
>+                       gStrings.linkX,
>+                       ""]);
So... bz pointed out two bugs:
1. no definition of ioService
2. 3rd arg to newURI is an nsIURI, not a string
(In reply to comment #70)
> (From update of attachment 167883 [details] [diff] [review] [edit])
> >+      linkView.addRow([getValueText(elem),
> >+                       ioService.newURI(elem.getAttributeNS(XLinkNS, "href"), null, elem.baseURI).spec,
> >+                       gStrings.linkX,
> >+                       ""]);
> So... bz pointed out two bugs:
> 1. no definition of ioService
> 2. 3rd arg to newURI is an nsIURI, not a string
> 
See bug 286949 for that. It seems that those bugs are both fixed. approval1.8.1 is pending though...
You need to log in before you can comment on or make changes to this bug.