Closed Bug 1519486 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: UI Widgets, task, P3)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: bgrins, Assigned: surkov)

References

Details

(Whiteboard: [xbl-available])

Attachments

(3 files, 9 obsolete files)

1519486 - convert autocomplete-richlistitem XBL binding to CE, r=bgrins
47 bytes, text/x-phabricator-request
Details | Review
1519486 - convert autocomplete-richlistitem-insecure-field XBL binding to CE, r=bgrins
47 bytes, text/x-phabricator-request
Details | Review
1519486 - hg copy autocomplete.xml to autocomplete-richlistitem.js to preserve history for autocomplete-richlistitems, r=bgrins
47 bytes, text/x-phabricator-request
Details | Review

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
Whiteboard: [xbl-available]
Priority: -- → P3
Blocks: 1519533

A couple more notes:

Attached patch part1: insecure field CE (obsolete) — — Splinter Review

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)

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

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

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)

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

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.

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

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

Attached patch part1: insecure field CE (v2) (obsolete) — — Splinter Review
Attachment #9038230 - Attachment is obsolete: true

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

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

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

Attached patch part1: insecure field CE (v3) (obsolete) — — Splinter Review

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
asyncTester_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

(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

Attached patch part1: insecure field CE (v4) (obsolete) — — Splinter Review
Attachment #9038427 - Attachment is obsolete: true
Attachment #9038549 - Flags: review?(bgrinstead)
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)
Attached patch part1: insecure field CE (obsolete) — — Splinter Review

apparently I forgot to add new files

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

(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

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)

(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

Attached patch part1: insecure field CE (obsolete) — — Splinter Review
Attachment #9038557 - Attachment is obsolete: true
Attachment #9038600 - Flags: review?(bgrinstead)
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
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

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

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 =

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

(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

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

Status: NEW → ASSIGNED

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

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

(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

Attachment #9038845 - Attachment is obsolete: true

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

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

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

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.

Attachment #9038845 - Attachment is obsolete: false

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

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

(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)
Blocks: 1189618

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.

Attachment #9040598 - Attachment is obsolete: true
Attachment #9040599 - Attachment is obsolete: true
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
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8641112b9487
remove unused autocomplete-richlistitem XBL binding
Depends on: 1524839
Depends on: 1524780
See Also: → 1526826
Depends on: 1527562
Depends on: 1531877
Type: enhancement → task
Regressions: 1524839
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: