260908 - Trim leading & trailing whitespace on new form history entries

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[M. LaPlante]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10

AutoComplete should not save trailing spaces on saved data.  I just performed a
search on Amazon.com, and found I had three copies of the same one word search
option available in autocomplete...one with no trailing spaces, one with a
single trailing space, one with two trailing spaces.  Logically there is no
reason for this, and there should be only one AutoComplete option with trailing
(and potentially leading) spaces trimmed.

Reproducible: Always
Steps to Reproduce:

Comment 1

[M. LaPlante]

Attachment: Proof of Bug - Image

Comment 2

[M. LaPlante]

This also affects the search toolbar (Google, wiki, etc...).

Can't anybody confirm this for me?

Using 1.0.3 now.

Comment 3

[Ian Macfarlane]

Yes I've just experienced this as well, and it was irritating enough to file a bug.

I don't think whitespace before should be trimmed, however, as this would affect
the visual appearance of the text. However, with the trailing space, you get
multiple entries that are exactly the same.

Severity should be trival or enhancement.

Comment 4

[Gabriel Sjoberg]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050901
Firefox/1.0+ ID:2005090106

This behavior does indeed exists.  I'm not certain it should be in core instead
of an extension, but I agree the functionality would be handy (a configurable
option, perhaps?).

Confirming and reclassifying as enhancement.

Comment 5

[Justin Dolske [:Dolske]]

Yeah, we should do this.

Comment 6

[Matthew N. [:MattN]]

Attachment: Patch v.1 (with mochitests)

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 378445 [details] [diff] [review]
Patch v.1 (with mochitests)

>--- a/toolkit/components/satchel/src/nsStorageFormHistory.cpp
...
>+        value.Trim(" ", PR_FALSE, PR_TRUE);

I think this should be:

value.Trim(" \t", PR_TRUE, PR_TRUE);

Strips tabs too (afaict \r\n can't really make it in), and we should trim leading space as well as trailing.

I wonder if it's worth clobbering duplicate whitespace within the string? (eg, double spaces). There is a .CompressWhiteSpace()... I'd be a little wary of some use-case wanting to preserve such spacing, but can't think of any. In my big 27,000 form history DB, I had 290 entries with at least 2 concurrent spaces, and 7 with 3 concurrent spaces (none with more). All looked like typos or cut'n'paste duplication. Will your intrastring matching in bug 370117 care either way? Maybe this just isn't worth doing.

The thought also occurs to me that we could do a 1-time pass to strip any existing entries, but I don't think that's work the effort and risk. (Now that we expire old unused entries, they'll eventually drop from the DB naturally).

>--- a/toolkit/components/satchel/test/test_form_submission.html

>+    case 104:
>+        ok(fh.entryExists("test4", " trimTrailingSpaceOnly"), "checking saved value is trimmed on the right only");
>+        ok(!fh.entryExists("test4", "trimTrailingSpaceOnly"), "checking that value is not trimmed on both sides");
>+        ok(!fh.entryExists("test4", " trimTrailingSpaceOnly "), "checking that untrimmed value is not saved");
>+        break;

Seems unlikely that we'd manage to save multiple versions of a single input. No need for the negative checks.

Comment 8

[Matthew N. [:MattN]]

Attachment: v.2 trim leading + trailing + tabs

> I wonder if it's worth clobbering duplicate whitespace within the string? (eg,
double spaces). There is a .CompressWhiteSpace()... I'd be a little wary of
some use-case wanting to preserve such spacing, but can't think of any. In my
big 27,000 form history DB, I had 290 entries with at least 2 concurrent
spaces, and 7 with 3 concurrent spaces (none with more). All looked like typos
or cut'n'paste duplication. Will your intrastring matching in bug 370117 care
either way? Maybe this just isn't worth doing.

I don't think that compressing whitespace within the fields is a good idea.  An example use case is people who put type two spaces following a period in a sentence.  The current patch on that bug does substring matching, not word matching, and so whitespace between words would have to be the same to match.  We can use experiment with the fulltext extension to match words.

> The thought also occurs to me that we could do a 1-time pass to strip any
existing entries, but I don't think that's work the effort and risk. (Now that
we expire old unused entries, they'll eventually drop from the DB naturally).

I agree that this could be useful especially if someone uses an item in form history that had leading/trailing spaces and then submits it on form creating a new form history entry for the trimmed version, thus, creating duplicates which is the problem that this bug is trying to solve.

Comment 9

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/ec7594969ed7