Implement additional toolkit features in XPFE autocomplete widget

RESOLVED FIXED in seamonkey2.24

Status

SeaMonkey
Autocomplete
RESOLVED FIXED
5 years ago
6 months ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

unspecified
seamonkey2.24

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Now that the XPFE autocomplete widget doesn't have to support the XPFE interfaces we can implement some additional toolkit features.
(Assignee)

Comment 1

5 years ago
Created attachment 792479 [details] [diff] [review]
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.
Attachment #792479 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 2

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

Comment 3

5 years ago
Created attachment 792487 [details] [diff] [review]
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!
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #792487 - Flags: review?(iann_bugzilla)
Attachment #792487 - Flags: feedback?(philip.chee)
(Assignee)

Comment 4

5 years ago
(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.

Updated

5 years ago
Attachment #792487 - Flags: feedback?(philip.chee) → feedback+

Comment 5

5 years ago
> +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");
(Assignee)

Comment 6

5 years ago
(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

5 years ago
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
Attachment #792487 - Flags: review?(iann_bugzilla)
Attachment #792487 - Flags: review+

Comment 8

5 years ago
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?
Attachment #792479 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 9

5 years ago
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...
Attachment #792479 - Flags: feedback?(stefanh)

Comment 10

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

Comment 11

5 years ago
(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

5 years ago
(In reply to neil@parkwaycc.co.uk 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

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

Comment 14

5 years ago
Created attachment 798631 [details] [diff] [review]
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.
Attachment #792487 - Attachment is obsolete: true
Attachment #798631 - Flags: review?(iann_bugzilla)
Attachment #798631 - Flags: feedback?(philip.chee)

Updated

5 years ago
Attachment #792479 - Flags: feedback?(stefanh) → feedback+

Comment 15

5 years ago
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).
Attachment #798631 - Flags: feedback+
(Assignee)

Comment 16

5 years ago
(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

5 years ago
(In reply to neil@parkwaycc.co.uk 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

5 years ago
Created attachment 803101 [details]
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

5 years ago
Comment on attachment 798631 [details] [diff] [review]
Fixed patch

This patch WORKSFORME a=me
Attachment #798631 - Flags: feedback?(philip.chee) → feedback+
(Assignee)

Comment 20

5 years ago
(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

5 years ago
> 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

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

Comment 23

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

Comment 24

5 years ago
Created attachment 812594 [details] [diff] [review]
With contentious hunk removed
Attachment #812594 - Flags: review?(iann_bugzilla)

Updated

5 years ago
Attachment #812594 - Flags: review?(iann_bugzilla) → review+

Updated

5 years ago
Attachment #798631 - Flags: review?(iann_bugzilla)
https://hg.mozilla.org/mozilla-central/rev/230e0dbeff0d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.24

Comment 27

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

Comment 28

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

6 months ago
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/c4c79dfa59f3
Implement additional toolkit features in the XPFE autocomplete widget r=IanN
You need to log in before you can comment on or make changes to this bug.