Implement highlightNonMatches in toolkit autocomplete

RESOLVED FIXED in mozilla1.9.1a2

Status

()

Toolkit
Autocomplete
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
mozilla1.9.1a2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 441530
(Assignee)

Comment 1

9 years ago
Created attachment 326661 [details] [diff] [review]
Proposed Fix - toolkit

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

Comment 2

9 years ago
Created attachment 326662 [details] [diff] [review]
Proposed sync - xpfe

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

Comment 3

9 years ago
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 4

9 years ago
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-
(Assignee)

Updated

9 years ago
Priority: -- → P2
(Assignee)

Comment 5

9 years ago
Created attachment 329865 [details] [diff] [review]
The fix

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 6

9 years ago
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+
(Assignee)

Comment 7

9 years ago
Created attachment 330943 [details] [diff] [review]
[checked in] The fix (mozilla-central parts)

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

Comment 8

9 years ago
Created attachment 330944 [details] [diff] [review]
The fix (comm-central parts)

comm-central specific changes.
(Assignee)

Comment 9

9 years ago
Created attachment 331080 [details] [diff] [review]
[checked in] the fix (comm-central parts)

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

Comment 10

9 years ago
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+
(Assignee)

Comment 12

9 years ago
Created attachment 331097 [details] [diff] [review]
[checked in] Calendar comment change

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

Updated

9 years ago
Attachment #331097 - Flags: review? → review?(daniel.boelzle)

Updated

9 years ago
Attachment #331097 - Flags: review?(daniel.boelzle) → review+
(Assignee)

Comment 13

9 years ago
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)

Updated

9 years ago
Attachment #331080 - Flags: review?(bienvenu) → review+
(Assignee)

Comment 14

9 years ago
mozilla-central: 16265:b6b0f1c44dc9
comm-central: 38:014093b71460

Also cross-committed to cvs for calendar.
(Assignee)

Updated

9 years ago
Attachment #330943 - Attachment description: The fix (mozilla-central parts) → [checked in] The fix (mozilla-central parts)
(Assignee)

Updated

9 years ago
Attachment #331080 - Attachment description: the fix (comm-central parts) → [checked in] the fix (comm-central parts)
(Assignee)

Updated

9 years ago
Attachment #331097 - Attachment description: Calendar comment change → [checked in] Calendar comment change
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a2
You need to log in before you can comment on or make changes to this bug.