convert autocomplete-rich-result-popup into CE

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
2 months ago
5 days ago

People

(Reporter: surkov, Assigned: surkov, NeedInfo)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(8 attachments, 5 obsolete attachments)

16.11 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Comment hidden (empty)
(Assignee)

Updated

2 months ago
Assignee: nobody → surkov.alexander

Updated

2 months ago
Priority: -- → P2
(Assignee)

Comment 3

a month ago

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

Flags: needinfo?(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.

Flags: needinfo?(emilio)

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.

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

Keywords: leave-open

Comment 7

a month ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f9e50bfaad1
Make the XULPopupManager caller to GenerateChildFrames sound. r=mats
(Assignee)

Comment 8

a month ago

(In reply to Pulsebot from comment #7)

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f9e50bfaad1
Make the XULPopupManager caller to GenerateChildFrames sound. r=mats

huge thanks, Emilio, for fixing it!

Should be fixed now.

Flags: needinfo?(emilio)

Comment 12

a month ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c405d8906d7c
Make the XULPopupManager caller to GenerateChildFrames sound. r=mats
Attachment #9044193 - Attachment is obsolete: true
Attachment #9041269 - Attachment is obsolete: true
Attachment #9044190 - Attachment is obsolete: true
Attachment #9044192 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Component: XBL → XUL Widgets
Product: Core → Toolkit
(Assignee)

Comment 21

a month ago

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

Flags: needinfo?(enndeakin)
Flags: needinfo?(enndeakin)

Comment 26

29 days ago
Pushed by asurkov@mozilla.com:
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
Attachment #9045409 - Attachment is obsolete: true
Attachment #9045334 - Attachment description: Bug 1525101 - copy autocomplete.xml to keep git history for autocomplete-rich-result-popup binding, r=bgrins → Bug 1525101 - Copy autocomplete.xml to keep hg history for autocomplete-rich-result-popup binding, r=MattN
Attachment #9045301 - Attachment description: Bug 1525101 - convert autocomplete-rich-result-popup into CE, r=bgrins → Bug 1525101 - Convert autocomplete-rich-result-popup into a Custom Element, r=MattN
Attachment #9045927 - Attachment description: Bug 1525101 - copy search.xml to keep hg history for search custom elements, r=bgrins → Bug 1525101 - Copy search.xml to keep hg history for search custom elements, r=MattN
Attachment #9045929 - Attachment description: Bug 1525101 - migrate browser-search-autocomplete-result-popup to custom elements, r=bgrins → Bug 1525101 - Convert browser-search-autocomplete-result-popup into a Custom Element, r=MattN

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

Flags: qe-verify+

Comment 29

21 days ago
Pushed by asurkov@mozilla.com:
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
(Assignee)

Comment 31

20 days ago

(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 +)

Flags: qe-verify+ → qe-verify?

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

Flags: qe-verify? → qe-verify+
(Assignee)

Comment 33

20 days ago

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

20 days ago
Pushed by asurkov@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Last Resolved: 19 days ago
status-firefox67: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1530961

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.

(Assignee)

Comment 38

19 days ago

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

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

Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 40

5 days ago

(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

Flags: needinfo?(surkov.alexander)

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

Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.