1525101 - convert browser-search-autocomplete-result-popup into CE

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, task, P2)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - convert autocomplete-rich-result-popup into CE, r=bgrins

Comment 2

[alexander :surkov (:asurkov)]

Attachment: nsIAutoCompletePopup interface

Comment 3

[alexander :surkov (:asurkov)]

Emilio, would you please take a look at assertion I get [1], in nsCSSFrameConstructor::ShouldCreateItemsForChild:

Assertion failure: false (asked to create frame construction item for a node that already has a frame), at /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5093

It fails for a richlistbox element inside a penel element, which is Custom Element now (was used to be XBL binding).

[1] https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227752282&repo=try&lineNumber=13557

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Well, the assertion happens here:

(rr) p DumpJSStack()
0 _openAutocompletePopup(aInput = [object XULElement], aElement = [object XULElement]) ["chrome://global/content/elements/autocomplete.js":300:6]
    this = [object XULPopupElement]
1 openAutocompletePopup(aInput = [object XULElement], aElement = [object XULElement]) ["chrome://global/content/elements/autocomplete.js":282:4]
    this = [object XULPopupElement]
2 openPopup() ["chrome://global/content/bindings/autocomplete.xml":338:12]
    this = [object XULElement]
3 set_popupOpen(val = true) ["chrome://global/content/bindings/autocomplete.xml":89:9]
    this = [object XULElement]
4 startSearch(aString = "r", aParam = "", aResult = null, aListener = [xpconnect wrapped (nsISupports, nsIAutoCompleteController, nsIAutoCompleteObserver) @ 0x7f1672e63c40 (native @ 0x7f1672e4f500)]) ["chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete2.xul":72:4]
    this = [object Object]

From nsXULPopupManager::FirePopupShowingEvent, which calls into frame construction directly:

https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/layout/xul/nsXULPopupManager.cpp#1289

The node is the richlistbox, which got its frame on the connectedCallback:

(rr) p DumpJSStack()
0 connectedCallback() ["chrome://global/content/elements/autocomplete.js":92:4]
    this = [object XULPopupElement]
1 get_popup() ["chrome://global/content/bindings/autocomplete.xml":79:12]
    this = [object XULElement]
2 startTest() ["chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete2.xul":113:5]
    this = [object Window]

void

The reason it got a frame is kind of expected:

https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/layout/xul/nsMenuPopupFrame.cpp#409

I guess the way this works with XBL is that it is hitting:

https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/layout/xul/nsMenuPopupFrame.cpp#398

While that's not the case for the dynamic insertion that's happening with the custom element, which goes via InsertFrames.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1525101 - Make the XULPopupManager caller to GenerateChildFrames sound.

It only works basically by chance with XBL and doesn't handle any kind of
dynamic insertion. See comment 4 in the bug for the diagnostic.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I attached a patch that should fix that. There's a test failure still, but that seems an unrelated issue with your patch.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f9e50bfaad1
Make the XULPopupManager caller to GenerateChildFrames sound. r=mats

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Pulsebot from comment #7)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f9e50bfaad1
Make the XULPopupManager caller to GenerateChildFrames sound. r=mats

huge thanks, Emilio, for fixing it!

Comment 9

[Dorel Luca [:dluca]]

Backed out changeset 7f9e50bfaad1 (bug 1525101) for Mochitest failures in toolkit/content/tests/chrome/test_menulist_paging.xul

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227947255&repo=autoland&lineNumber=18640

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7f9e50bfaad127c642d8537b1a4d4feda283048b

Backout:
https://hg.mozilla.org/integration/autoland/rev/b6f75644ba86d2cc6f4af683a2c45e4f138d02cc

Comment 10

[Andreea Pavel [:apavel]]

There is also this bc failure https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227985450&repo=autoland&lineNumber=1594

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Should be fixed now.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c405d8906d7c
Make the XULPopupManager caller to GenerateChildFrames sound. r=mats

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c405d8906d7c

Comment 14

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - make nsIAutoCompletePopup::popup implementable by custom elements

Comment 15

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - copy autocomplete.xml to keep autocomplete-richlistbox binding, r=bgrins

Comment 16

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - convert autocomplete-rich-result-popup into CE

Comment 17

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - adjust nsIAutoCompletePopup to make custom element popups working, r=peterv

Comment 18

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - adjust nsIFormFillController to make custom elements popup working, r=peterv

Comment 19

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - Convert autocomplete-rich-result-popup into a Custom Element, r=MattN

Comment 20

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - Copy autocomplete.xml to keep hg history for autocomplete-rich-result-popup binding, r=MattN

Comment 21

[alexander :surkov (:asurkov)]

