Closed
Bug 333131
Opened 18 years ago
Closed 18 years ago
[1.0.8] Image Context menu opens as Normal Context Menu
Categories
(Core :: XUL, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: tracy, Assigned: Gavin)
References
Details
(Keywords: regression, verified1.7.13)
Attachments
(4 files, 1 obsolete file)
3.74 KB,
text/html
|
Details | |
1.93 KB,
patch
|
dveditz
:
review+
bzbarsky
:
superreview+
timr
:
approval-aviary1.0.8+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
Details | Diff | Splinter Review | |
670 bytes,
text/html
|
Details |
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•18 years ago
|
||
Are we still pulling by date and not updated the date for the fix for 333035?
Assignee | ||
Comment 2•18 years ago
|
||
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?
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•18 years ago
|
||
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?
Assignee | ||
Comment 4•18 years ago
|
||
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?
Comment 5•18 years ago
|
||
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.... :(
Assignee | ||
Updated•18 years ago
|
Attachment #217564 -
Attachment description: patch → patch?
Attachment #217564 -
Attachment is obsolete: false
Comment 6•18 years ago
|
||
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•18 years ago
|
||
(because then you don't have to worry about clearing it)
Comment 8•18 years ago
|
||
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•18 years ago
|
||
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...))
Blocks: 333035
Comment 10•18 years ago
|
||
The idea from comment 9 should work (with the imageBlockingStatus prop and checking whether it's a REJECT_*, of course).
Assignee | ||
Comment 11•18 years ago
|
||
Attachment #217564 -
Attachment is obsolete: true
Assignee | ||
Comment 12•18 years ago
|
||
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 13•18 years ago
|
||
Comment on attachment 217604 [details] [diff] [review] Firefox patch (dveditz's suggestion) r=dveditz
Attachment #217604 -
Flags: review? → review+
Updated•18 years ago
|
Attachment #217604 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 14•18 years ago
|
||
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?
Assignee | ||
Updated•18 years ago
|
Attachment #217604 -
Attachment description: aviary patch (dveditz's suggestion) → Firefox patch (dveditz's suggestion)
Attachment #217604 -
Flags: approval-aviary1.0.8?
Comment 15•18 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•18 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+
Assignee | ||
Comment 17•18 years ago
|
||
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: 18 years ago
Keywords: fixed-aviary1.0.8,
fixed1.7.13
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Attachment #217607 -
Flags: approval1.7.13?
Reporter | ||
Comment 18•18 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•18 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•18 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•18 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?).
Assignee | ||
Comment 22•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Keywords: fixed-aviary1.0.8,
fixed1.7.13
Comment 23•18 years ago
|
||
(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•18 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
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Comment 25•18 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.
Assignee | ||
Updated•18 years ago
|
Keywords: fixed-aviary1.0.8,
fixed1.7.13
Reporter | ||
Comment 26•18 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
Assignee | ||
Comment 27•18 years ago
|
||
just dumping this here in case its useful
Comment 28•18 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•18 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! ;-)
Assignee | ||
Comment 30•18 years ago
|
||
> 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•18 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
Assignee | ||
Updated•18 years ago
|
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.
Description
•