906920 - Implement additional toolkit features in XPFE autocomplete widget

Status: RESOLVED FIXED

(SeaMonkey :: Autocomplete, defect)

Description

[[e-mail]]

Now that the XPFE autocomplete widget doesn't have to support the XPFE interfaces we can implement some additional toolkit features.

Comment 1

[[e-mail]]

Attachment: Theme changes

The communicator changes have two purposes:
1. To leave room for the favicons to load asynchronously (see next patch)
2. To support file autocomplete on Linux (type / into the url bar)
The modern global change affects the XUL filepicker on Linux so that the autocomplete distinguishes folders from files. The classic skin already does this. You need to set the ui.allow_platform_file_picker preference to false to choose the XUL filepicker instead of the Gnome filepicker.

Comment 2

[[e-mail]]

Comment on attachment 792479 [details] [diff] [review]
Theme changes

Oh, and sorry for the weird CSS, but I wanted it to work in the URL bar, the open location dialog and DOM Inspector all at the same time.

Comment 3

[[e-mail]]

Attachment: Proposed patch

The first hunk seems to be fixing an error in my previous patch. Oops.
The second hunk:
a. Strengthens the conditions for backspacing deleting the match
b. Fixes some indentation
c. Simplifies some code
d. Adds some code to allow the delete key (Shift+Del or Shift+Bksp on Mac) to remove a value from the autocomplete results. Note that some of our autocomplete datasources don't implement this, so the code checks to see whether the value actually got removed or not.
The third hunk makes it clearer why part 2a works.
The fourth hunk switches from values to labels. I think this might be needed to make input lists work correctly, they are the only autocomplete datasource that uses them.
The fifth and last (but not least) hunk adds support for image URLs in autocomplete. This gives us favicons in urlbar autocomplete!

Comment 4

[[e-mail]]

