333131 - [1.0.8] Image Context menu opens as Normal Context Menu

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Tracy Walker [:tracy]]

Seen with FFx 1.0.8 respin 2006-04-07

steps to reporduce:
-Go to http://www.google.com
-Right click the Google logo

tested results:
Normal page context menu appears

expected results:
Image context menu should appear

regression from fix for bug 333035?  The image context menu was working on the 04/01 1.0.8 respin that had bug 333035.

Comment 1

[Tim Riley [:timr]]

Are we still pulling by date and not updated the date for the fix for 333035?

Comment 2

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

crap: aviary 101's nsIImageLoadingContent has no currentURI. Bug 333035 fixed the branch porting of the patch from bug 301329, but the patch from bug 301329 doesn't actually work for the 1.7/1.0.x branches.

Is there anything else I could use here to detect whether an nsIImageLoadingContent is an image? Maybe loadingEnabled? Or checking for a current or pending request?

Comment 3

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

Attachment: patch?

Looking at nsImageLoadingContent::GetCurrentURI, I guess I can just check that a current request exists? This seems to work.

Comment 4

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

Comment on attachment 217564 [details] [diff] [review]
patch?

Hmm, I'm not sure this will work (I didn't notice that it fell back to mCurrentURI).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Checking the request will treat blocked images as not images (since they have no request)...  It's better than nothing if we need something fast and can't get currentURI working, though.

biesi, could we pull the currentURI changes out of the patch for bug 196380 and land them on branch in a safe way?  We'd have to be careful about clearing it in the relevant places too, I guess....  :(

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, urg. I guess it's not very risky, so we could do that... but I wonder if it would be less risky to have some sort of isImage attribute that would be implemented by the subclasses?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

