Huge context menu pops up when right click after selected text in sidebar web page. And Context menu for selection never pops up

RESOLVED FIXED in Firefox 4.0b12

Status

()

defect
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: alice0775, Assigned: mak)

Tracking

(Blocks 1 bug, {regression})

Trunk
Firefox 4.0b12
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker])

Attachments

(2 attachments, 2 obsolete attachments)

Posted image screen shot
Build Identifier:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110208 Firefox/4.0b12pre ID:20110208030358

When I was testing Bug 627065, I encountered the following problem.

Huge context menu pops up when right click after selected text in sidebar web page.
After even once displayed another context menu, the problem does not occur.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Bookmark any page and set "Load this bookmark in the sidebar"
3. Open the bookmark in the sidebar
4. Select text
5. Right click

Actual Results:
 Huge context menu pops up.

Expected Results:
 Normal context menu should pop up.

Regression window(cached m-c hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/00955067e4b5
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100504 Minefield/3.7a5pre ID:20100505053644
Fails:
http://hg.mozilla.org/mozilla-central/rev/d7393e28fb2d
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100504 Minefield/3.7a5pre ID:20100505071655
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=00955067e4b5&tochange=d7393e28fb2d
Suspected bug;
d7393e28fb2d	Asaf Romano — Bug 528884 - Remove places' menu and toolbar bindings. r=mak
blocking2.0: --- → ?
Error shows when popup huge context menu in error console.

Error: BrowserSearch is undefined
Source file: chrome://browser/content/nsContextMenu.js
Line: 1128

      var ss = Cc["@mozilla.org/browser/search-service;1"].
               getService(Ci.nsIBrowserSearchService);
>>    if (isElementVisible(BrowserSearch.searchBar))
        engineName = ss.currentEngine.name;
      else
Component: General → Menus
QA Contact: general → menus
the error could make sense but the relation with the Places patch is not that clear
In addition to comment #0
>After even once displayed another context menu, the problem does not occur.
However, It is not context menu related to a selected text.
Summary: Huge context menu pops up when right click after selected text in sidebar web page. → Huge context menu pops up when right click after selected text in sidebar web page. And Context menu for selection never pops up
(In reply to comment #2)
> the error could make sense but the relation with the Places patch is not that
> clear

In local build, I confirmed that the following cset causes the problem.
d7393e28fb2d	Asaf Romano — Bug 528884 - Remove places' menu and toolbar bindings. r=mak

build from d7393e28fb2d : fails
build from 2c98eeb202b9 : works
build from 00955067e4b5 : works
This pains me to say, but it's a softblocker unless there's some more common path to trigger it. Mano - what's the likelihood that this is worse than it seems?
blocking2.0: ? → final+
Whiteboard: [softblocker]
The real error is:
Error: PlacesMenu is not defined
Source File: chrome://browser/content/browser.js
Line: 2935

That triggers "BrowserSearch is not defined" later because browser.js failed to load completely.
Posted patch option #1 (obsolete) — Splinter Review
I don't think any of the other global-scripts are dangerous to include, and this might help avoid this kind of trouble in the future? Maybe overkill, though...
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #510705 - Flags: review?(mano)
Posted patch option #2 (obsolete) — Splinter Review
Attachment #510709 - Flags: review?(mano)
(In reply to comment #7)
> Created attachment 510705 [details] [diff] [review]
> option #1

Er, this patch is reversed somehow. But you get the idea!
ah, the PlacesMenu issue is tracked by bug 610187 as well (see bug 610187 comment 2), I think Mano had ideas regarding that, but there is no clear winner and we left that for post FX4.
Personally I'd just include global-script.
Depends on: 610187
Actually, we could even just do a mock PlacesMenu in the sidebar context, as I pointed out in bug 610187... What happens here is that we have a object that inherits from PlacesMenu, when the file is read the prototype is created, and here we fail... but we don't need that object in the sidebar.
We could even set the inheriting proto in the constructor I guess?
Posted patch option #3Splinter Review
I think this is a less scary option, if we want to delay solving the borken browser dependencies we have here to FX5, in the meanwhile we could take this and leave Bug 610187 to properly solve the issue with more thoughts.
both history menu and the context menu work fine after the fix.
Attachment #510728 - Flags: review?(mano)
Gavin, do we know for sure that there's any benifit from #including browser-places rathan having this script separated?

Another options is to move HistoryMenu to browserPlacesViews.
Comment on attachment 510705 [details] [diff] [review]
option #1

There is no reason to load browserPlacesViews.js in the sidebar.
Attachment #510705 - Flags: review?(mano) → review-
Comment on attachment 510728 [details] [diff] [review]
option #3

If we don't do any of the options I propossed above, I would take this one.
Moving HistoryMenu has other dependency implications, since it includes code from Sync and Session Restore. Most likely it would work fine but at this late stage in the product I'd take the workaround and let the final solution to Bug 610187 for post-FX4.
I really cannot see how  would it break anything, at all. We use browserPlacesViews only in browser.xul (by design). Thus, any dependencies of HistoryMenu on browser.xul scripts wouldn't be affected.  There's no reason to nastyify the code if the good fix isn't risky.
Moving things around is inherently risky, it's nearly impossible to be sure that it won't break things. Let's just go with Marco's patch for now (though I'd use |this| rather than |HistoryMenu|).
Assignee: gavin.sharp → mak77
Comment on attachment 510728 [details] [diff] [review]
option #3

Well, fine with me. r=mano.
Attachment #510728 - Flags: review?(mano) → review+
(In reply to comment #18)
> I'd use |this| rather than |HistoryMenu|).

that doesn't work, I tried a patch with "this" in the Console and it didn't work... I tried in the Error Console though so it's possible new js makes that possible. will check.
This works for me:

function a() { } a.prototype = { x: 1}; function b() { this.__proto__.__proto__ = a.prototype } b.prototype = { y: 2 }; var c = new b(); alert(c.x + " " + c.y);

But if you're really looking for correctness - I don't think you should - use Object.create.
yeah that works, also arguments.callee.prototype.__proto__ works...
Mano I still think my original fix is more readable than both "this.__proto__.__proto__" and "arguments.callee.prototype.__proto__" approaches... I'd go for the original one if you're fine with it, Gavin said he has faith in us (just a little bit) ;)
Ah I missed the Object.create suggestion, looking into it.
Object.create unfortunately is still a bit factory-ish.
Let's stop the bikeshed discussion, I took a look around to see what's the most used standard for this kind of inheritance and looks like __proto__.__proto__ is pretty much used.
So I'll go for this.__proto__.__proto__, I'll directly fix that in the push.
Thank you!
Comment 19 said just "r=mano", not "r=mano with gavin's nit addressed". I'm fine with all options. Here's another one:

window.a = function() { }; a.prototype = { x: 3 }; var b = function() { }; b.prototype = { __proto__: "a" in window ? window.a.prototype  : null }; alert((new b()).x) 

vs

var b = function() { }; b.prototype = { __proto__: "a" in window ? window.a.prototype  : null }; alert((new b()).x)
ok, let's stop overengineering this thing, there is better stuff for a headache.
http://hg.mozilla.org/mozilla-central/rev/98cf4955a4b5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
Attachment #510705 - Attachment is obsolete: true
Attachment #510709 - Attachment is obsolete: true
Duplicate of this bug: 637100
You need to log in before you can comment on or make changes to this bug.