82307 - Image Properties window should have image dimensions info

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement, P3)

Description

[Agustín Fernández]

It would be great if the image dimensions and (even more important) their size
(content-size) were on the properties dialog. I know that you can get their
dimensions from page info, but for that you must look for an image in a
(sometimes very) big list. It would be a lot easier to just right-click at them.
The content-size is another story. There is no way to get that information
without actually saving it and checking with another program. Web developers
(well, at least I) look for this information all the time.

Comment 1

[Daniel Brooks [:db48x]]

I'd say gerv might like this. See bug 74121. As a related note, content size
will be shown in the new page info, in addition to will width and height.

Comment 2

[Håkan Waara]

-> XP Apps: GUI Features

Comment 3

[Håkan Waara]

reassign too. :)  sorry for the spam.

Comment 4

[Blake Ross]

--> Gerv

Comment 5

[Gervase Markham [:gerv]]

Yep, sounds good to me. I can't promise when I'll get to it, though.

Gerv

Comment 6

[Gervase Markham [:gerv]]

I've done a patch for this - coming up. I've looked at the other Properties
window bugs to see if any of them are worth fixing while I'm there but IMHO none
of them are :-)

Gerv

Comment 7

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Comment 8

[Gervase Markham [:gerv]]

OK, try this on for size. Sicking, could you review?

I've also fixed the problem whereby the link underlining would continue to the
end of the row if there was something else in the column longer than the link. I
did this by putting each link in an hbox and using a spring.

Oops, hang on, I haven't done the content-size. Dunno if I can actually get that
info. Anyway, this patch is enough to be going on with.

Gerv

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This won't show the physical width and height of the image if the image is 
scaled I think. Though this is better then nothing I would really like to get 
the physical width/height rather then the displayed. (this was actually the 
reason I didn't add width/height originally)

Comment 10

[Gervase Markham [:gerv]]

I doubt there's any easy way of getting the physical size of the image. First
we'd have to be able to convert the handle we have on the <image> tag to a
handle to something representing the image itself, which we can ask the image
library about. That is definitely a separate, more complex RFE.

Gerv

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

yeah... but that is probobly where you'll find the physical size though. I 
think Daniel Brooks does something similar in bug 52730, would you mind taking 
a look at that?

Also, could you add the " pixels" string in the xul file instead by doing <text 
value=" &pixels;">. Otherwise you'll get "Width:  pixels" if the img dosn't 
have a .width property (for instance a <input type="image">)

Comment 12

[Gervase Markham [:gerv]]

I have the latest version of db48x's patch for that bug and, as far as I can
see, there's nothing in the JS about getting widths and heights.

I think logical width and height are a good start.

Adding &pixels.label; in the XUL file will mean it gets overwritten when I set
the label with the width/height value, won't it? Is it really possible to have
an image which is visible on screen (so it can be right-clicked) which doesn't
have a width and a height? If so, I can just do:
if (width in img)
   // set width

Gerv

Gerv

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Gerv: Remember that the 'img' object isn't necessarily an <img>. I'm not sure 
is a <input type="image"> has a .width property?

The " pixels" text won't be overwritten if you add it in a separate <text>. IE 
if you do:

 <row id="image-width">
   <text value="&image-width.label; "/>
   <hbox>
     <text id="image-width-text" value=""/>
     <text value=" &pixels.lable;"/>
   </hbox>
 </row>

Daniel: You say that your new page-info window will contain content-size, 
how/where do you get that?

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Comment 15

[Gervase Markham [:gerv]]

Unless someone reviews this quick, it's not going to make it in...

Gerv

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For resons not told here my ISP has cut me off so I can't really apply and test 
this patch. However it's a fairly simple patch, and assuming you have tested it 
and it works (with <input type="image"> too):

r=sicking

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

cc'ing db48x for input on the content-size thing. Please see my comment 2001-07-
18 07:42

Comment 18

[Gervase Markham [:gerv]]

Attachment: Remove parentheses; fix a couple of JS warnings

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

please move the |var i| declaration out of the first for-loop to clarify that 
the variable is used further down. Other then that, r=sicking still applies

Comment 20

[Blake Ross]

+        if ("width" in img) {
+            setInfo("image-width", img.width);
+            setInfo("image-height", img.height);
+        }


Why is that check sufficient to assume that there's a height?

Almost nothing should use onclick.  Those <text/> don't seem to be an exception.
 Does oncommand work?

Comment 21

[Gervase Markham [:gerv]]

> Why is that check sufficient to assume that there's a height?

It's not. But if you can think of any reason that a clickable-on image could
have a width but not a height, I'd be very impressed.

> Almost nothing should use onclick.  Those <text/> don't seem to be an 
> exception. Does oncommand work?

A quick search-and-replace yields the answer: No. :-| Can we avoid letting this
patch spread from the issue? I don't want to open other cans of worms...

Gerv

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

DOH! I missed that |if|. You don't need it at all, just do:

+        setInfo("image-width", img.width);
+        setInfo("image-height", img.height);

If width and/or height are undefined they will be hidden (the setInfo function 
does that) so in fact you *shouldn't* have that if

Comment 23

[Gervase Markham [:gerv]]

No, you need the if to avoid JS strict errors.

Gerv

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

hmm.. didn't know that. Thought that was legal. Ok, in that case you need to add

+        else {
+            setInfo("image-width", "");
+            setInfo("image-height", "");
+        }

for the case when .width dosn't exist

Comment 25

[Gervase Markham [:gerv]]

Done. Let's put this to bed. sicking - review, please? :-)

Gerv

Comment 26

[Gervase Markham [:gerv]]

Attachment: Patch v.4

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That |else { ... }| was all you changed, right?

If so, r=sicking

Comment 28

[Blake Ross]

sr=blake

Comment 29

[Gervase Markham [:gerv]]

Checked in.

Gerv

Comment 30

[sairuh (rarely reading bugmail)]

vrfy fixed. tested at http://wired.com/news/nc_index.html, and the Image
Properties in Page Info had columns and listed data for width, height and alt text.

linux, 2001.08.15.14-comm
winnt, 2001.08.15.06-comm
mac OS 9.1 [emul on X], 2001.08.15.08-comm