398135 - [FIX]Since the landing of 372769, adblock plus fails to allow new filters to be added

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[mdew]

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

Comment 1

[Wladimir Palant]

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.

Comment 2

[Dão Gottwald [:dao]]

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 ...

Comment 3

[Wladimir Palant]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Wladimir Palant]

(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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 1. Install Adblock Plus 0.7.5.3 in Firefox

URI for this?

Comment 7

[Wladimir Palant]

https://addons.mozilla.org/en-US/firefox/addon/1865

Comment 8

[Wladimir Palant]

Attachment: Minimized testcase

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.

Comment 9

[mdew]

(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

Comment 10

[Boris Zbarsky [:bzbarsky]]

> menulist element is moved after the document loaded

That would do it!  I should have a patch soon.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

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.

Comment 12

[mdew]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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).

Comment 14

[Wladimir Palant]

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.

Comment 15

[Wladimir Palant]

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.

Comment 16

[Wladimir Palant]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[patrickdrd]

so, how can we make this one work?

Comment 19

[Boris Zbarsky [:bzbarsky]]

We wait for the patch to be reviewed?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> 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.  ;)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 23

[Carsten Book [:Tomcat]]

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