Closed Bug 398135 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: XUL, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: mp3geek, Assigned: bzbarsky)

References

(Depends on 1 open bug, )

Details

(Keywords: regression, Whiteboard: [dbaron-1.9:RpCs])

Attachments

(2 files)

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
Summary: Since the landing of 372769, adblock fails to allow new filters to be added → Since the landing of 372769, adblock plus fails to allow new filters to be added
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.
Component: General → XP Toolkit/Widgets: XUL
OS: Linux → All
Product: Firefox → Core
QA Contact: general → xptoolkit.xul
Hardware: PC → All
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
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?
Blocks: 372769
Flags: blocking1.9?
(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?
Attached file 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.
(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.
Attached patch Possible patchSplinter Review
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #283292 - Flags: superreview?(jonas)
Attachment #283292 - Flags: review?(jonas)
Priority: -- → P1
Summary: Since the landing of 372769, adblock plus fails to allow new filters to be added → [FIX]Since the landing of 372769, adblock plus fails to allow new filters to be added
Target Milestone: --- → mozilla1.9 M9
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?
Flags: blocking1.9? → blocking1.9+
Whiteboard: [dbaron-1.9:RpCs]
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.
Attachment #283292 - Flags: superreview?(jonas)
Attachment #283292 - Flags: superreview+
Attachment #283292 - Flags: review?(jonas)
Attachment #283292 - Flags: review+
> 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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 400780
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
Status: RESOLVED → VERIFIED
Depends on: 401569
Depends on: 403556
No longer depends on: 403556
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Blocks: abp
You need to log in before you can comment on or make changes to this bug.