Brian Grinstead [:bgrins] Assignee
	Description • 10 years ago

I need a way to select only the autocomplete popups for the search box and the url bar for styling, while leaving all other instances alone (including those loaded for web page content)

	:Gijs (he/him)
	Comment 1 • 10 years ago

Keep in mind we need to ensure that content can't name their input fields the same as one of our UI elements and influence behaviour that way. :-)

	Brian Grinstead [:bgrins] Assignee
	Comment 2 • 10 years ago

Gijs, what do you think?  I will add a test if you think this approach looks good

	:Gijs (he/him)
	Comment 3 • 10 years ago

Comment on attachment 8508389 [details] [diff] [review]
autocomplete-popup-selector.patch

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

Approach looks fine, see comments below. For the real review, it would be best if someone who actually knows more about autocomplete could look. Try dao or gavin?

::: browser/components/search/content/search.xml
@@ +29,5 @@
>        "SuggestAutoComplete._historyLimit" (nsSearchSuggestions.js). Changing
>        one of them requires changing the other one.
>        -->
>        <xul:textbox class="searchbar-textbox"
> +                   id="searchbar-textbox"

It's bad form to add ids to xbl bindings (although admittedly we only use this here...).

::: toolkit/content/widgets/autocomplete.xml
@@ +896,5 @@
>          if (this._normalMaxRows < 0 && this.mInput) {
>            this._normalMaxRows = this.mInput.maxRows;
>          }
> +        if (this.mInput) {
> +          this.setAttribute("autocompleteInput", this.mInput.id);

Nit:
if (this.mInput && this.mInput.ownerDocument.documentURI.startsWith("chrome")) {
// The above is kind of a poor man's security check, but considering what we're doing
// isn't insanely security sensitive (just trying to avoid authors shooting themselves in the foot),
// I think it works. Maybe make the id "content" if we're not in chrome:// ?
  let bindingId = this.mInput.id;
// take care of elements with no id that are inside xbl bindings
  if (!bindingId) {
    let boundParent = document.getBindingParent(this.mInput);
    bindingId = boundParent ? boundParent.id : "unknown";
  }
}

	Brian Grinstead [:bgrins] Assignee
	Comment 4 • 10 years ago

This adds an attribute to the autocomplete popup to allow us to style it only when opened from certain inputs.  I'm not sure if the test changes are in the right place - I could put it elsewhere or add an entirely new test if that's better.

	Dão Gottwald [:dao]
	Comment 5 • 10 years ago

Comment on attachment 8508817 [details] [diff] [review]
autocomplete-popup-selector.patch

>+        // Set an ID for targetting CSS based on the input.

s/ID/attribute/

>+        if (this.mInput && this.mInput.ownerDocument.documentURI.startsWith("chrome")) {

this.mInput.ownerDocument.documentURIObject.schemeIs("chrome")

>+
>+        this.setAttribute("autocompleteinput", bindingID || "unknown");

Why "unknown"? What's wrong with setting this attribute to an empty string?

nit: remove empty line before this line, add one after it

	Brian Grinstead [:bgrins] Assignee
	Comment 6 • 10 years ago

Thanks for the notes, I've updated the patch.

> Why "unknown"? What's wrong with setting this attribute to an empty string?

An empty string seems fine, I switched to that.

	Dão Gottwald [:dao]
	Comment 7 • 10 years ago

Comment on attachment 8508865 [details] [diff] [review]
autocomplete-popup-selector.patch

>+        // Set an attribute for targetting CSS based on the input.

I'm not sure how one should read "for targetting CSS based on the input." Rephrase as "for styling the popup based on the input"?

>+        let bindingID = "";
>+        if (this.mInput && this.mInput.ownerDocument.documentURIObject.schemeIs("chrome")) {
>+          bindingID = this.mInput.id;
>+          // Take care of elements with no id that are inside xbl bindings
>+          if (!bindingID) {
>+            let boundParent = document.getBindingParent(this.mInput);
>+            if (boundParent) {
>+              bindingID = boundParent.id;
>+            }
>+          }
>+        }
>+        this.setAttribute("autocompleteinput", bindingID);

Please rename bindingID to inputID, boundParent to bindingParent, and call this.mInput.ownerDocument.getBindingParent instead of document.getBindingParent. r=me with these changes

	Brian Grinstead [:bgrins] Assignee
	Comment 8 • 10 years ago

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9d30a841e120

	Brian Grinstead [:bgrins] Assignee
	Comment 9 • 10 years ago

There was actually a case in a number of tests where mInput.ownerDocument was undefined.  This just adds a check for that before checking if it is a chrome doc.  For instance, I saw this on toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html:

JavaScript error: chrome://global/content/bindings/autocomplete.xml, line 885: TypeError: this.mInput.ownerDocument is undefined

Carrying over r+ because it was such a simple fix.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=0e81a7045242.

	Dão Gottwald [:dao]
	Comment 10 • 10 years ago

Comment on attachment 8509070 [details] [diff] [review]
autocomplete-popup-selector.patch

>+        // Set an attribute for targetting CSS based on the input.

see my previous comment...

	Brian Grinstead [:bgrins] Assignee
	Comment 11 • 10 years ago

Updated comment

	Brian Grinstead [:bgrins] Assignee
	Comment 12 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/ace4370f715b

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 10 years ago

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

	Andrei Vaida [:avaida]
	Comment 14 • 10 years ago

Since this is already covered by automated testing, I'm setting qe-verify-. Brian, if you think there's something manual QA should look at here, please let me know.

	Brian Grinstead [:bgrins] Assignee
	Comment 15 • 10 years ago

Comment on attachment 8509076 [details] [diff] [review]
autocomplete-popup-selector.patch

Approval Request Comment
[Feature/regressing bug #]: 1054353
[User impact if declined]: We need this for devedition
[Describe test coverage new/current, TBPL]: Adds a test case for the new attribute added to the autocomplete popup
[Risks and why]: It adds a new attribute on any chrome popup element opened by an autocomplete box (like the search box, url bar, or autocomplete for in-content text boxes).  It's using a rare enough attribute name that it's not likely to be targeted by any query selectors within chrome on accident though.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/32ad158dd41a