(because then you don't have to worry about clearing it)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, that would probably be easier... Do you have time to do that?  Or should I try to do it?  I might be able to on Sunday or so.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 217564 [details] [diff] [review]
patch?

>+             if ( this.target instanceof nsIILC &&
>+                  (request = this.target.getRequest(nsIILC.CURRENT_REQUEST)) ) {

What if this were
     if ( this.target instanceof nsIILC &&
          (request = this.target.getRequest(nsIILC.CURRENT_REQUEST) ||
          this.target.imageBlocked) ) {

>-                var request = this.target.getRequest( Components.interfaces.nsIImageLoadingContent.CURRENT_REQUEST );
>-                if (request && (request.imageStatus & request.STATUS_SIZE_AVAILABLE))
>+                if (request.imageStatus & request.STATUS_SIZE_AVAILABLE)

Then you'd still need to check if(request && (request.imageStatus etc...))

Comment 10

[Boris Zbarsky [:bzbarsky]]

The idea from comment 9 should work (with the imageBlockingStatus prop and checking whether it's a REJECT_*, of course).

Comment 11

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

Attachment: testcase

Comment 12

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

Attachment: Firefox patch (dveditz's suggestion)

This passes all the tests in the attached testcase. Not sure who to ask for review, if anyone wants to stamp it go ahead.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 217604 [details] [diff] [review]
Firefox patch (dveditz's suggestion)

r=dveditz

Comment 14

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

Attachment: xpfe (suite and thunderbird)

Thunderbird is actually using the xpfe version of this file. I found out about this because http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mail/base/content/nsContextMenu.js&rev=1.4.24.4&mark=248#244 is broken (missing paren), but TB's context menu isn't. I checked the latest TB nightly from the AVIARY_1_0_1 branch, and it's using the XPFE nsContextMenu.js.

Comment 15

[Tim Riley [:timr]]

Comment on attachment 217604 [details] [diff] [review]
Firefox patch (dveditz's suggestion)

Critical regression to fix. a=timr for drivers

Comment 16

[Tim Riley [:timr]]

This is a critical regression for 1.0.8 and 1.7.13.  a=timr for drivers

Comment 17

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

Checked in attachment 217604 [details] [diff] [review] (Firefox) and attachment 217607 [details] [diff] [review] (Thunderbird) on AVIARY_1_0_1_20050124_BRANCH:
mozilla/browser/base/content/browser.js 	1.296.2.3.2.134.2.18
mozilla/xpfe/communicator/resources/content/nsContextMenu.js 	1.88.16.5

Checked in attachment 217607 [details] [diff] [review] (Suite) on MOZILLA_1_7_BRANCH:
mozilla/xpfe/communicator/resources/content/nsContextMenu.js 	1.88.2.7

Comment 18

[Tracy Walker [:tracy]]

test with Windows Firefox 1.0.8 build from: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/1.0.8-candidates/

gavin is there supposed to be a image context menu for each image in the testcase that you supplied?

1.0.8 respin from 0408 only shows an image context menu on a few of the images. -The first four images display a normal page context menu.
-the Fifth image (broken) displays an image context menu as does the next image.
-The next to last image does not display an image context menu. and the last image does.

Oh and the Google logo at www.google.com still displays a normal page context menu.

reopening

but is it possible that the respins, for some reason, did not pick up the fix?

Comment 19

[Tim Riley [:timr]]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060408 Firefox/1.0.8

Tracy and I still get this problem from the latest build.  I got the build from here:

   http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/1.0.8-candidates/

Comment 20

[Tim Riley [:timr]]

Test case comparison (attachment 217594 [details]):

Test    1.0.8        1.5.0.1
Item    20060408
-----   ---------    ----------
1       page menu*   image menu
2       page menu*   image menu
3       page menu*   image menu
4       page menu*   image menu
5       image menu   image menu
6       image menu   image menu
7       page menu*   image menu
8       image menu   image menu
9       text menu    text menu

* Incorrect

Comment 21

[Jay Patel [:jay]]

I can confirm the same results as Tim on Windows: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060408 Firefox/1.0.8  Looks like we made some progress, but still some bustage.

If Gavin did a thorough test run of his testcases with his build before checking in, I wonder if this was a build issue (perhaps we respun with source pulled from an old tag?).

Comment 22

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

This looks like a build issue. I just downloaded the windows build from http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/1.0.8-candidates/ , and by looking at it's browser.js, I do not see the patch applied.

Comment 23

[J. Paul Reed [:preed]]

(In reply to comment #22)
> This looks like a build issue. I just downloaded the windows build from
> http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/1.0.8-candidates/ ,
> and by looking at it's browser.js, I do not see the patch applied.

I see the problem.

The tag was not correctly updated because the checkins to the files were not made "atomically" (which isn't possible in CVS, but bear with me here for a sec):

If you look at the bonsai query I got for the first round of checkins on Friday, they're grouped together as "one" checkin (the date shows they're actually a bunch of separate checkins):

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=&branchtype=match&dir=&file=&filetype=match&who=gavin%25gavinsharp.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-04-06+15%3A51&maxdate=2006-04-06+15%3A53&cvsroot=%2Fcvsroot

The query for yesterday's checkins:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=&branchtype=match&dir=&file=&filetype=match&who=gavin%25gavinsharp.com&whotype=match&sortby=Date&hours=48&date=explicit&mindate=2006-04-07+14%3A33&maxdate=2006-04-07+14%3A41&cvsroot=%2Fcvsroot

shows that the changes to browser.js and nsContextMenu.js were made in separate checkins, causing Bonsai to split them up in the output... so I didn't include the update to browser.js in the tag.

I'll fix this, but this likely implies that for respins of this nature, the build team will have to come up with a better protocol for confirming the inclusion of patches. Maybe that's looking at the patch (although, I'd rather not do that), and maybe it's getting a complete bonsai query from the release driver/developer before modifying the CVS tag.

Comment 24

[Jay Patel [:jay]]

v.fixed on latest 1.0.8 candidate build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060408 Firefox/1.0.8 (ftp timestamp: 08-Apr-2006 17:51)

All of Gavin's testcases pass and general browsing on topsites (yahoo, google, cnn, espn, etc) show the correct context menus for page conent, form fields, and images.

Marking Fixed again.

Comment 25

[Mike Schroepfer]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060408 Firefox/1.0.8

Test    1.0.8                 1.5.0.2
-----   ---------             ----------
1       image menu(no save)   image menu(no save)
2       page menu             page menu
3       image menu(save)      image menu(save)
4       image menu(save)      image menu(save)
5       image menu(no save)   image menu(no save)
6       image menu(save)      image menu(save)
7       image menu(save)      image menu(save)
8       image menu(save)      image menu(save)
9       text menu             text menu


Based on my understanding of this testcase this looks to be passing.  I'm at least seeing identical behavior between 1.5.0.2 and 1.0.8.

Comment 26

[Tracy Walker [:tracy]]

Jay Verified on Windows 1.0.8 respn. I just verified the testcase looks good on Linux 1.0.8 respins

Comment 27

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

Attachment: testcase #2 (relative .src)

just dumping this here in case its useful

Comment 28

[Mike Schroepfer]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060410 Firefox/1.0.8

Looks good on latest respin.

Comment 29

[Jay Patel [:jay]]

v.fixed with latest respins on Windows and Linux as well.  I see the correct image context menus, but one thing I noticed was that the last two images for "And the following with mozilla.org blocked from loading images:" did have the "Save..." context menu item (but that might be ok if mozilla.org images are not currently blocked)?

Gavin:  Could you explain what I'm seeing and clarify the testcase a bit for us?  I'm just curious to know the details. Thanks! ;-)

Comment 30

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

> but one thing I noticed was that the last two images for "And the following
> with mozilla.org blocked from loading images:" did have the "Save..." context
> menu item (but that might be ok if mozilla.org images are not currently
> blocked)?

That is correct. The "expected results" are with mozilla.org blocked. If mozilla.org is not blocked, the behavior should be the same as cases #3 and #6, respectively (normal image context menu, with save).

Comment 31

[Tracy Walker [:tracy]]

Verified on Mozilla 1.7.13 candidates from 04-14 on Mac and Windows