[1.0.8] Image Context menu opens as Normal Context Menu

VERIFIED FIXED

Status

()

--
blocker
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: tracy, Assigned: Gavin)

Tracking

({regression, verified1.7.13})

1.7 Branch
regression, verified1.7.13
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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

13 years ago
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
Posted 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).
Posted 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 15

13 years ago
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+

Comment 16

13 years ago
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
Last Resolved: 13 years ago
Keywords: fixed-aviary1.0.8, fixed1.7.13
Resolution: --- → FIXED
Attachment #217607 - Flags: approval1.7.13?
(Reporter)

Comment 18

13 years ago
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 → ---

Comment 19

13 years ago
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

13 years ago
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

13 years ago
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.
Keywords: fixed-aviary1.0.8, fixed1.7.13
(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

13 years ago
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
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Comment 25

13 years ago
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.
Keywords: fixed-aviary1.0.8, fixed1.7.13
(Reporter)

Comment 26

13 years ago
Jay Verified on Windows 1.0.8 respn. I just verified the testcase looks good on Linux 1.0.8 respins
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8
Depends on: 333305
just dumping this here in case its useful

Comment 28

13 years ago
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

13 years ago
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).
(Reporter)

Comment 31

13 years ago
Verified on Mozilla 1.7.13 candidates from 04-14 on Mac and Windows
Status: RESOLVED → VERIFIED
Keywords: fixed1.7.13 → verified1.7.13
Summary: Image Context menu opens as Normal Context Menu → [1.0.8] Image Context menu opens as Normal Context Menu

Updated

11 years ago
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.