517902 - Reimplement image properties, using the existing "Media" panel

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement)

Description

[Frédéric Buclin]

Per bug 515368 comment 13:

Right clicking on an image should still display "Properties", and this item should point to the existing "Media" tab in Page Info, with the image item preselected, so that you immediately get the metadata you want. This way, you don't need to duplicate the code/UI; all you need is Properties pointing to the right place. I doubt it would take 48 Kb of code to implement, and it would make everybody happy.

Comment 1

[Tanner M. Young [:tmyoung]]

I have a patch in the works, so I will assign it to me.

Comment 2

[Tanner M. Young [:tmyoung]]

Attachment: Context menu option for image properties

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Won't block on this, cc'ing Johnath and Alex for some UI opinion. My feeling is that it's reasonable to have this sort of linkage for items for which we show specific properties within the Page Info window, and potentially helpful.

Comment 4

[O. Atsushi (Torisugari)]

(In reply to comment #3)
> My feeling is that it's reasonable to have this sort of linkage
> for items for which we show specific properties within the Page
> Info window, and potentially helpful.

By "items", you mean not only images, but also links, blockquote/q/ins/del etc. ? Now, bug 1995 is totally regressed on Firefox. 

If we are to fix bug 1995 along with that strategy, we need another 2 panels.

1: Links tab
2: Cites/Dates tab 

|cite| attribute is almost identical to |href|, as long as we neglect XHTML2.0-like fundamentalism. So that we might need just one tab as a UI. Anyway, is it possible to implement Links panel again?

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

It would be nice if we exposed the color profile of the image (if any) in the properties, since we now render based off of those.  Not sure if that is in scope for reimplementing image properties.

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> It would be nice if we exposed the color profile of the image (if any) in the
> properties, since we now render based off of those.  Not sure if that is in
> scope for reimplementing image properties.

This should be done in a separate bug. The goal of this bug is to reimplement the item in the context menu of images.

Comment 7

[Tanner M. Young [:tmyoung]]

Attachment: Screenshot of context menu item

Here is a screenshot of the new context menu item.

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 402433 [details] [diff] [review]
Context menu option for image properties

ui-r+ with the nit that this should appear as the very last item in the context menu, after block image.

Comment 9

[WulfTheSaxon [:Wulf]]

(In reply to comment #5)

I think you're looking for bug 232806 and bug 106068 (EXIF and IPTC, respectively).

Comment 10

[Daniel Brooks [:db48x]]

Comment on attachment 402433 [details] [diff] [review]
Context menu option for image properties

> // mmm, yummy. global variables.
> var gWindow = null;
> var gDocument = null;
>+var imgUrl = null;

Well, I don't care for global variables, but lets at least follow the naming conventions and name it gImageURL.

>@@ -521,37 +530,30 @@ function processFrames()
>     var doc = gFrameList[0];
>     onProcessFrame.forEach(function(func) { func(doc); });
>     var iterator = doc.createTreeWalker(doc, NodeFilter.SHOW_ELEMENT, grabAll, true);
>     gFrameList.shift();
>     setTimeout(doGrab, 16, iterator);
>   }
>   else
>     onFinished.forEach(function(func) { func(); });
>+    onFinished.push(selectImgUrl);

This line is indented as if you intend for it to be a part of the else clause, but without braces it is actually happening unconditionally.

>+function selectImgUrl ()
>+{
>+  if (imgUrl) {
>+    var tree = document.getElementById("imagetree");
>+    for (c = 0; c <= tree.view.rowCount; c++)
>+    {
>+      if (imgUrl == gImageView.data[c][COL_IMAGE_ADDRESS]) {
>+        tree.view.selection.select(c);
>+        return;
>+      }
>+    }
>+  }
>+}

As written 'c' is a global variable. Lexically scope it with a 'var':

for (var c = 0; c < tree.view.rowCount; c++)

Also, note that using <= in the test will produce an off-by-one error resulting in a js exception when you read past the end of the array.

Comment 11

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.1

Here is the new patch.  I have fixed the Mochitests, moved the item to the bottom of the image context menu--from ui-review, and fixed the three items from the review.

Comment 12

[Daniel Brooks [:db48x]]

Comment on attachment 405688 [details] [diff] [review]
Patch v 1.1

> // doc - document to use for source, or null for this window's document
> // initialTab - name of the initial tab to display, or null for the first tab
>-function BrowserPageInfo(doc, initialTab)
>-{
>-  var args = {doc: doc, initialTab: initialTab};
>+function BrowserPageInfo(doc, initialTab, gImageUrl)
>+{
>+  var args = {doc: doc, initialTab: initialTab, gImageUrl: gImageUrl};

This gImageUrl is a local variable, so don't use the 'g' prefix. Don't use the 'g' prefix on the property name either. Also, add a comment describing the new argument while you're here.

r=db48x with those minor changes.

Comment 13

[Tanner M. Young [:tmyoung]]

Attachment: Patch v 1.2

I've made those review changes.

Comment 14

[Fabian Norman]

I want to add my 2cents as a user that has been made aware of this "removed properties menu" bug.

It appears that this will only add the right click properties menu back when clicking on images and links. Do be aware that the proprieties menu can be also used to identify the language of a site, and that you get from not clicking on a link or image, but just on any random element on the web-page.
This is vital information for many.
Example:
http://imgur.com/wInyw.png

Comment 15

[Leman Bennett [Omega]]

(In reply to comment #14)
> I want to add my 2cents as a user that has been made aware of this "removed
> properties menu" bug.
> 
> It appears that this will only add the right click properties menu back when
> clicking on images and links. Do be aware that the proprieties menu can be also
> used to identify the language of a site, and that you get from not clicking on
> a link or image, but just on any random element on the web-page.
> This is vital information for many.
> Example:
> http://imgur.com/wInyw.png


This is only for images, not links. You should probably open up a new bug for other element properties.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get a different reviewer than Vlad? I wager he's a little too busy at the moment ...

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I won't have time to get to this for a while, and I doubt mconnor will either -- but from a quick glance, this is r-; adding a context menu item is a no-go.  The main browser context menu is a cesspit, and adding more stuff to it is not going to work.

Comment 18

[WulfTheSaxon [:Wulf]]

(In reply to comment #17)
Except this isn't adding a new item, just replacing an old one (and in fewer circumstances).

(On a side note, and IMHO, more needs to be done to discourage extension developers from adding needless context menu items in the AMO review process.)

Comment 19

[Frédéric Buclin]

(In reply to comment #18)
> Except this isn't adding a new item, just replacing an old one (and in fewer
> circumstances).

Yeah, exactly. Comparing Fx 3.5 and this patch, there is no context menu change when viewing images. And beltzner agrees in comment 3 that this may be helpful.

Comment 20

[Tanner M. Young [:tmyoung]]

For my part--and I am obviously biased--now that I have this feature, I use it all the time.  In fact, I use it far more often than "Page Info" which brings up the same window.  The time savings for users, particularly web developers, is substantial enough that it's definitely worth it.

Also, from Alex's comments, I have moved it to the bottom of the image menu, so it shouldn't get in the way, and as Frédéric notes, it only applies to images so the majority of users wont see it in their day-to-day routine anyway.

And, mconnor said he would get to it today.

Comment 21

[Shawn Wilsher :sdwilsh]

This, of course, should have a test...

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Yeah, definitely needs a test; Axel, would you be willing to allow this string into mozilla-1.9.2?

Comment 23

[Axel Hecht [:Pike]]

I am unwilling, in general. I'd be easier to push over if it'd be my own "get me back my properties" pet thing (that'd be links, "opens in new tab or not"). I'm sensitive to the outcry there, though, and I'd not be blocking, but it's not my personal experience that this would be blocking.

Basically, I'm coming in here with stop energy, nothing more, nothing less. It's Thursday, it's not on trunk yet, etc... .

I defer to beltzner to push me over my inner hill, or this bug off to 3.7. I'd personally say "nah, not really".

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #21)
> This, of course, should have a test...
And to clarify, the only test this has is to make sure that the right thing is shown in the menu.  What it's missing is testing the that actually opens the window and contains the right information for the image that we right clicked on.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

No test, no review, not on trunk yet, doesn't look like it'll make it for Firefox 3.6. :(

Comment 26

[Mike Connor [:mconnor]]

Comment on attachment 406368 [details] [diff] [review]
Patch v 1.2


>+// imageUrl - url of an image to load in the Media Tab of the Page Info window

this can be null/omitted, so please include that in the comment

sr=me, discussed this with beltzner a little, we both think it's worthy, whether it makes 3.6 or not is hard to say, but we should get a quick test added and landed on trunk.

Comment 27

[Tanner M. Young [:tmyoung]]

(In reply to comment #24)
> And to clarify, the only test this has is to make sure that the right thing is
> shown in the menu.  What it's missing is testing the that actually opens the
> window and contains the right information for the image that we right clicked
> on.

Where do we want the test, in the test_contextmenu.html file?  And, how do I simulate a click on the context menu item in this case?  (With every day, my memory gets worse and worse...)

Comment 28

[Reed Loden [:reed]]

Spoke with mconnor about landing this without a test for now, and he was ok with it as long as a test is quickly added soon (preferably later tonight). tmyoung is actively working on getting a test written, so he will hopefully have it soon.

http://hg.mozilla.org/mozilla-central/rev/9617b5335022

Comment 29

[Frédéric Buclin]

Comment on attachment 406368 [details] [diff] [review]
Patch v 1.2

We should take it for 3.6. Having a whole cycle without it is painful and there is no release *without* this feature yet, AFAIK, so this would be transparent to the average user.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 406368 [details] [diff] [review]
Patch v 1.2

OK, let's get this on to 1.9.2. I think I agree that it's a significant loss for a subset of users, and a limitation of the media panel that there's no easy way to get info for a specific image.

Tanner: please get that test done ASAP.

Comment 31

[Tanner M. Young [:tmyoung]]

It needs check-in for 1.9.2.

Comment 32

[Tanner M. Young [:tmyoung]]

Attachment: First attempt at a testcase

This is the general test I have been trying to make.

My test should load the image > click the context menu item > open the page info dialog > check if the preview image exists and has the correct source.

I'm sure I've made a stupid mistake somewhere (or many stupid mistakes), so any help is welcome and desperately needed.

Comment 33

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a7e017d058ba

Comment 34

[Nochum Sossonko [:Natch]]

Some general comments:

We generally place all the testing code in a function (runTest) and run it onload, so in the window element you want |onload="runTest"|.

You need to QI the pageInfo window to nsIDOMWindow off the enumerator.

There's a simpler way of doing this context menu test, see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug424101.js .

Where were you planning on dropping in this test? Browser chrome tests are js files, which this one should be.

Comment 35

[Marat Tanalin]

Maybe of some importance: in Page Info dialog, media item is indeed preselected (as expected), but media files list is *not* automatically *scrolled* to make this item *visible*. For example, try see properties of "QMO" image on http://www.mozilla.org/projects/minefield/ page.

Comment 36

[Tanner M. Young [:tmyoung]]

(In reply to comment #35)
> Maybe of some importance: in Page Info dialog, media item is indeed preselected
> (as expected), but media files list is *not* automatically *scrolled* to make
> this item *visible*. For example, try see properties of "QMO" image on
> http://www.mozilla.org/projects/minefield/ page.

Bug 524090 was just filed for that.

Comment 37

[Tanner M. Young [:tmyoung]]

Attachment: Updated testcase

(In reply to comment #34)
> We generally place all the testing code in a function (runTest) and run it
> onload, so in the window element you want |onload="runTest"|.
Done.

> You need to QI the pageInfo window to nsIDOMWindow off the enumerator.
I tried adding that, but it returned "[Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: chrome://mochikit/content/browser/browser/base/content/test/browser_bug517902.js :: anonymous :: line 32"  data: no]"]"

> There's a simpler way of doing this context menu test, see
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug424101.js
Done.

> Where were you planning on dropping in this test? Browser chrome tests are js
> files, which this one should be.
Now, I will do it as a js. file, where it belongs...

The only thing that I still lack on this test is the ability to check if the preview image src is correct.  I keep getting null values.

Comment 38

[Tanner M. Young [:tmyoung]]

> The only thing that I still lack on this test is the ability to check if the
> preview image src is correct.  I keep getting null values.

The reason for this is that the src is not populated until some time after the pageInfo window loads.  Setting a setTimeout and other methods like that don't work nor does using waitForEvents(event) like in http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/test_contextmenu.html?force=1#369 since the events never seem to fire.

The only time it has worked is when a js alert was fired and closed manually by me, and then the test finished, but that's useless for automated test.

Advise please.  With this fixed, the test is complete...

Comment 39

[Philip Chee]

Could you try hooking into the onLoadRegistry which is called at the end of loadPageInfo().

Comment 40

[Tanner M. Young [:tmyoung]]

Attachment: Testcase v 1.2

This passes, but it can sometimes skip the last steps that check for the image (they are in the scope of the observer function) without returning any error.  There aren't any js console errors, and it functions on the same logic as the browser_pageInfo.js test, so I'm not sure why these events are skipped in one and not the other.

Comment 41

[Nochum Sossonko [:Natch]]

1) I'd suggest placing the observer out of the test function.

2) Add the observer _before_ opening the page info dialog.

3) The test belongs in browser/base/content/test/

Comment 42

[Tanner M. Young [:tmyoung]]

Attachment: Testcase v 1.3

Here is the testcase ready to go.  It uses a new observer notifier in pageInfo.js for when an image is selected.  This wont clutter the "observer airways"; it enables the testcase to be more elegant; and add-ons or other features that relate to this new feature will be able to access this information, since adding a hook to onFinished wont  guarantee that the correct image is selected because selectImgUrl could still be running when a test's, add-on's, or feature's hook is started.

Comment 43

[Tanner M. Young [:tmyoung]]

Dao's patch for Bug 524106 fixes several idiosyncrasies with this patch.  However, his patch uses the element instead of the src to load the image information, which I agree is a better way to do it, but it will break this test.  Do we want this test and redo his patch or commit his patch and do a test for everything afterwards?

Comment 44

[Nochum Sossonko [:Natch]]

Fwiw, I don't think it's a good idea to add an observer for every image selection. But that's just me...

Alternatively, you can just add your own "select" event listener in the test (if I'm reading it correctly)...

Comment 45

[Tanner M. Young [:tmyoung]]

The observer should only fire when someone calls View Image Info with an imageUrl (imageElement in Bug 524106), and if it is not present no observer fires.  Also if you change the image after it has selected the image the first time, the observer will not continue fire.

The reason I added this is that even if I hooked into onFinished, it wouldn't know if the image has been selected or not.  This not only applies to this test but to any addons or other features that might depend on this functionality.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 408240 [details] [diff] [review]
Testcase v 1.3

>diff --git a/browser/base/content/test/browser_bug517902.js b/browser/base/content/test/browser_bug517902.js

>+netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

this isn't necessary for browser chrome tests, the code only runs in a chrome context.

Comment 47

[Tanner M. Young [:tmyoung]]

Comment on attachment 408240 [details] [diff] [review]
Testcase v 1.3

Since Bug 524106 was landed, I need to update the testcase.

Comment 48

[Tanner M. Young [:tmyoung]]

Attachment: Testcase v 1.4

Updated testcase after Bug 524106 was landed, and I removed the line Gavin noted was unnecessary.

Comment 49

[Dão Gottwald [:dao]]

Attachment: alternative test

I don't think we should add that observer topic, so here's a test without that. This covers bug 524106, too. What do you think, Tanner?

Comment 50

[Tanner M. Young [:tmyoung]]

> I don't think we should add that observer topic, so here's a test without that.
> This covers bug 524106, too. What do you think, Tanner?

Perfect! It looks good to me.  I'm happy to use it instead.  The only thing is that we must land Bug 524106 on 1.9.2 at the same time, which I think we should anyway...

Comment 51

[Dão Gottwald [:dao]]

Anything you'd like to add to the test? Your version also tested the presence of the context menu item, but I think that's covered elsewhere.

Comment 52

[Tanner M. Young [:tmyoung]]

No, there is nothing I need.  The context menu test is part of the test submitted with the initial patch, so we don't need to add it again in the chrome test.

Comment 53

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/487808781b1f

Comment 54

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 409534 [details] [diff] [review]
alternative test

a192=beltzner, though tests do not need review, really

Comment 55

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4c30145dd7f3

Comment 56

[Tanner M. Young [:tmyoung]]

Verified Fixed.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091104 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091104045446

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks good on 1.9.2 too with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b2pre) Gecko/20091106 Namoroka/3.6b2pre ID:20091106033745