User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007100118 Minefield/3.0a9pre Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007100118 Minefield/3.0a9pre Unable to add any new filters into adblock plus Reproducible: Always Steps to Reproduce: 1. Right click on an image 2. Select Adblock Image 3. press enter Actual Results: 4. nothing happens Expected Results: should be able to add new filters Regression Range 20070928_0331 works 20070928_0650 fails, bonsai, http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1190975460&maxdate=1190987399 caused by the landing of https://bugzilla.mozilla.org/show_bug.cgi?id=372769

I already managed to find out that menulist.focus() doesn't transfer input focus to the editor field correctly for some reason. I still have to investigate why this happens for Adblock Plus while my testcase works correctly.

Bug 372769 is "Do lazy evaluation of XBL fields." Somebody with privileges should set that dependency. (In reply to comment #1) > I already managed to find out that menulist.focus() doesn't transfer input > focus to the editor field correctly for some reason. Do you get "editor has no properties"? 'Cause that's what I've seen elsewhere ...

No, menulist.editor is indeed null but I am not using it. The issue is that after calling menulist.focus() the input focus is transfered to the menulist (Down Arrow key works) but not to the editor field (and calling menulist.inputField.focus() doesn't seem to do anything at all). This pretty much breaks editing of filters.

So what are the actual steps to reproduce, starting with a vanilla profile? Assume you're giving directions to someone who normally doesn't use either Adblock Plus nor Firefox (a good assumption). I assume the patch in bug 397924 doesn't affect this, right? I wouldn't expect it to, but.... Wladimir, is menulist.inputField returning the right object?

(In reply to comment #4) > Wladimir, is menulist.inputField returning the right object? It is - an HTML input field. It seems that we have timing issues here but I didn't figure out yet what kind of timing issues. Steps to reproduce: 1. Install Adblock Plus 0.7.5.3 in Firefox 2. Restart browser 3. Go to menu Tools / Adblock Plus 4. Click "Add filter" button Observed results: Inline editor (text field with dropdown list) appears - no caret in the text field however. Expected results: Caret should be in the text field.

> 1. Install Adblock Plus 0.7.5.3 in Firefox URI for this?

Clicking the button in the testcase should make the caret appear in the text field. This doesn't happen in the most recent Minefield builds. Note that this issue only occurs if the menulist element is moved after the document loaded (even though its position doesn't really change). Removing the load event handler from the testcase will make everything work correctly.

(In reply to comment #4) > I assume the patch in bug 397924 doesn't affect this, right? I wouldn't expect > it to, but.... tried the patch, no go, doesn't effect this bug

> menulist element is moved after the document loaded That would do it! I should have a patch soon.

So the problem was that we only evaluated fields if there was no such prop.... which meant that on removal and reinsertion the fields did NOT get reevaluated. This patch makes us actually remove all the props that correspond to field names when we unhook the binding. It also fixes bug 310107, I think. I can take out the changes that fix bug 310107 and just unhook the fields if you think the extra safety is worth it. But it seems odd to unhook the fields and leave the properties and methods in place. The patch does NOT fix bug 230086, mostly because I don't want to try to deal with the interaction between destructors here and the ones called on window close right now.

partially works with adblock plus, you can add new filters manually, but with this patch you cannot right-click on an object and add.

I have no idea what comment 12 is talking about, frankly. If there is a problem (either new with the patch or still remaining with the patch), I need to know the following: 1) Exact steps to reproduce (starting with a browser that has about:blank loaded). 2) Expected behavior. 3) Actual behavior. 4) Whether it's a problem without the patch, if you happen to know. 5) Whether it works on trunk prior to bug 372769, if you happen to have that information. Assume you're giving directions to someone who has never used either Adblock Plus or Firefox (which is more or less the case here, anyway).

Boris, I tested your patch. It fixes the testcase, now I see the message "editor.removeAllItems() is not a function" whenever I open Adblock Plus preferences (Tools / Adblock Plus). editor is a menulist, method removeAllItems() is defined in its binding. Note that I didn't have errors when opening this dialog before - it must be the "unhooking of methods" from your patch. I should attach a new testcase soon.

Ok, the issue mentioned in previous comment should probably be expected - Adblock Plus reinserts the editor as a child of a hidden box, meaning that the binding doesn't get applied, hence no method removeAllItems. Swapping two lines in the source code fixes it. I see some more strange behavior but it is probably unrelated, I will investigate this.

Yes, the other issues I noticed were my fault, I fixed them. So this patch is definitely doing its job correctly. Thank you, Boris.

Hmm. We could avoid unhooking the properties/methods when the binding is detached if we think it's too much of a change. We could even avoid blowing away fields if we're willing to eagerly install fields for any props that are not already defined on the JSObject. That does mean checking that for all fields at binding attachment time. I do think that this change is what we want, though.

so, how can we make this one work?

We wait for the patch to be reviewed?

Comment on attachment 283292 [details] [diff] [review] Possible patch it's a little scary to rip out the field values, though it is the right thing to do. Mostly worried about extensions failing, so it's not a huge deal for beta.

> Mostly worried about extensions failing, so it's not a huge deal for beta. Jonas, this is breaking parts of the Firefox UI too, most likely. See bug 398346 for example. In any case, it's a beta blocker so I'll just go land it. ;)

Fixed.

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102604 Minefield/3.0a9pre and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a9pre) Gecko/2007102605 Minefield/3.0a9pre with the steps to reproduce and the testcase -> Verified