Closed Bug 333131 Opened 15 years ago Closed 15 years ago

[1.0.8] Image Context menu opens as Normal Context Menu

Categories

(Core :: XUL, defect)

1.7 Branch
defect
Not set
blocker

Tracking

()

VERIFIED FIXED

People

(Reporter: tracy, Assigned: Gavin)

References

Details

(Keywords: regression, verified1.7.13)

Attachments

(4 files, 1 obsolete file)

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.
Are we still pulling by date and not updated the date for the fix for 333035?
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?
Assignee: nobody → gavin.sharp
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
Status: NEW → ASSIGNED
Attached patch patch? (obsolete) — Splinter Review
Looking at nsImageLoadingContent::GetCurrentURI, I guess I can just check that a current request exists? This seems to work.
Attachment #217564 - Flags: superreview?(dveditz)
Attachment #217564 - Flags: review?
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).
Attachment #217564 - Attachment is obsolete: true
Attachment #217564 - Flags: superreview?(dveditz)
Attachment #217564 - Flags: review?
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....  :(
Attachment #217564 - Attachment description: patch → patch?
Attachment #217564 - Attachment is obsolete: false
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?
(because then you don't have to worry about clearing it)
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 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...))
The idea from comment 9 should work (with the imageBlockingStatus prop and checking whether it's a REJECT_*, of course).
Attached file testcase
Attachment #217564 - Attachment is obsolete: true
This passes all the tests in the attached testcase. Not sure who to ask for review, if anyone wants to stamp it go ahead.
Attachment #217604 - Flags: superreview?(bzbarsky)
Attachment #217604 - Flags: review?
Comment on attachment 217604 [details] [diff] [review]
Firefox patch (dveditz's suggestion)

r=dveditz
Attachment #217604 - Flags: review? → review+
Attachment #217604 - Flags: superreview?(bzbarsky) → superreview+
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.
Attachment #217607 - Flags: approval1.7.13?
Attachment #217604 - Attachment description: aviary patch (dveditz's suggestion) → Firefox patch (dveditz's suggestion)
Attachment #217604 - Flags: approval-aviary1.0.8?
Comment on attachment 217604 [details] [diff] [review]
Firefox patch (dveditz's suggestion)

Critical regression to fix. a=timr for drivers
Attachment #217604 - Flags: approval-aviary1.0.8? → approval-aviary1.0.8+
This is a critical regression for 1.0.8 and 1.7.13.  a=timr for drivers
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #217607 - Flags: approval1.7.13?
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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/
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
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?).
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.
(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.
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.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
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.
Jay Verified on Windows 1.0.8 respn. I just verified the testcase looks good on Linux 1.0.8 respins
Depends on: 333305
just dumping this here in case its useful
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.
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! ;-)
> 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).
Verified on Mozilla 1.7.13 candidates from 04-14 on Mac and Windows
Status: RESOLVED → VERIFIED
Summary: Image Context menu opens as Normal Context Menu → [1.0.8] Image Context menu opens as Normal Context Menu
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.