convert autocomplete-rich-result-popup into CE

VERIFIED FIXED

Status

()

task
P2
normal
VERIFIED FIXED
4 months ago
12 days ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 verified, firefox68 verified)

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
Assignee

Description

4 months ago
No description provided.
Assignee

Updated

4 months ago
Assignee: nobody → surkov.alexander

Updated

4 months ago
Priority: -- → P2
Assignee

Comment 3

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

Comment 7

4 months 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

4 months 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

4 months 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

4 months ago

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

Flags: needinfo?(enndeakin)

Comment 22

4 months ago
Flags: needinfo?(enndeakin)

Comment 26

4 months 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

3 months 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

3 months 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

3 months 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

3 months 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
Closed: 3 months ago
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

3 months 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

3 months 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)

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.

Flags: needinfo?(MattN+bmo)

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

12 days ago
Type: defect → task
You need to log in before you can comment on or make changes to this bug.