725784 - XBL breaks when replacing a node with itself inside a panel

Status: RESOLVED WONTFIX

(Core :: XBL, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/7c0ba1c98ff7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0a1) Gecko/20120209 Firefox/13.0a1 ID:20120209031242

Label of remove button does not be taken into account plural bookmarks till restarting browser

Reproducible: Always

Steps to Reproduce:
1. Start Firefox with new profile
2. Enable Bookmark toolbar if any

3. Open a web site (Ex. http://www.mozilla.org/ )
4. Click STAR UI in Location bar
5. Click STAR UI again  --- you can see "Remove Bookmark" button

6. Drag a Identify Icon in the Location bar to Bookmarks toolbar and drop it. so that there are 2 bookmarks with same URL.

7. Click STAR UI again


Actual Results:
  The button is still labeled "Remove Bookmark"

Expected Results:
  The button should be labeled "Remove 2 Bookmarks"


Regression window(cachedm-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/7aa58a99a2e5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110630 Firefox/7.0a1 ID:20110630003937
Fails:
http://hg.mozilla.org/mozilla-central/rev/5c246f2bccb1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110630 Firefox/7.0a1 ID:20110630035829
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7aa58a99a2e5&tochange=5c246f2bccb1


Regression window(cached m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6777320f6f29
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110629 Firefox/7.0a1 ID:20110629215351
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c2f48684b9f5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110629 Firefox/7.0a1 ID:20110629231921
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6777320f6f29&tochange=c2f48684b9f5


In local build:
first bad changeset c2f48684b9f5
last good changeset fb03584dd823

Comment 1

[Alice0775 White]

This also happen on ubuntu.
http://hg.mozilla.org/mozilla-central/rev/7c0ba1c98ff7
Mozilla/5.0 (X11; Linux i686; rv:13.0a1) Gecko/20120209 Firefox/13.0a1 ID:20120209031242

Comment 2

[Boris Zbarsky [:bzbarsky]]

Is this a DOM bug, or a bug in the UI that's note expecting a remove+reinsert there?

Comment 3

[Alice0775 White]

And reproduce another way and it is same regression range.

It is not reflected to button label even if I delete one of the duplicating bookmark

Reproducible: Always

Steps to Reproduce:
1-7 same as commenet #0 --  make duplicate bookmarks
8. Restart browser
9. Click STAR UI again  --- you can see "Remove 2 Bookmarks" button as expected
10. Delete one of those bookmark
11. Click STAR UI again  

Actual Results:
  The button is still labeled "Remove 2 Bookmarks"

Expected Results:
  The button should be labeled "Remove Bookmark"

Comment 4

[Alice0775 White]

Attachment: possible fix

using setAttribute to set label strings

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 596070 [details] [diff] [review]
possible fix

So is the problem somehow that the remove and reinsert confuses the XBL binding?

Comment 6

[Alice0775 White]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Comment on attachment 596070 [details] [diff] [review]
> possible fix
> 
> So is the problem somehow that the remove and reinsert confuses the XBL
> binding?
I do not know why Element.label=xxx does not work in this case.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

There are no replaceChild calls in any of this code, so I'm not sure how bug 647603 would have caused this. Did it have wider impact than just replaceChild? Does XBL use replaceChild internally somehow?

Comment 8

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

I think the change also changed the behavior for things like

a.insertBefore(b, b);
a.appendChild(a.lastChild);

Comment 9

[Marco Bonardo [:mak]]

I may suppose that the first time _doShowEditBookmarkPanel is invoked rows.insertBefore(header, rows.firstChild); will move the header from the current position to the grid.  Next invokations would probably just remove and reinsert it from the grid, hitting the changed code path. So this should be fixable by just checking if header.parentNode is editBookmarkPanelGrid, makes sense.

Though this also means xbl is broken when removing and reinserting the same node.

Comment 10

[Marco Bonardo [:mak]]

taking cause I don't want to lose this, should be fixed before the release.

The local fix is simple, though I think we also want an XBL bug filed to check why the general binding is lost when reinserting the same node.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: attempt at a testcase

I tried using this to reproduce the bug, but it doesn't seem to. The bookmarks UI is rather complicated, it's difficult to minimize...

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Attachment: attempt at a testcase

(attached the wrong file)

Comment 13

[Marco Bonardo [:mak]]

Attachment: chrome testcase

I started from Gavin's test and added stuff till I could make a working testcase.
This is not yet a chrome test but should not be hard to make one.
It is not minimal yet, but I didn't want to hack on it till I had attached this working version somewhere. Just put it in a chrome tests folder, add to Makefile.in and run it. If you comment out
  rows.insertBefore(row, rows.firstChild);
the label correctly changes.

From what I see, being inside a <panel> is related to the issue.

Comment 14

[Marco Bonardo [:mak]]

Attachment: reduced chrome test

This is reduced and in form of a chrome test

Comment 15

[Marco Bonardo [:mak]]

Any idea where such a test should go in the tree?
layout/xul/test/?
toolkit/content/tests/chrome/?

That said, I'm not sure what's a good code point to start debugging the problem, any help is welcome.

Comment 16

[Marco Bonardo [:mak]]

Attachment: tests v1.0

test in patch form (toolkit/content/tests/chrome/test_panel_replaceSame.xul)

Comment 17

[Marco Bonardo [:mak]]

So, I'm transforming this bug in the one handling the XBL problem, and will use bug 726440 to fix the browser code that now is replacing the node with itself, since it's no more a no-op.

Comment 18

[Neil Deakin]

My guess is that when an element which has a binding but no frame, upon inserting, now gains a frame but the binding it had is lost. Whereabouts would this be handled?

Comment 19

[Boris Zbarsky [:bzbarsky]]

The frame constructor tries to create a binding if one is needed and if there isn't one yet.

Comment 20

[Neil Deakin]

Actually I realize now that in the testcase, the node is simply being moved to another place in the same panel, so it doesn't have a frame before or after the insert. I verified that no frame construction is happening during the call to insertBefore, and bindings are not being processed.

So whatever mechanism is creating the binding when the window first loads for frameless elements is happening, but not when the element is inserted later on.

Comment 21

[Boris Zbarsky [:bzbarsky]]

There is no such mechanism.  For frameless nodes, a binding is created when the node is first js-wrapped.

Comment 22

[Boris Zbarsky [:bzbarsky]]

XBL is now disabled in Firefox (Bug 1583314) and is in the process of being removed from Gecko (Bug 1566221), so closing bugs requesting changes to its implementation as wontfix.