Neil, could you take a look at the approach taken for nsIAutoCompletePopup/nsIFormFillController? Does it look reasonable?

Comment 22

[Neil Deakin]

I commented on https://phabricator.services.mozilla.com/D20504

Comment 23

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - copy autocomplete.xml to keep hg history for autocomplete-rich-result-popup binding, r=bgrins

Comment 24

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - Copy search.xml to keep hg history for search custom elements, r=MattN

Comment 25

[alexander :surkov (:asurkov)]

Attachment: Bug 1525101 - Convert browser-search-autocomplete-result-popup into a Custom Element, r=MattN

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bea571f9708b
adjust nsIAutoCompletePopup to make custom element popups working, r=peterv
https://hg.mozilla.org/integration/autoland/rev/fcf3d5468b1f
adjust nsIFormFillController to make custom elements popup working, r=peterv

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/bea571f9708b
https://hg.mozilla.org/mozilla-central/rev/fcf3d5468b1f

Comment 28

[Matthew N. [:MattN]]

QA should verify that autocomplete popups in content and the searchbar popup work fine.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87dab0b128cb
Copy autocomplete.xml to keep hg history for autocomplete-rich-result-popup binding, r=bgrins,MattN
https://hg.mozilla.org/integration/autoland/rev/a83c218cd961
Convert autocomplete-rich-result-popup into a Custom Element, r=MattN

Comment 30

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/87dab0b128cb
https://hg.mozilla.org/mozilla-central/rev/a83c218cd961

Comment 31

[alexander :surkov (:asurkov)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #28)

QA should verify that autocomplete popups in content and the searchbar popup work fine.

I suppose it should be ? request (not +)

Comment 32

[Matthew N. [:MattN]]

(In reply to alexander :surkov (:asurkov) from comment #31)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #28)

QA should verify that autocomplete popups in content and the searchbar popup work fine.

I suppose it should be ? request (not +)

No, that's not how it works… please read the tooltip on the dropdown.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #32)

(In reply to alexander :surkov (:asurkov) from comment #31)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #28)

QA should verify that autocomplete popups in content and the searchbar popup work fine.

I suppose it should be ? request (not +)

No, that's not how it works… please read the tooltip on the dropdown.

oh, right, sorry, was confused with review request

btw, is there anything left on my side about the last search-popup before review/landing?

Comment 34

[Matthew N. [:MattN]]

I'm reviewing it now :)

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49ae69e7af10
Copy search.xml to keep hg history for search custom elements, r=bgrins,MattN
https://hg.mozilla.org/integration/autoland/rev/ebd4899255b6
Convert browser-search-autocomplete-result-popup into a Custom Element, r=MattN

Comment 36

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/49ae69e7af10
https://hg.mozilla.org/mozilla-central/rev/ebd4899255b6

Comment 37

[Mark Banner (:standard8)]

In case it comes up, this broke right-click -> search in new tab for the existing address bar. I'm fixing that in bug 1530961 as I was adding a test there and fixing it for QuantumBar anyway.

Comment 38

[alexander :surkov (:asurkov)]

(In reply to Mark Banner (:standard8) from comment #37)

In case it comes up, this broke right-click -> search in new tab for the existing address bar. I'm fixing that in bug 1530961 as I was adding a test there and fixing it for QuantumBar anyway.

thank you, I somehow missed that quantum search still has a dependency on autocomplete popup XBL binding.

Comment 39

[Daniel Bodea [:danibodea]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #28)

QA should verify that autocomplete popups in content and the searchbar popup work fine.

How am I supposed to verify these steps? Steps to reproduce required to verify this bug.

Comment 40

[alexander :surkov (:asurkov)]

(In reply to Bodea Daniel [:danibodea] from comment #39)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #28)

QA should verify that autocomplete popups in content and the searchbar popup work fine.

How am I supposed to verify these steps? Steps to reproduce required to verify this bug.

Matt might be a better person to answer this, but I used [1] to test in content autocomplete popups and searchbar, available from Customize (context menu of Firefox toolbar).

[1] https://luke-chang.github.io/autofill-demo/autocomplete-all.html

Comment 41

[Daniel Bodea [:danibodea]]

@Matt: Can you help me with some steps to reproduce/verify?

Comment 42

[Matthew N. [:MattN]]

There are no specific steps since this wasn't a regression fix, it was a refactor. You would have to explore behaviour with autocomplete popups on https://luke-chang.github.io/autofill-demo/autocomplete-all.html for addresses, credit cards, logins, and regular form history (no key icon or grey footer). You should also test the searchbar autocomplete popup. Look for regressions compared to before this change.

Comment 43

[Daniel Bodea [:danibodea]]

This bug was verified with the feature testing in Fx 67 and second run in Fx 68.