Closed Bug 441526 Opened 14 years ago Closed 14 years ago

Implement highlightNonMatches in toolkit autocomplete

Categories

(Toolkit :: Autocomplete, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1a2

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(3 files, 4 obsolete files)

Toolkit autocomplete should implement xpfe's highlightNonMatches attribute. This is used in the autocomplete widget to bring the user's attention to the fact that an email address hasn't been matched in the address book.

TB & SM wish to keep this functionality as they move from xpfe to toolkit.
Depends on: 441530
Attached patch Proposed Fix - toolkit (obsolete) — Splinter Review
This is how I'm proposing to implement highlightNonMatches for toolkit. This patch can only be applied after the patch for bug 441530 is applied.

Includes test case extension, I wasn't sure if it was necessary to check the style as well, but I thought it wouldn't hurt to.
Attached patch Proposed sync - xpfe (obsolete) — Splinter Review
I'm proposing that we sync the xpfe with the toolkit changes.

The only thing this patch looses is highlighting when autocomplete to my domain is on. I'm not too worried about this, considering that when it is on at the moment, there is inconsistent colouring, and when a non-matching option is entered it currently doesn't have any highlighting.
Comment on attachment 326662 [details] [diff] [review]
Proposed sync - xpfe

I'll get additional reviews once I know you're happy with it Neil.
Attachment #326662 - Flags: superreview?(neil)
Attachment #326662 - Flags: review?(neil)
Comment on attachment 326662 [details] [diff] [review]
Proposed sync - xpfe

>diff --git a/xpfe/components/autocomplete/resources/content/autocomplete.css b/xpfe/components/autocomplete/resources/content/autocomplete.css
>--- a/xpfe/components/autocomplete/resources/content/autocomplete.css
>+++ b/xpfe/components/autocomplete/resources/content/autocomplete.css
>@@ -1,3 +1,7 @@
>+
>+textbox[type="autocomplete"][nomatch="true"][highlightnonmatches="true"]) {
>+  color: red;
>+}
Three issues here:
1. Extra )
2. type="autocomplete" is probably superfluous here.
2. I don't think it's correct to move the style rule into content.

By the way, I've worked out why toolkit sets nomatch on the popup.
The attribute was (and still is in suite) used by the search engine entry, to remove the top border when there are no autocomplete items. Because xpfe uses different XBL the attribute is automatically propagated to the popup. Presumably there was a time after toolkit rewrote its autocomplete but before it switched to a separate search bar when it needed to have the attribute set on the popup.
Attachment #326662 - Flags: superreview?(neil)
Attachment #326662 - Flags: review?(neil)
Attachment #326662 - Flags: review-
Priority: -- → P2
Attached patch The fix (obsolete) — Splinter Review
Revised fix. I realised what my mistake was last time - forgetting that SeaMonkey classic and Thunderbird themes also use toolkit. So I need to put this in as one patch rather than two.

This will provide us with the correct settings and a unit test.

I'll get toolkit review once I know we're happy with the xpfe parts.

Note: you'll need to apply the patch on bug 441530 if testing on cvs (though I'm not going to check that or this one in on cvs), and you may or may not need to apply it if testing on hg.
Attachment #326661 - Attachment is obsolete: true
Attachment #326662 - Attachment is obsolete: true
Attachment #329865 - Flags: superreview?(neil)
Attachment #329865 - Flags: review?(neil)
Comment on attachment 329865 [details] [diff] [review]
The fix

> textbox[type="autocomplete"] {
>   cursor: default !important;
>+}
>+
>+textbox[type="autocomplete"][nomatch="true"][highlightnonmatches="true"] {
>+  color: red;
> }
As in the toolkit themes, the [type="autocomplete"] should be unnecessary.
[In fact, I'm not actually sure why we have that cursor rule at all.]

>           this.mNeedToComplete = false;
>-          this.removeAttribute("noMatchesFound");
>           this.clearTimer();

>           this.currentSearchString = str;
>           this.resultsPopup.clearSelection();
>-          this.removeAttribute("noMatchesFound");
I managed to get into a state whereby I couldn't get rid of the red highlight but this may be an existing bug exposed by testing with invalid LDAP settings.
Attachment #329865 - Flags: superreview?(neil)
Attachment #329865 - Flags: superreview+
Attachment #329865 - Flags: review?(neil)
Attachment #329865 - Flags: review+
Updated patch (mozilla-central parts), this changes the styles as Neil suggested and updates to the latest code changes.
Attachment #329865 - Attachment is obsolete: true
Attached patch The fix (comm-central parts) (obsolete) — Splinter Review
comm-central specific changes.
Carrying forward Neil's sr. Requesting r for the mail/ changes.
Attachment #330944 - Attachment is obsolete: true
Attachment #331080 - Flags: superreview+
Attachment #331080 - Flags: review?(mkmelin+mozilla)
Comment on attachment 330943 [details] [diff] [review]
[checked in] The fix (mozilla-central parts)

Carrying forward Neil's sr for the xpfe parts, requesting review for the toolkit changes.
Attachment #330943 - Flags: superreview+
Attachment #330943 - Flags: review?(enndeakin)
Comment on attachment 330943 [details] [diff] [review]
[checked in] The fix (mozilla-central parts)

OK, but please also test the highlightNonMatches getter as well. You can just add extra checks after the getAttribute check.
Attachment #330943 - Flags: review?(enndeakin) → review+
Comment addition Calendar - the theme item will only be required for the 1.8 branch. Request review so calendar also knows what I'm doing.
Attachment #331097 - Flags: review?
Attachment #331097 - Flags: review? → review?(daniel.boelzle)
Attachment #331097 - Flags: review?(daniel.boelzle) → review+
Comment on attachment 331080 [details] [diff] [review]
[checked in] the fix (comm-central parts)

Magnus seems to be afk a bit at the moment.
Attachment #331080 - Flags: review?(mkmelin+mozilla) → review?(bienvenu)
Attachment #331080 - Flags: review?(bienvenu) → review+
mozilla-central: 16265:b6b0f1c44dc9
comm-central: 38:014093b71460

Also cross-committed to cvs for calendar.
Attachment #330943 - Attachment description: The fix (mozilla-central parts) → [checked in] The fix (mozilla-central parts)
Attachment #331080 - Attachment description: the fix (comm-central parts) → [checked in] the fix (comm-central parts)
Attachment #331097 - Attachment description: Calendar comment change → [checked in] Calendar comment change
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a2
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/7c0511017c7a
Implement highlightNonMatches in toolkit autocomplete. r/sr=Neil for xpfe parts,r=enndeakin for toolkit parts
You need to log in before you can comment on or make changes to this bug.