195492 - Tidy up media tab in page info

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect)

Description

[piers]

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?

Comment 1

[piers]

Attachment: Screenshot of changes

Comment 2

[piers]

* Possibly also add "chrome" as an allowed protocol.

Comment 3

[Daniel Brooks [:db48x]]

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.

Comment 4

[Daniel Brooks [:db48x]]

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. :)

Comment 5

[Per Ångström]

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.

Comment 6

[Daniel Brooks [:db48x]]

you liked those numbers? hmm. I guess I can add them back. File a seperate bug
for it and assign it to me.

Comment 7

[piers]

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.

Comment 8

[piers]

Attachment: Screenshot showing "physical" and italitc "Not specified"

Comment 9

[Per Ångström]

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.

Comment 10

[piers]

Attachment: Page info meda tab

Comment 11

[Daniel Brooks [: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 :)

Comment 12

[Daniel Brooks [:db48x]]

whoops, meant to trim the length a bit before I submitted :)

Comment 13

[Daniel Brooks [:db48x]]

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.

Comment 14

[[e-mail]]

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...

Comment 15

[[e-mail]]

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

Comment 16

[[e-mail]]

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?)

Comment 17

[piers]

>-    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.

Comment 18

[[e-mail]]

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.

Comment 19

[Daniel Brooks [:db48x]]

Piers: ah, ok. I hadn't realized we were changing the ratio of the picture.

Comment 20

[piers]

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". :)

Comment 21

[[e-mail]]

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.

Comment 22

[piers]

Attachment: Page info meda tab

Ok, change the "unset" indicator to just disable the textbox per comment 21.

Comment 23

[Henrik Lynggaard Hansen]

Adding depency, which request adding a count to the media lines indicating the
number of times a image is used.

Comment 24

[James Carr]

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 25

[James Carr]

Attachment: Screenshot of Page Info -> Media Tab under certain resize

Comment 26

[[e-mail]]

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?

Comment 27

[James Carr]

i'm not sure but I definately think it needs a label

Comment 28

[Daniel Brooks [:db48x]]

Attachment: patch

clean up, account for some bit rot, touch up a few other things, same changes
to firefox.

Comment 29

[[e-mail]]

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.

Comment 30

[Daniel Brooks [:db48x]]

Attachment: patch

Comment 31

[Daniel Brooks [:db48x]]

Attachment: patch

same changes in firefox, a few other minor tweaks.

Comment 32

[Daniel Brooks [:db48x]]

Attachment: patch

this should do it, but with the same changes to firefox as well. I think I got
everything Neil mentioned in his last review.

Comment 33

[Daniel Brooks [:db48x]]

Comment on attachment 164112 [details] [diff] [review]
patch

request r=

Comment 34

[Daniel Brooks [:db48x]]

Attachment: 195492-7.diff

slight update. Not calling this one 'patch' for Neil's sake

Comment 35

[Daniel Brooks [:db48x]]

Comment on attachment 164112 [details] [diff] [review]
patch

request r= on newer patch

Comment 36

[[e-mail]]

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 37

[[e-mail]]

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.

Comment 38

[Daniel Brooks [:db48x]]

(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.

Comment 39

[Daniel Brooks [:db48x]]

Attachment: 195492-8.diff

addresses Neil's comments

Comment 40

[Daniel Brooks [:db48x]]

Attachment: 195492-9.diff

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.

Comment 41

[Daniel Brooks [:db48x]]

this patch fixes bugs 195492, 201741, 203184, 244044, 261666, 252043, 135902,
181057, 182749, 239979, 158425, 188049 and 136532

Comment 42

[Daniel Brooks [:db48x]]

Attachment: 195492-10.diff

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.

Comment 43

[Daniel Brooks [:db48x]]

also fixes 249726, 252213, 253528, 256710, 235447 and 200025

Comment 44

[[e-mail]]

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?

Comment 45

[Ian Neal]

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?

Comment 46

[Daniel Brooks [:db48x]]

Attachment: 195492-11.diff

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>

Comment 47

[Daniel Brooks [:db48x]]

Comment on attachment 165042 [details] [diff] [review]
195492-11.diff

request r= and sr=

Comment 48

[Boris Zbarsky [:bzbarsky]]

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.

Comment 49

[Daniel Brooks [:db48x]]

Attachment: 195492-12.diff

Comment 50

[[e-mail]]

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 51

[[e-mail]]

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

Comment 52

[Daniel Brooks [:db48x]]

Attachment: 195492-13.diff

Comment 53

[Daniel Brooks [:db48x]]

Comment on attachment 167814 [details] [diff] [review]
195492-13.diff

request r= on new patch

Comment 54

[[e-mail]]

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.

Comment 55

[Daniel Brooks [:db48x]]

Attachment: 195492-14.diff

change accesskey

Comment 56

[Daniel Brooks [:db48x]]

Comment on attachment 167883 [details] [diff] [review]
195492-14.diff

request r= and sr=

Comment 57

[rbs]

Comment on attachment 167883 [details] [diff] [review]
195492-14.diff

sr=rbs

Comment 58

[Daniel Brooks [:db48x]]

just chekcked in, marking bug 19492 and it's dependants fixed

Comment 59

[Steffen Wilberg]

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.

Comment 60

[Bill Gianopoulos [:WG9s]]

This appears to have completely broken page info for Firefox.

Comment 61

[Daniel Brooks [:db48x]]

I would do something like that, wouldn't I?

Comment 62

[Daniel Brooks [:db48x]]

Attachment: 195492-15.diff

Comment 63

[Bill Mason]

*** Bug 274484 has been marked as a duplicate of this bug. ***

Comment 64

[rbs]

Comment on attachment 168642 [details] [diff] [review]
195492-15.diff

sr=rbs

Comment 65

[[e-mail]]

Comment on attachment 168642 [details] [diff] [review]
195492-15.diff

Actually can't you get rid of the crop="right" too?

Comment 66

[PikeUK]

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?

Comment 67

[Jay Patel [:jay]]

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!

Comment 68

[Nickolay_Ponomarev]

this caused bug 275096

Comment 69

[Peter Weilbacher]

*** Bug 160694 has been marked as a duplicate of this bug. ***

Comment 70

[[e-mail]]

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

Comment 71

[Florian Quèze [:florian]]

(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...