Convert autocomplete-richlistitem and autocomplete-richlistitem-insecure-field to customized Custom Elements extending richlistitem

RESOLVED FIXED in Firefox 67

Status

()

P3
normal
RESOLVED FIXED
a month ago
4 days ago

People

(Reporter: bgrins, Assigned: surkov)

Tracking

(Depends on: 2 bugs, Blocks: 3 bugs)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [xbl-available])

Attachments

(3 attachments, 9 obsolete attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
(Reporter)

Description

a month ago

Originally in Bug 1512432 I was thinking we'd need to tackle all of the richlistitem bindings at once (https://bgrins.github.io/xbl-analysis/tree/#richlistitem).

After talking with Tim and Matt, we realized that there's likely a path forward where we can take just a chunk at a time, which will make the work easier to break down and make progress on.

autocomplete-richlistitem the inherited autocomplete-richlistitem-insecure-field bindings stood out as candidates to try this approach: https://bgrins.github.io/xbl-analysis/tree/#autocomplete-richlistitem.

Basic outline of what would need to happen:

  1. Take the patch from Bug 1512432, but don't actually change the binding or do customElement.define for richlistitem
  2. Extend these two bindings from the base class(es)
  3. Register them as customized built-ins, for example: customElements.define("autocomplete-richlistitem", ..., { extends: "richlistitem" }
  4. Create them with [is], for example: <richlistitem is="autocomplete-richlistitem">
  5. Set -moz-binding: none on the matching richlistitem[is] selector - in this case it's keying on the originaltype attr, so we'd update that to use [is]: https://searchfox.org/mozilla-central/rev/7adb490485eff0783071a3e132005bceeb337461/browser/base/content/browser.css#695
(Reporter)

Updated

a month ago
Whiteboard: [xbl-available]

Updated

a month ago
Priority: -- → P3
(Reporter)

Updated

a month ago
Blocks: 1519533
(Reporter)

Comment 1

29 days ago

A couple more notes:

(Assignee)

Comment 2

26 days ago

Created attachment 9038230 [details] [diff] [review]
part1: insecure field CE

part #1: insecure-field binding. Started try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b1f1091e750c94020e0186a28f273c8333381b7. Brian, sounds about right?

Assignee: nobody → surkov.alexander
Attachment #9038230 - Flags: feedback?(bgrinstead)
(Assignee)

Comment 3

26 days ago

aha, XXX: Implement this.inheritAttribute() is missing

(Assignee)

Comment 4

26 days ago

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

aha, XXX: Implement this.inheritAttribute() is missing

I was running ./mach test browser/extensions/formautofill/test/browser/browser_insecure_form.js on a local machine, and it didn't catch the problem. Do you have any suggestions how to test the binding/CE manually so that sort of thing doesn't slip unnoticed.

(Reporter)

Comment 5

26 days ago
Comment on attachment 9038230 [details] [diff] [review]
part1: insecure field CE

Review of attachment 9038230 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/autocomplete.xml
@@ +1085,5 @@
>                  item._cleanup();
>                }
>                item.setAttribute("originaltype", style);
> +              if (style == "insecureWarning") {
> +                item.setAttribute("is",

You can't set the [is] attribute after creating an element, the call to document.createElement needs to be updated with { is }. See https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/toolkit/components/printing/content/printUtils.js#546-547.

::: toolkit/content/widgets/richlistitem.js
@@ +7,5 @@
> +// This is loaded into all XUL windows. Wrap in a block to prevent
> +// leaking to window scope.
> +{
> +
> +class MozAutocompleteRichlistitemInsecureField extends MozXULElement {

This should extend MozRichlistitem as defined in https://hg.mozilla.org/try/rev/bca2b3ac9aaee4094d58e4cf022ac8b7aae437ec#l3.17. That patch should be pulled into this bug as part 1, with these two chunks removed:

https://hg.mozilla.org/try/rev/bca2b3ac9aaee4094d58e4cf022ac8b7aae437ec#l3.177
https://hg.mozilla.org/try/rev/bca2b3ac9aaee4094d58e4cf022ac8b7aae437ec#l4.12

@@ +81,5 @@
> +    return [this._learnMoreString.toLowerCase()];
> +  }
> +}
> +
> +customElements.define("autocomplete-richlistitem-insecure-field", ..., { extends: "richlistitem" }

This is a syntax error - it should be pass MozAutocompleteRichlistitemInsecureField in as the second param.
Attachment #9038230 - Flags: feedback?(bgrinstead)
(Reporter)

Comment 6

26 days ago

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

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

aha, XXX: Implement this.inheritAttribute() is missing

I was running ./mach test browser/extensions/formautofill/test/browser/browser_insecure_form.js on a local machine, and it didn't catch the problem. Do you have any suggestions how to test the binding/CE manually so that sort of thing doesn't slip unnoticed.

I'm surprised it passed since the Custom Element isn't being created properly in the patch, but maybe not having the CE / XBL only regresses visuals? Not sure. You may also try toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html. You can also test manually on the demo page at https://bugzilla.mozilla.org/show_bug.cgi?id=1519486#c1.

(Reporter)

Comment 7

26 days ago
Comment on attachment 9038230 [details] [diff] [review]
part1: insecure field CE

Review of attachment 9038230 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.css
@@ -705,5 @@
>    min-width: 200px;
>  }
>  
>  #PopupAutoComplete > richlistbox > richlistitem[originaltype="insecureWarning"] {
> -  -moz-binding: url("chrome://global/content/bindings/autocomplete.xml#autocomplete-richlistitem-insecure-field");

We should force `-moz-binding: none` here, since this is going to extend the richlistitem CE and we won't want both a XBL binding and a CE on the same element.
(Assignee)

Comment 8

26 days ago

(In reply to Brian Grinstead [:bgrins] from comment #5)

+class MozAutocompleteRichlistitemInsecureField extends MozXULElement {

This should extend MozRichlistitem as defined in
https://hg.mozilla.org/try/rev/bca2b3ac9aaee4094d58e4cf022ac8b7aae437ec#l3.
17. That patch should be pulled into this bug as part 1, with these two
chunks removed:

https://hg.mozilla.org/try/rev/bca2b3ac9aaee4094d58e4cf022ac8b7aae437ec#l3.
177
https://hg.mozilla.org/try/rev/bca2b3ac9aaee4094d58e4cf022ac8b7aae437ec#l4.12

oh, you want to have complete CE implementation of richlistitem for autocomplete-richlistitem-(insecure-field) widgets. Then it means I have to move autocomplete-richlistitem binding code into CE hierarchy at same time as well.

(Reporter)

Comment 9

26 days ago

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

(In reply to Brian Grinstead [:bgrins] from comment #5)

+class MozAutocompleteRichlistitemInsecureField extends MozXULElement {

This should extend MozRichlistitem as defined in
https://hg.mozilla.org/try/rev/bca2b3ac9aaee4094d58e4cf022ac8b7aae437ec#l3.
17. That patch should be pulled into this bug as part 1, with these two
chunks removed:

https://hg.mozilla.org/try/rev/bca2b3ac9aaee4094d58e4cf022ac8b7aae437ec#l3.
177
https://hg.mozilla.org/try/rev/bca2b3ac9aaee4094d58e4cf022ac8b7aae437ec#l4.12

oh, you want to have complete CE implementation of richlistitem for autocomplete-richlistitem-(insecure-field) widgets. Then it means I have to move autocomplete-richlistitem binding code into CE hierarchy at same time as well.

Yeah, this bug is tracking both of them (it'd probably be possible to tear them apart into two separate bugs, but it looks like more trouble than it's worth).

(Assignee)

Comment 10

25 days ago

Created attachment 9038371 [details] [diff] [review]
part1: insecure field CE (v2)

Attachment #9038230 - Attachment is obsolete: true
(Assignee)

Comment 11

25 days ago

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

Created attachment 9038371 [details] [diff] [review]
part1: insecure field CE (v2)

try build asserts "Callback should define the definition of type" at dom/base/CustomElementRegistry.cpp:333, presumably when running MozAutocompleteRichlistitemInsecureWarning CE. Any ideas what this assertion is about?

Attachment #9038371 - Attachment is patch: true
Attachment #9038371 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 12

25 days ago
Comment on attachment 9038371 [details] [diff] [review]
part1: insecure field CE (v2)

Review of attachment 9038371 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/customElements.js
@@ +375,5 @@
>  
> +  for (let [tag, scripts] of [
> +    ["findbar", ["chrome://global/content/elements/findbar.js"]],
> +    ["richlistbox", ["chrome://global/content/elements/richlistbox.js"]],
> +    ["richlistitem", ["chrome://global/content/elements/richlistitem.js",

You should key this on the name registered to customElements.define (in this case "autocomplete-richlistitem-insecure-warning") and not the base tag name.

So you can add two new entries here:

["richlistitem", "chrome://global/content/elements/richlistitem.js"],
["autocomplete-richlistitem-insecure-warning", "chrome://global/content/elements/autocomplete-richlistitem.js"],

and then drop the change which wraps the scripts into arrays
(Assignee)

Comment 13

25 days ago

(In reply to Brian Grinstead [:bgrins] from comment #12)

Comment on attachment 9038371 [details] [diff] [review]
part1: insecure field CE (v2)

Review of attachment 9038371 [details] [diff] [review]:

::: toolkit/content/customElements.js
@@ +375,5 @@

  • for (let [tag, scripts] of [
  • ["findbar", ["chrome://global/content/elements/findbar.js"]],
  • ["richlistbox", ["chrome://global/content/elements/richlistbox.js"]],
  • ["richlistitem", ["chrome://global/content/elements/richlistitem.js",

You should key this on the name registered to customElements.define (in this
case "autocomplete-richlistitem-insecure-warning") and not the base tag name.

right, thank you

So you can add two new entries here:

["richlistitem", "chrome://global/content/elements/richlistitem.js"],
["autocomplete-richlistitem-insecure-warning",
"chrome://global/content/elements/autocomplete-richlistitem.js"],

will it load "richlistitem" script? there's no customElements.define() for it?

(Reporter)

Comment 14

25 days ago

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

(In reply to Brian Grinstead [:bgrins] from comment #12)

Comment on attachment 9038371 [details] [diff] [review]
part1: insecure field CE (v2)

Review of attachment 9038371 [details] [diff] [review]:

::: toolkit/content/customElements.js
@@ +375,5 @@

  • for (let [tag, scripts] of [
  • ["findbar", ["chrome://global/content/elements/findbar.js"]],
  • ["richlistbox", ["chrome://global/content/elements/richlistbox.js"]],
  • ["richlistitem", ["chrome://global/content/elements/richlistitem.js",

You should key this on the name registered to customElements.define (in this
case "autocomplete-richlistitem-insecure-warning") and not the base tag name.

right, thank you

So you can add two new entries here:

["richlistitem", "chrome://global/content/elements/richlistitem.js"],
["autocomplete-richlistitem-insecure-warning",
"chrome://global/content/elements/autocomplete-richlistitem.js"],

will it load "richlistitem" script? there's no customElements.define() for it?

Oh, right. Yeah I think I'd just move richlistitem up into the unconditional loads for now.

(Assignee)

Comment 15

25 days ago

Created attachment 9038427 [details] [diff] [review]
part1: insecure field CE (v3)

window.getComputedStyle(result._typeIcon) fails in /browser_autocomplete_tag_star_visibility.js fails

browser/components/urlbar/tests/legacy/browser_autocomplete_tag_star_visibility.js
FAIL Uncaught exception - at chrome://mochitests/content/browser/browser/components/urlbar/tests/legacy/browser_autocomplete_tag_star_visibility.js:92 - TypeError: Argument 1 of Window.getComputedStyle is not an object.
Stack trace:
@chrome://mochitests/content/browser/browser/components/urlbar/tests/legacy/browser_autocomplete_tag_star_visibility.js:92:25
AsyncTester_execTest/<@chrome://mochikit/content/browser-test.js:1108:34
async
Tester_execTest@chrome://mochikit/content/browser-test.js:1099:16
nextTest/<@chrome://mochikit/content/browser-test.js:997:9
SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:803:59

I think this is because result._typeIcon is null, i.e. CE widget's connectedCallback is not yet called.

Any suggestions how to modify the test to workaround the issue?

Attachment #9038371 - Attachment is obsolete: true
(Assignee)

Comment 16

25 days ago

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

I think this is because result._typeIcon is null, i.e. CE widget's connectedCallback is not yet called.

Any suggestions how to modify the test to workaround the issue?

ignore it, I think I just found the issue

(Assignee)

Comment 17

25 days ago

Created attachment 9038549 [details] [diff] [review]
part1: insecure field CE (v4)

Attachment #9038427 - Attachment is obsolete: true
Attachment #9038549 - Flags: review?(bgrinstead)
(Reporter)

Comment 18

25 days ago
Comment on attachment 9038549 [details] [diff] [review]
part1: insecure field CE (v4)

Review of attachment 9038549 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/customElements.js
@@ +290,5 @@
> +    if (this.delayConnectedCallback()) {
> +      return;
> +    }
> +
> +    this.labelElement = "";

I think the converter could actually have ignored this field (https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/toolkit/content/widgets/general.xml#56).

Let's drop this entire connectedCallback function - I don't see any need for a default value here and labelElement will get set elsewhere.

::: toolkit/content/jar.mn
@@ +87,5 @@
>     content/global/bindings/toolbarbutton.xml   (widgets/toolbarbutton.xml)
>     content/global/bindings/tree.xml            (widgets/tree.xml)
>     content/global/bindings/videocontrols.xml   (widgets/videocontrols.xml)
>  *  content/global/bindings/wizard.xml          (widgets/wizard.xml)
> +   content/global/elements/autocomplete-richlistitem.js     (widgets/autocomplete-richlistitem.js)

These files aren't in the patch. Forgot to `hg add`?
Attachment #9038549 - Flags: review?(bgrinstead)
(Assignee)

Comment 19

25 days ago

Created attachment 9038557 [details] [diff] [review]
part1: insecure field CE

apparently I forgot to add new files

Attachment #9038549 - Attachment is obsolete: true
Attachment #9038557 - Flags: review?(bgrinstead)
(Assignee)

Comment 20

25 days ago

(In reply to Brian Grinstead [:bgrins] from comment #18)

Let's drop this entire connectedCallback function - I don't see any need for
a default value here and labelElement will get set elsewhere.

lgtm

::: toolkit/content/jar.mn
@@ +87,5 @@

content/global/bindings/toolbarbutton.xml   (widgets/toolbarbutton.xml)
content/global/bindings/tree.xml            (widgets/tree.xml)
content/global/bindings/videocontrols.xml   (widgets/videocontrols.xml)
  • content/global/bindings/wizard.xml (widgets/wizard.xml)
  • content/global/elements/autocomplete-richlistitem.js (widgets/autocomplete-richlistitem.js)

These files aren't in the patch. Forgot to hg add?

right, already fixed

(Reporter)

Comment 21

25 days ago
Comment on attachment 9038557 [details] [diff] [review]
part1: insecure field CE

Review of attachment 9038557 [details] [diff] [review]:
-----------------------------------------------------------------

Just did a quick round - I can have another look after it's updated. Have you tested this manually?

::: toolkit/content/widgets/autocomplete-richlistitem.js
@@ +49,5 @@
> +      control.currentItem = this;
> +    });
> +  }
> +
> +  static get observedAttributes() {

observedAttributes is set up here, but there's no `attributeChangedCallback`. The attribute inheriting code starting ~ line 109 can be extracted into a function and called in that, similar to https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/toolkit/content/widgets/tree.js#332.

@@ +94,5 @@
> +      </hbox>
> +    `));
> +
> +    this.setAttribute("align", "center");
> +    this.addEventListener("overflow", () => this._onOverflow());

I think these two event listeners can move out to the constructor (and be dropped from both connectedCallbacks)

@@ +123,5 @@
> +
> +    this._adjustAcItem();
> +  }
> +
> +  get _typeIcon() { return this.querySelector(".ac-type-icon"); }

Formatting nit - please split this up like:

get foo() {
  return bar;
}

Not sure if the converter put these all on one line? I would have expected them to be formatted consistently from the tool.

@@ +131,5 @@
> +  get _separator() { return this.querySelector(".ac-separator"); }
> +  get _urlText() { return this.querySelector(".ac-separator"); }
> +  get _actionText() { return this.querySelector(".ac-action-text"); }
> +
> +  get Services() {

Now that we are in JS we can clean this up and import Services.jsm before the class and the find/replace `this.Services` with `Services`

@@ +904,5 @@
> +      });
> +    });
> +  }
> +
> +  static get observedAttributes() {

Same issue with attributeChangedCallback as above.

@@ +914,5 @@
> +      "type",
> +    ];
> +  }
> +
> +  connectedCallback() {

I think we need to call super.connectedCallback() here (in XBL parent constructors fire as well). May require pulling some of the actual markup out into a getter and having only the base class do `this.appendChild(MozXULElement.parseXULToFragment(...` to avoid doing extra work in the DOM. (Assuming the markup is different between the two classes).

::: toolkit/content/widgets/richlistitem.js
@@ +147,5 @@
> +    var event = document.createEvent("Events");
> +    event.initEvent(name, true, true);
> +    this.dispatchEvent(event);
> +  }
> +  disconnectedCallback() {

Nit: newline between these functions
Attachment #9038557 - Flags: review?(bgrinstead)
(Assignee)

Comment 22

25 days ago

(In reply to Brian Grinstead [:bgrins] from comment #21)

Comment on attachment 9038557 [details] [diff] [review]
part1: insecure field CE

Review of attachment 9038557 [details] [diff] [review]:

Just did a quick round - I can have another look after it's updated. Have
you tested this manually?

yes, insecure form look same with and without the patch

  • static get observedAttributes() {

observedAttributes is set up here, but there's no
attributeChangedCallback. The attribute inheriting code starting ~ line
109 can be extracted into a function and called in that, similar to
https://searchfox.org/mozilla-central/rev/
39265dd58298c40bed029617ad408bf806cce761/toolkit/content/widgets/tree.js#332.

right, we should come with somewhat simpler API to inherit attributes

@@ +94,5 @@

  •  </hbox>
    
  • `));
  • this.setAttribute("align", "center");
  • this.addEventListener("overflow", () => this._onOverflow());

I think these two event listeners can move out to the constructor (and be
dropped from both connectedCallbacks)

k

@@ +123,5 @@

  • this._adjustAcItem();
  • }
  • get _typeIcon() { return this.querySelector(".ac-type-icon"); }

Formatting nit - please split this up like:

get foo() {
return bar;
}

sure, linting was ok with styling though

Not sure if the converter put these all on one line? I would have expected
them to be formatted consistently from the tool.

nah, I think it was me.

@@ +131,5 @@

  • get _separator() { return this.querySelector(".ac-separator"); }
  • get _urlText() { return this.querySelector(".ac-separator"); }
  • get _actionText() { return this.querySelector(".ac-action-text"); }
  • get Services() {

Now that we are in JS we can clean this up and import Services.jsm before
the class and the find/replace this.Services with Services

right

914,5 @@

  •  "type",
    
  • ];
  • }
  • connectedCallback() {

I think we need to call super.connectedCallback() here (in XBL parent
constructors fire as well). May require pulling some of the actual markup
out into a getter and having only the base class do
this.appendChild(MozXULElement.parseXULToFragment(... to avoid doing extra
work in the DOM. (Assuming the markup is different between the two classes).

agreed

(Assignee)

Comment 23

25 days ago

Created attachment 9038600 [details] [diff] [review]
part1: insecure field CE

Attachment #9038557 - Attachment is obsolete: true
Attachment #9038600 - Flags: review?(bgrinstead)
(Reporter)

Comment 24

25 days ago
Comment on attachment 9038600 [details] [diff] [review]
part1: insecure field CE

Review of attachment 9038600 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/autocomplete-richlistitem.js
@@ +899,5 @@
> +    return action;
> +  }
> +}
> +
> +MozXULElement.implementCustomInterface(

It looks like we should also `customElements.define("autocomplete-richlistitem", MozAutocompleteRichlistitem, { extends: "richlistitem" }) ` here, based on: https://bgrins.github.io/xbl-analysis/tree/#autocomplete-richlistitem.

Then set -moz-binding: none at https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/toolkit/content/xul.css#585
(Reporter)

Comment 25

25 days ago
Comment on attachment 9038600 [details] [diff] [review]
part1: insecure field CE

Review of attachment 9038600 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/autocomplete.xml
@@ +1231,1 @@
>    <binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">

This binding can be removed as well, right? this is MozAutocompleteRichlistitem
(Assignee)

Comment 26

25 days ago

(In reply to Brian Grinstead [:bgrins] from comment #24)

It looks like we should also
customElements.define("autocomplete-richlistitem", MozAutocompleteRichlistitem, { extends: "richlistitem" }) here, based on:
https://bgrins.github.io/xbl-analysis/tree/#autocomplete-richlistitem.

Then set -moz-binding: none at
https://searchfox.org/mozilla-central/rev/
39265dd58298c40bed029617ad408bf806cce761/toolkit/content/xul.css#585

(In reply to Brian Grinstead [:bgrins] from comment #25)

Comment on attachment 9038600 [details] [diff] [review]
part1: insecure field CE

Review of attachment 9038600 [details] [diff] [review]:

::: toolkit/content/widgets/autocomplete.xml
@@ +1231,1 @@

<binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">

This binding can be removed as well, right? this is
MozAutocompleteRichlistitem

yep, but it's 2nd part of the bug, right?

(Reporter)

Comment 27

25 days ago
Comment on attachment 9038600 [details] [diff] [review]
part1: insecure field CE

Review of attachment 9038600 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/widgets/autocomplete.xml
@@ +1054,5 @@
>              } else {
>                // need to create a new item
> +              let options = style == "insecureWarning" ?
> +                { is: "autocomplete-richlistitem-insecure-warning" } : null;
> +              item = document.createXULElement("richlistitem", options);

We'll need to also set { is: "autocomplete-richlistitem" } in the appropriate case, based on: https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/toolkit/content/widgets/autocomplete.xml#1101-1103.

It's not obvious to me just scanning that function, but I _think_ we will always want to set this if the style isn't "insecureWarning", so sojmething like:

let options = style == "insecureWarning" ?
  { is: "autocomplete-richlistitem-insecure-warning" } :
  { is: "autocomplete-richlistitem" };


Please also update the comment linked there in autocomplete.xml to remove any reference to XBL

::: toolkit/content/widgets/richlistitem.js
@@ +6,5 @@
> +// This is loaded into chrome windows with the subscript loader. If you need to
> +// define globals, wrap in a block to prevent leaking onto `window`.
> +
> +MozElements.MozRichlistitem =
> +class MozRichlistitem extends MozElements.BaseText {

Nit: put this on the same line as MozElements.MozRichlistitem =
(Reporter)

Comment 28

25 days ago

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

(In reply to Brian Grinstead [:bgrins] from comment #24)

It looks like we should also
customElements.define("autocomplete-richlistitem", MozAutocompleteRichlistitem, { extends: "richlistitem" }) here, based on:
https://bgrins.github.io/xbl-analysis/tree/#autocomplete-richlistitem.

Then set -moz-binding: none at
https://searchfox.org/mozilla-central/rev/
39265dd58298c40bed029617ad408bf806cce761/toolkit/content/xul.css#585

(In reply to Brian Grinstead [:bgrins] from comment #25)

Comment on attachment 9038600 [details] [diff] [review]
part1: insecure field CE

Review of attachment 9038600 [details] [diff] [review]:

::: toolkit/content/widgets/autocomplete.xml
@@ +1231,1 @@

<binding id="autocomplete-richlistitem" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">

This binding can be removed as well, right? this is
MozAutocompleteRichlistitem

yep, but it's 2nd part of the bug, right?

If it's not much extra to go ahead and cover both in one patch I think it could make sense (just this change + Comment 27). If there's a bunch of tests that need to be updated or something then splitting it into two patches would be fine.

(Assignee)

Comment 29

25 days ago

(In reply to Brian Grinstead [:bgrins] from comment #28)

yep, but it's 2nd part of the bug, right?

If it's not much extra to go ahead and cover both in one patch I think it could make sense (just this change + Comment 27). If there's a bunch of tests that need to be updated or something then splitting it into two patches would be fine.

agreed, there's a lesser chance to regress if keeping/landing those separately, and also a bit easier to review. However if you think it should be fine, then I can add the changes on top of this patch

(Reporter)

Comment 30

24 days ago

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

(In reply to Brian Grinstead [:bgrins] from comment #28)

yep, but it's 2nd part of the bug, right?

If it's not much extra to go ahead and cover both in one patch I think it could make sense (just this change + Comment 27). If there's a bunch of tests that need to be updated or something then splitting it into two patches would be fine.

agreed, there's a lesser chance to regress if keeping/landing those separately, and also a bit easier to review. However if you think it should be fine, then I can add the changes on top of this patch

I think they are coupled together enough that we can shoot for a single patch - it may highlight issues with the base class that we aren't noticing with just the insecure password loaded. I'd say give it a try and see if anything breaks.

(Reporter)

Updated

24 days ago
Status: NEW → ASSIGNED
(Assignee)

Comment 32

24 days ago

Created attachment 9038802 [details]
1519486 - convert autocomplete-richlistitem-insecure-field XBL binding to CE, r=bgrins

(Assignee)

Comment 33

24 days ago

Created attachment 9038803 [details]
1519486 - convert autocomplete-richlistitem XBL binding to CE, r=bgrins

(Assignee)

Comment 34

24 days ago

(In reply to Brian Grinstead [:bgrins] from comment #30)

I think they are coupled together enough that we can shoot for a single patch - it may highlight issues with the base class that we aren't noticing with just the insecure password loaded. I'd say give it a try and see if anything breaks.

I filed them as two separate patches, one of top each other, just to stay on safe side to avoid any crazy spagetti-code regressions. If you prefer them squashed for review, then let me know, I'll file a new version.

(Reporter)

Updated

24 days ago
Attachment #9038600 - Attachment is obsolete: true
Attachment #9038600 - Flags: review?(bgrinstead)
(Assignee)

Comment 35

24 days ago

(In reply to Brian Grinstead [:bgrins] from comment #27)

It's not obvious to me just scanning that function, but I think we will
always want to set this if the style isn't "insecureWarning", so sojmething
like:

let options = style == "insecureWarning" ?
{ is: "autocomplete-richlistitem-insecure-warning" } :
{ is: "autocomplete-richlistitem" };

it turns out that it has a conflict with other set of bindigns autocomplete-profile-listitem

(Assignee)

Comment 36

24 days ago

Created attachment 9038845 [details]
1519486 - convert autocomplete-richlistitem-insecure-field XBL binding to CE, r=bgrins

Attachment #9038845 - Attachment is obsolete: true
(Assignee)

Comment 37

24 days ago

having test failures with expected and unexpected reflow

browser/base/content/test/performance/browser_urlbar_search.js
FAIL Unused expected reflow at _handleOverflow@chrome://global/content/bindings/autocomplete.xml:
This is probably a good thing - just remove it from the whitelist. - false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/head.js :: reportUnexpectedReflows :: line 174

Indeed autocomplete.xml is not longer used for autocomplete-richlistitems so no failures. I guess I can follow the advise to fix the problem.

But also it has multiple opposite failures for autocomplete.js, in a number which seems outcomes the number of autocomplete.xml failures:

FAIL unexpected reflow at _handleOverflow@chrome://global/content/elements/autocomplete-richlistitem.js hit 1 times

(Reporter)

Comment 38

24 days ago

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

having test failures with expected and unexpected reflow

browser/base/content/test/performance/browser_urlbar_search.js
FAIL Unused expected reflow at _handleOverflow@chrome://global/content/bindings/autocomplete.xml:
This is probably a good thing - just remove it from the whitelist. - false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/head.js :: reportUnexpectedReflows :: line 174

Indeed autocomplete.xml is not longer used for autocomplete-richlistitems so no failures. I guess I can follow the advise to fix the problem.

But also it has multiple opposite failures for autocomplete.js, in a number which seems outcomes the number of autocomplete.xml failures:

FAIL unexpected reflow at _handleOverflow@chrome://global/content/elements/autocomplete-richlistitem.js hit 1 times

Looks like the underlying issue is still there but we are just moving around where it happens, so you can change the whitelist to switch from "bindings/autocomplete.xml" to "elements/autocomplete-richlistitem.js".

(Reporter)

Comment 39

24 days ago

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

(In reply to Brian Grinstead [:bgrins] from comment #30)

I think they are coupled together enough that we can shoot for a single patch - it may highlight issues with the base class that we aren't noticing with just the insecure password loaded. I'd say give it a try and see if anything breaks.

I filed them as two separate patches, one of top each other, just to stay on safe side to avoid any crazy spagetti-code regressions. If you prefer them squashed for review, then let me know, I'll file a new version.

I've thought about it some more, and I think we should stick with 1 patch and a part 0 that does hg cp toolkit/content/widgets/autocomplete.xml toolkit/content/widgets/autocomplete-richlistitem.js. This will let us preserve as much history / blame as possible without having an intermediate step where we remove all of the autocomplete-richlistitem binding in the copied file in part 1 and then re-add it in part 2. What do you think?

(Reporter)

Comment 40

24 days ago

Also, up to you but I usually do the hg cp and also remove everything possible in that file except for the two bindings, so that the next patch is easier to review (you see mostly moved chunks). The downside to this (and Comment 39) is that rebasing is more of a pain, but I get the sense this is pretty close to being landable so hopefully it won't require any / many rebases.

(Assignee)

Comment 41

23 days ago

Created attachment 9038894 [details]
1519486 - hg copy autocomplete.xml to autocomplete-richlistitem.js to preserve history for autocomplete-richlistitems, r=bgrins

Attachment #9038845 - Attachment is obsolete: false
(Assignee)

Comment 42

23 days ago

browser/modules/test/browser/browser_UsageTelemetry_searchbar.js fails at:

FAIL browser.engagement.navigation.searchbar must contain the 'search_suggestion' key. - false == true - JS frame :: resource://testing-common/TelemetryTestUtils.jsm :: assertKeyedScalar :: line 44

Stack trace:
resource://testing-common/TelemetryTestUtils.jsm:assertKeyedScalar:44
chrome://mochitests/content/browser/browser/modules/test/browser/browser_UsageTelemetry_searchbar.js:test_suggestion_click:292
chrome://mochikit/content/browser-test.js:Tester_execTest/<:1108
chrome://mochikit/content/browser-test.js:Tester_execTest:1099
chrome://mochikit/content/browser-test.js:nextTest/<:997
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803

Any hints/ideas how telemetry search_suggestion is related with autocomplete-richlistitems?

At one point in time the autocomplete dropdown from the search bar used the same popup as the one you're changing IIRC.

Attachment #9038802 - Attachment is obsolete: true
(Reporter)

Comment 44

23 days ago

I'm not expecting any perf regressions, but please also do a talos test just to be sure. I've been doing two pushes (one without any patches and one with), using this syntax:

./mach try fuzzy -q '!qr /-talos-e10s' --rebuild 6 --artifact

Then take the two revisions and plug them into the URL like: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=XXX&newProject=try&newRevision=XXX&framework=1&showOnlyConfident=1

(Assignee)

Comment 45

23 days ago

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

At one point in time the autocomplete dropdown from the search bar used the same popup as the one you're changing IIRC.

Could you elaborate please? I don't explicitly touch autocomplete dropdowns, only autocomplete-richlistitems, which are contained by dropdowns. If you could provide some links to code, it'd be great.

Flags: needinfo?(MattN+bmo)

They likely use the same items… did you check what kind of items the searchbox dropdown uses? https://searchfox.org/mozilla-central/source/browser/components/search/content/search.xml is for the search box and popup, I don't know off-hand what item element it uses.

Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 47

22 days ago

for the record: this.setAttribute('align', 'center') doesn't always reflected as a style (for example, when running accessible/tests/browser/events/browser_test_focus_urlbar.js test), adding -moz-box-align: center style for .autocomplete-richlistbox helps.

(Assignee)

Comment 49

16 days ago

Created attachment 9040598 [details]
Bug 1519486 - hg copy autocomplete.xml to autocomplete-richlistitem.js to preserve history for autocomplete-richlistitems, r=bgrins

(Assignee)

Comment 50

16 days ago

Created attachment 9040599 [details]
Bug 1519486 - convert autocomplete-richlistitem-insecure-field XBL binding to CE, r=bgrins

Attachment #9040598 - Attachment is obsolete: true
Attachment #9040599 - Attachment is obsolete: true

Comment 51

15 days ago
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/322610d2c338
hg copy autocomplete.xml to autocomplete-richlistitem.js to preserve history for autocomplete-richlistitems, r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a0bd44bd411
convert autocomplete-richlistitem-insecure-field XBL binding to CE, r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/e454c9f14044
convert autocomplete-richlistitem XBL binding to CE, r=MattN

Comment 52

15 days ago
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8641112b9487
remove unused autocomplete-richlistitem XBL binding

Comment 53

15 days ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 15 days ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1524839
Depends on: 1524780
(Reporter)

Updated

6 days ago
See Also: → bug 1526826
You need to log in before you can comment on or make changes to this bug.