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

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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)
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. :-)
Gijs, what do you think?  I will add a test if you think this approach looks good
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8508389 - Flags: feedback?(gijskruitbosch+bugs)
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";
  }
}
Attachment #8508389 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
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.
Attachment #8508389 - Attachment is obsolete: true
Attachment #8508817 - Flags: review?(gavin.sharp)
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
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.
Attachment #8508817 - Attachment is obsolete: true
Attachment #8508817 - Flags: review?(gavin.sharp)
Attachment #8508865 - Flags: review?(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
Attachment #8508865 - Flags: review?(dao) → review+
Component: Theme → Autocomplete
Product: Firefox → Toolkit
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.
Attachment #8508880 - Attachment is obsolete: true
Attachment #8509070 - Flags: review+
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...
Updated comment
Attachment #8509070 - Attachment is obsolete: true
Attachment #8509076 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ace4370f715b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
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.
Flags: qe-verify-
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.
Attachment #8509076 - Flags: approval-mozilla-aurora?
Attachment #8509076 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.