1085745 - Add a way to select the autocomplete popup in CSS based on which input it is opened for

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Brian Grinstead [:bgrins]]

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)

Comment 1

[:Gijs (he/him)]

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

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: autocomplete-popup-selector.patch

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

Comment 3

[:Gijs (he/him)]

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";
  }
}

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: autocomplete-popup-selector.patch

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.

Comment 5

[Dão Gottwald [:dao]]

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

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: autocomplete-popup-selector.patch

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.

Comment 7

[Dão Gottwald [:dao]]

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

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: autocomplete-popup-selector.patch

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

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: autocomplete-popup-selector.patch

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.

Comment 10

[Dão Gottwald [:dao]]

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

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: autocomplete-popup-selector.patch

Updated comment

Comment 12

[Brian Grinstead [:bgrins]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Andrei Vaida [:avaida]]

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.

Comment 15

[Brian Grinstead [:bgrins]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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