(In reply to comment #3)
> The fourth hunk switches from values to labels. I think this might be needed
> to make input lists work correctly, they are the only autocomplete
> datasource that uses them.
Actually they already work for a different reason (although it's an XPFE popup, it's attached to a toolkit controller). But labels might get used for something else in future anyway.

Comment 5

[Philip Chee]

> +textbox[autocompletesearch="history file"] .autocomplete-treebody::-moz-tree-image(treecolAutoCompleteValue) {
> +  width: 16px;
> +  height: 16px;
> +  margin-left: 1px;
Do we have to care about RTL UI-locales?

> +textbox[autocompletesearch="history file"] .autocomplete-treebody::-moz-tree-image(treecolAutoCompleteValue, directory) {
> +  /*list-style-image: url("chrome://global/skin/filepicker/dir-closed.gif");*/
What's with this commented out line?

> +  list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-folder-closed.gif");

Comment 6

[[e-mail]]

(In reply to Philip Chee from comment #5)
> > +textbox[autocompletesearch="history file"] .autocomplete-treebody::-moz-tree-image(treecolAutoCompleteValue) {
> > +  width: 16px;
> > +  height: 16px;
> > +  margin-left: 1px;
> Do we have to care about RTL UI-locales?
No idea but URLs are always LTR.

> > +textbox[autocompletesearch="history file"] .autocomplete-treebody::-moz-tree-image(treecolAutoCompleteValue, directory) {
> > +  /*list-style-image: url("chrome://global/skin/filepicker/dir-closed.gif");*/
> What's with this commented out line?
> 
> > +  list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-folder-closed.gif");
For Classic I didn't want to use the filepicker icons because they're Linux-specific (although strictly speaking it doesn't hurt because they can only appear on Linux). For Modern I couldn't decide whether to follow the Classic skin or whether to follow the filepicker (which doesn't have a file icon). Presumably I tried it one way and then the other. I guess the bookmark icon is the default icon for pages without favicons anyway, so it works well, I just forgot to finish removing the original version ;-)

Comment 7

[Ian Neal]

Comment on attachment 792487 [details] [diff] [review]
Proposed patch

Is TB still a consumer of this part of the code?
Doesn't seem to break anything r=me

Comment 8

[Ian Neal]

Comment on attachment 792479 [details] [diff] [review]
Theme changes

r=me with commented out line removed
Do you need to get someone to check it looks ok on the Mac?

Comment 9

[[e-mail]]

Comment on attachment 792479 [details] [diff] [review]
Theme changes

Well, the default styles might suffice on Mac; these styles are really to ensure that file completions appear correctly on Linux. But just to be sure...

Comment 10

[Stefan [:stefanh]]

Comment on attachment 792479 [details] [diff] [review]
Theme changes

These changes doesn't affect Mac since you don't change anything in suite/themes/classic/mac/communicator/communicator.css. If you tell me how I can test this on Mac I can suggest (if needed) changes, though.

Comment 11

[[e-mail]]

(In reply to Stefan from comment #10)
> If you tell me how I can test this on Mac I can suggest changes

Apply the other attachment and then type into the URLbar.

Comment 12

[Stefan [:stefanh]]

(In reply to [e-mail] from comment #11)
> (In reply to Stefan from comment #10)
> > If you tell me how I can test this on Mac I can suggest changes
> 
> Apply the other attachment and then type into the URLbar.

Ah, ok. Will do (not in front of my mac atm) - might take a few days before I get to it, though.

Comment 13

[Philip Chee]

Comment on attachment 792487 [details] [diff] [review]
Proposed patch

Tue Sep 03 2013 03:06:18
Error: ReferenceError: endPoint is not defined
Source file: chrome://global/content/autocomplete.xml
Line: 1007

> - var endPoint = this.value.length;
> + if (match.startsWith(entry)) {
> 	this.setTextValue(this.value + resultValue.substr(endPoint));
> - this.mInputElt.setSelectionRange(endPoint, this.value.length);

Comment 14

[[e-mail]]

Attachment: Fixed patch

I think I decided that endPoint was unnecessary, then changed my mind and decided to remove startPt instead, and then changed my mind again and removed both and got it wrong. So I put endPoint back, I think it looks best like that.

Comment 15

[Stefan [:stefanh]]

Comment on attachment 798631 [details] [diff] [review]
Fixed patch

Turns out that with this applied, I get icons/favicons in the drop-down list (which I didn't got before).

Comment 16

[[e-mail]]

(In reply to Stefan from comment #15)
> Turns out that with this applied, I get icons/favicons in the drop-down list
> (which I didn't got before).

Yes, that is rather the point, see the end of commment #3. The reason for the request on the other attachment was in case you think any Mac theme changes were necessary.

Comment 17

[Stefan [:stefanh]]

(In reply to [e-mail] from comment #16)
> (In reply to Stefan from comment #15)
> > Turns out that with this applied, I get icons/favicons in the drop-down list
> > (which I didn't got before).
> 
> Yes, that is rather the point, see the end of commment #3. The reason for
> the request on the other attachment was in case you think any Mac theme
> changes were necessary.
Ah, sorry - I should have read the comments more carefully. Anyway, I think it looks OK :-)

Comment 18

[Philip Chee]

Attachment: Screenshot of the urlbar autocomplete drop down.

In the attachment, with the last line the text "Search Google for..." isn't aligned with the autocomplete results but ISTR this is an existing (and long standing) bug.

Comment 19

[Philip Chee]

Comment on attachment 798631 [details] [diff] [review]
Fixed patch

This patch WORKSFORME a=me

Comment 20

[[e-mail]]

(In reply to Philip Chee from comment #18)
> In the attachment, with the last line the text "Search Google for..." isn't
> aligned with the autocomplete results but ISTR this is an existing (and long
> standing) bug.

Well, it didn't before because it had an icon and the autocomplete results didn't. I could probably tweak the margins to make the search text line up with the autocomplete results, but in Classic I can't make it line up with the textbox, because that requires knowing the margins of the native border. Something might be possible in Modern but I'd have to see.

Comment 21

[Philip Chee]

> Well, it didn't before because it had an icon and the autocomplete results didn't
I meant lets land this patch ASAP and deal with alignment issues in a separate bug.

Comment 22

[Ian Neal]

Comment on attachment 798631 [details] [diff] [review]
Fixed patch

>               var resultValue = this.getSessionValueAt(aSessionName, indexToUse);
>               var match = resultValue.toLowerCase();
>               var entry = this.currentSearchString.toLowerCase();
>+              var endPoint = this.value.length;
>               this.ignoreInputEvent = true;
>-              if (match.indexOf(entry) == 0) {
>-                var endPoint = this.value.length;
>+              if (match.startsWith(entry)) {
>                 this.setTextValue(this.value + resultValue.substr(endPoint));
>-                this.mInputElt.setSelectionRange(endPoint, this.value.length);
>               } else {
>                 if (this.completeDefaultIndex) {
>                   this.setTextValue(this.value + " >> " + resultValue);
>-                  this.mInputElt.setSelectionRange(entry.length, this.value.length);
>                 } else {
>                   var postIndex = resultValue.indexOf(this.value);
>                   if (postIndex >= 0) {
>-                    var startPt = this.value.length;
>                     this.setTextValue(this.value + 
>-                                      resultValue.substr(startPt+postIndex));
>-                    this.mInputElt.setSelectionRange(startPt, this.value.length);
>+                                      resultValue.substr(endPoint + postIndex));
>                   }
>                 }
>               }
>+              this.mInputElt.setSelectionRange(endPoint, this.value.length);
Does entry.length = endpoint?
So previously if resultValue.indexOf(this.value) < 0 the setSelectionRange did not happen

Comment 23

[[e-mail]]

You're right, I should just remove that hunk completely. (I didn't notice the problem because the URLbar actually has its own override of that function.)

Comment 24

[[e-mail]]

Attachment: With contentious hunk removed

Comment 25

[[e-mail]]

http://hg.mozilla.org/comm-central/rev/b4e5e8c43e19
https://hg.mozilla.org/integration/mozilla-inbound/rev/230e0dbeff0d

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/230e0dbeff0d

Comment 27

[patrickjdempsey]

Is there a follow-up planned to get the dropmarker working?  I kept thinking this feature wasn't working because the old popup is being shown if you click the dropmarker.

Comment 28

[[e-mail]]

(In reply to patrickjdempsey from comment #27)
> Is there a follow-up planned to get the dropmarker working?  I kept thinking
> this feature wasn't working because the old popup is being shown if you
> click the dropmarker.

No, we need the old popup for the editor link dialog (to show anchors and headings in the page to link to) and we also use the old popup for our location history.

However I do intend to work on an as yet unfiled bug to make it easier for add-ons to choose which autocomplete they use.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c4c79dfa59f3
Implement additional toolkit features in the XPFE autocomplete widget r=IanN