Merge lp:~wallyworld/launchpad/better-popup-show-widget-name into lp:launchpad

[Ian Booth]

[r=sinzui][bug=787389] Better generation of unique field names when they contain invalid html id characters

The fix for bug 777766 breaks existing Windmill tests and also makes new tests extremely hard to write because the show_widget_id attribute is no longer easily discoverable from the field name. The fix uses a base64 encoding to convert the field to non-human readable text.

The point of the fix was to remove invalid characters from the field name so that a valid HTML node id could be generated from the field name. This needs to be done another way so that developers can still relate the generated show_widget_id value back to the associated field name. Otherwise windmill tests which rely on this known mapping break.

The existing fix also didn't totally solve the problem. The input_id property of the widget which is used to construct the node for the text field didn't have the invalid characters removed.

== Implementation ==

The field names on a page/form are unique and the node names are based on these so that aspect is covered already. The invalid HTML characters are simply removed using a regexp substitution. In the process, an unnecessary property "suffix" on VocabularyPickerWidget was able to be removed.

So in summary, there are 2 nodes rendered for a VocabularyPickerWidget widget:
1. the text input field, which has id = widget.input_id
2. the Choose... anchor, which has id = "show-widget-" + widget.input_id

where widget.input_id is widget.name with the invalid chars stripped out.

== Tests ==

The TestVocabularyPickerWidget test case was modified to render a widget with an invalid field name. A custom metaclass for the form schema interface was used to do this.

Also, the failing Windmill test test_person_picker_widget was run successfully.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/widgets/popup.py
  lib/lp/app/widgets/tests/test_popup.py

[Curtis Hovey (sinzui)]

We solved this problem a few years ago.

    from lp.app.browser.stringformatter import FormattersAPI
    valid_content_id_or_class = FormattersAPI(self.context.name).css_id()

You may have several widgets for a field and there are two ways to make a valid class or id:
    widget_id = '%s-edit' % valid_content_id_or_class
or
    widget_id = FormattersAPI('%s-edit' % valid_content_id_or_class).css_id()

review: Needs Fixing (code)

[Ian Booth (wallyworld)]

> We solved this problem a few years ago.
>
> from lp.app.browser.stringformatter import FormattersAPI
> valid_content_id_or_class = FormattersAPI(self.context.name).css_id()
>
> You may have several widgets for a field and there are two ways to make a
> valid class or id:
> widget_id = '%s-edit' % valid_content_id_or_class
> or
> widget_id = FormattersAPI('%s-edit' % valid_content_id_or_class).css_id()

The problem with the above approach is that it doesn't ensure uniqueness. If all invalid chars are simply converted to '-', then if we have fields named like so:
field.foo+
field.foo^

then css_id() will convert both to field.foo-

The approach used in the latest version of this mp is to extract the portion of the field name after the 'field.' bit and base64 encode that, retaining the 'field.' prefix. If this is considered valid, perhaps I should implement this approach in the FormattersAPI.css_id() method.

Thoughts?

[Curtis Hovey (sinzui)]

There were no cases in the past were we could have collisions are described. Most of the fields we use in control names are based on lp name fields. Is there an example of a field where this can happen?

If this really can happen, then I propose that the method take an optional arg (tales does not need this) that appends the base64 encoding so that we have a human-readable fragment that can be debugged.

Most importantly, I do not think we want two implementations. A smarter function may be all that we need.

[Ian Booth (wallyworld)]

On 26/05/11 00:08, Curtis Hovey wrote:
> There were no cases in the past were we could have collisions are described. Most of the fields we use in control names are based on lp name fields. Is there an example of a field where this can happen?
>

I made the above argument on irc but was assured there is a remote
chance a collision could happen. This page:

https://bugs.launchpad.net/ubuntu/+source/gtk+2.0/+bug/713922

has a dom node which is named after the project gtk+2.0 and the argument
made to me was that since such cases involve not just lp name fields
with statically unique names but names derived from user data, there is
a remote chance there may be a collision at some point in the future and
we shouldn't put in code which is ok now but may break later. I still
don't see how it could happen but deferred to those with more lp knowledge.

> If this really can happen, then I propose that the method take an optional arg (tales does not need this) that appends the base64 encoding so that we have a human-readable fragment that can be debugged.
>
> Most importantly, I do not think we want two implementations. A smarter function may be all that we need.

Agree with the above. My suggestion was that the existing formatter be
enhanced if deemed necessary.

[Ian Booth (wallyworld)]

I have modified the VocabularyPickerWidget to use FormattersAPI.css_id() to generate its input_id value.

The implementation of css_id has been changed:
1. '.' are considered valid id characters and will no longer be replaced by '-'. This is to allow ids like 'field.foo' to be rendered as is and not changed to 'field-foo'
2. If any invalid characters are found in the id and are replaced by '-', then the base64 encoding of the id name is appended to ensure uniqueness.

Unit tests have been added for css_id (there were none).

The only place that used css_id before this was the +review-licences page for projects. I checked that this page still works as expected.

[Curtis Hovey (sinzui)]

dots are not valid in CSS3 Ids, that is why YUI3 hates them and one of the reasons we created css_id().

[Ian Booth (wallyworld)]

And yet almost all of our pages have nodes with id and often name equal
to "field.xxx" and the only page which uses css_id() is the
+review-licences page. I'll revert the acceptance of "." but we should
consider fixing the rest of the system at some point.

On 26/05/11 14:33, Curtis Hovey wrote:
> dots are not valid in CSS3 Ids, that is why YUI3 hates them and one of the reasons we created css_id().

[Curtis Hovey (sinzui)]

On Thu, 2011-05-26 at 11:39 +0000, Ian Booth wrote:
> And yet almost all of our pages have nodes with id and often name
> equal
> to "field.xxx"

That is Zope.

> and the only page which uses css_id() is the
> +review-licences page. I'll revert the acceptance of "." but we should
> consider fixing the rest of the system at some point.

Many pages use the fmt:css-id.

--
__Curtis C. Hovey_________
http://launchpad.net/

[Curtis Hovey (sinzui)]

This looks good to land.

review: Approve (code)

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2011-02-02 00:58:03 +0000
+++ lib/lp/app/browser/stringformatter.py	2011-05-29 23:36:46 +0000
@@ -2,6 +2,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """TALES formatter for strings."""
+from base64 import urlsafe_b64encode
 
 __metaclass__ = type
 __all__ = [
@@ -119,7 +120,7 @@
         if nchars >= maxlen:
             # stop if we've reached the maximum chunk size
             break
-        if nchars >= minlen and not word[endpos-1].isalnum():
+        if nchars >= minlen and not word[endpos - 1].isalnum():
             # stop if we've reached the minimum chunk size and the last
             # character wasn't alphanumeric.
             break
@@ -216,7 +217,7 @@
         If there are opening parens in the url that are matched by closing
         parens at the start of the trailer, those closing parens should be
         part of the url."""
-        assert trailers != '', ( "Trailers must not be an empty string.")
+        assert trailers != '', ("Trailers must not be an empty string.")
         opencount = url.count('(')
         closedcount = url.count(')')
         missing = opencount - closedcount
@@ -791,7 +792,7 @@
     def shorten(self, maxlength):
         """Use like tal:content="context/foo/fmt:shorten/60"."""
         if len(self._stringtoformat) > maxlength:
-            return '%s...' % self._stringtoformat[:maxlength-3]
+            return '%s...' % self._stringtoformat[:maxlength - 3]
         else:
             return self._stringtoformat
 
@@ -817,7 +818,7 @@
         header_next = False
         for row, line in enumerate(text.splitlines()[:max_format_lines]):
             result.append('<tr>')
-            result.append('<td class="line-no">%s</td>' % (row+1))
+            result.append('<td class="line-no">%s</td>' % (row + 1))
             if line.startswith('==='):
                 css_class = 'diff-file text'
                 header_next = True
@@ -850,13 +851,26 @@
         result.append('</table>')
         return ''.join(result)
 
-    _css_id_strip_pattern = re.compile(r'[^a-zA-Z0-9-]+')
+    _css_id_strip_pattern = re.compile(r'[^a-zA-Z0-9-_]+')
+    _zope_css_id_strip_pattern = re.compile(r'[^a-zA-Z0-9-_\.]+')
 
     def css_id(self, prefix=None):
+        """Return a CSS compliant id."""
+        return self._css_id(self._css_id_strip_pattern, prefix)
+
+    def zope_css_id(self, prefix=None):
+        """Return a CSS compliant id compatible with zope's form fields.
+
+        The strip pattern allows ids which contain periods which is required
+        for compatibility with zope form fields.
+        """
+        return self._css_id(self._zope_css_id_strip_pattern, prefix)
+
+    def _css_id(self, strip_pattern, prefix=None):
         """Return a CSS compliant id.
 
-        The id may contain letters, numbers, and hyphens. The first
-        character must be a letter. Unsupported characters are converted
+        The id may contain letters, numbers, hyphens and underscores. The
+        first character must be a letter. Unsupported characters are converted
         to hyphens. Multiple characters are replaced by a single hyphen. The
         letter 'j' will start the id if the string's first character is not a
         letter.
@@ -868,8 +882,25 @@
             raw_text = prefix + self._stringtoformat
         else:
             raw_text = self._stringtoformat
-        id_ = self._css_id_strip_pattern.sub('-', raw_text)
-        if id_[0] in '-0123456789':
+        id_ = strip_pattern.sub('-', raw_text)
+
+        # If any characters are converted to a hyphen, we cannot be 100%
+        # assured of a unique id unless we take further action. Note that we
+        # use the _zope_css_id_strip_pattern for the check because ids with
+        # periods are still in common usage.
+        if self._zope_css_id_strip_pattern.search(raw_text):
+            # We need to ensure that the id is always guaranteed to be unique,
+            # hence we append URL-safe base 64 encoding of the name. However
+            # we also have to strip off any padding characters ("=") because
+            # Python's URL-safe base 64 encoding includes those and they
+            # aren't allowed in IDs either.
+            unique_suffix = urlsafe_b64encode(raw_text)
+            # Ensure we put a '-' between the id and base 64 encoding.
+            if id_[-1] != '-':
+                id_ += '-'
+            id_ += unique_suffix.replace('=', '')
+
+        if id_[0] in '-_0123456789':
             # 'j' is least common starting character in technical usage;
             # engineers love 'z', 'q', and 'y'.
             return 'j' + id_
@@ -926,6 +957,11 @@
                 return self.css_id(furtherPath.pop())
             else:
                 return self.css_id()
+        elif name == 'zope-css-id':
+            if len(furtherPath) > 0:
+                return self.zope_css_id(furtherPath.pop())
+            else:
+                return self.zope_css_id()
         elif name == 'oops-id':
             return self.oops_id()
         else:

=== modified file 'lib/lp/app/doc/tales.txt'
--- lib/lp/app/doc/tales.txt	2011-03-17 03:03:33 +0000
+++ lib/lp/app/doc/tales.txt	2011-05-29 23:36:46 +0000
@@ -1033,29 +1033,39 @@
 -------
 
 Strings can be converted to valid CSS ids. The id will start with 'j' if
-the start of the string is not a letter.
+the start of the string is not a letter. If any invalid characters are
+stripped out, to ensure the id is unique, a base64 encoding is appended to the
+id.
 
     >>> test_tales('foo/fmt:css-id', foo='beta2-milestone')
     'beta2-milestone'
 
     >>> test_tales('foo/fmt:css-id', foo='user name')
-    'user-name'
+    'user-name-dXNlciBuYW1l'
 
     >>> test_tales('foo/fmt:css-id', foo='1.0.1_series')
-    'j1-0-1-series'
+    'j1-0-1_series'
 
 An optional prefix for the if can be added to the path. It too will be
 escaped.
 
     >>> test_tales('foo/fmt:css-id/series-', foo='1.0.1_series')
-    'series-1-0-1-series'
+    'series-1-0-1_series'
 
     >>> test_tales('foo/fmt:css-id/series_', foo='1.0.1_series')
-    'series-1-0-1-series'
+    'series_1-0-1_series'
 
     >>> test_tales('foo/fmt:css-id/0series-', foo='1.0.1_series')
-    'j0series-1-0-1-series'
-
+    'j0series-1-0-1_series'
+
+Zope fields are rendered with a period, and we need to ensure there is a way
+to retain the periods in the css id even though we would prefer not to.
+
+    >>> test_tales('foo/fmt:zope-css-id', foo='field.bug.target')
+    'field.bug.target'
+
+    >>> test_tales('foo/fmt:zope-css-id', foo='field.gtk+_package')
+    'field.gtk-_package-ZmllbGQuZ3RrK19wYWNrYWdl'
 
 The fmt: namespace to get strings (obfuscation)
 -----------------------------------------------

=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py	2011-05-05 15:58:47 +0000
+++ lib/lp/app/widgets/popup.py	2011-05-29 23:36:46 +0000
@@ -7,9 +7,9 @@
 
 __metaclass__ = type
 
-from base64 import urlsafe_b64encode
 import cgi
 
+import re
 import simplejson
 from z3c.ptcompat import ViewPageTemplateFile
 from zope.app.form.browser.itemswidgets import (
@@ -19,6 +19,7 @@
 from zope.schema.interfaces import IChoice
 
 from canonical.launchpad.webapp import canonical_url
+from lp.app.browser.stringformatter import FormattersAPI
 from lp.services.propertycache import cachedproperty
 
 
@@ -80,7 +81,7 @@
     def inputField(self):
         d = {
             'formToken': cgi.escape(self.formToken, quote=True),
-            'name': self.name,
+            'name': self.input_id,
             'displayWidth': self.displayWidth,
             'displayMaxWidth': self.displayMaxWidth,
             'onKeyPress': self.onKeyPress,
@@ -94,19 +95,8 @@
                          class="%(cssClass)s" />""" % d
 
     @property
-    def suffix(self):
-        """This is used to uniquify the widget ID to avoid ID collisions."""
-        # Since this will be used in an HTML ID, the allowable set of
-        # characters is smaller than the set that can appear in self.name.
-        # Therefore we use the URL-safe base 64 encoding of the name.  However
-        # we also have to strip off any padding characters ("=") because
-        # Python's URL-safe base 64 encoding includes those and they aren't
-        # allowed in IDs either.
-        return urlsafe_b64encode(self.name).replace('=', '')
-
-    @property
     def show_widget_id(self):
-        return 'show-widget-%s' % self.suffix
+        return 'show-widget-%s' % self.input_id.replace('.', '-')
 
     @property
     def extra_no_results_message(self):
@@ -144,7 +134,8 @@
 
     @property
     def input_id(self):
-        return self.name
+        """This is used to ensure the widget id contains only valid chars."""
+        return FormattersAPI(self.name).zope_css_id()
 
     def chooseLink(self):
         if self.nonajax_uri is None:

=== modified file 'lib/lp/app/widgets/tests/test_popup.py'
--- lib/lp/app/widgets/tests/test_popup.py	2011-05-05 15:58:47 +0000
+++ lib/lp/app/widgets/tests/test_popup.py	2011-05-29 23:36:46 +0000
@@ -4,31 +4,58 @@
 __metaclass__ = type
 
 import simplejson
+
+from zope.interface import Interface
+from zope.interface.interface import InterfaceClass
+from zope.schema import Choice
 from zope.schema.vocabulary import getVocabularyRegistry
 
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.app.widgets.popup import VocabularyPickerWidget
-from lp.registry.interfaces.person import ITeam
 from lp.testing import TestCaseWithFactory
 
 
+class TestMetaClass(InterfaceClass):
+# We want to force creation of a field with an invalid HTML id.
+    def __init__(self, name, bases=(), attrs=None, __doc__=None,
+                 __module__=None):
+        attrs = {
+            "test_invalid_chars+":
+            Choice(vocabulary='ValidTeamOwner'),
+            "test_valid.item":
+            Choice(vocabulary='ValidTeamOwner')}
+        super(TestMetaClass, self).__init__(
+            name, bases=bases, attrs=attrs, __doc__=__doc__,
+            __module__=__module__)
+
+
+class ITest(Interface):
+# The schema class for the widget we will test.
+    __metaclass__ = TestMetaClass
+
+
 class TestVocabularyPickerWidget(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
         super(TestVocabularyPickerWidget, self).setUp()
-        context = self.factory.makeTeam()
-        field = ITeam['teamowner']
-        self.bound_field = field.bind(context)
+        self.context = self.factory.makeTeam()
         vocabulary_registry = getVocabularyRegistry()
-        self.vocabulary = vocabulary_registry.get(context, 'ValidTeamOwner')
+        self.vocabulary = vocabulary_registry.get(
+            self.context, 'ValidTeamOwner')
         self.request = LaunchpadTestRequest()
 
     def test_widget_template_properties(self):
+        # Check the picker widget is correctly set up for a field which has a
+        # name containing only valid HTML ID characters.
+
+        field = ITest['test_valid.item']
+        bound_field = field.bind(self.context)
         picker_widget = VocabularyPickerWidget(
-            self.bound_field, self.vocabulary, self.request)
+            bound_field, self.vocabulary, self.request)
+
         self.assertEqual(
             'ValidTeamOwner', picker_widget.vocabulary_name)
         self.assertEqual(
@@ -37,12 +64,27 @@
         self.assertEqual(
             simplejson.dumps(self.vocabulary.step_title),
             picker_widget.step_title_text)
-        # The widget name is encoded to get the widget's ID.  The content of
-        # the ID is unimportant, the fact that it is unique on the page and a
-        # valid HTML element ID are what's important.
-        self.assertEqual(
-            'show-widget-ZmllbGQudGVhbW93bmVy', picker_widget.show_widget_id)
-        self.assertEqual(
-            'field.teamowner', picker_widget.input_id)
+        self.assertEqual(
+            'show-widget-field-test_valid-item', picker_widget.show_widget_id)
+        self.assertEqual(
+            'field.test_valid.item', picker_widget.input_id)
         self.assertEqual(
             simplejson.dumps(None), picker_widget.extra_no_results_message)
+
+    def test_widget_fieldname_with_invalid_html_chars(self):
+        # Check the picker widget is correctly set up for a field which has a
+        # name containing some invalid HTML ID characters.
+
+        field = ITest['test_invalid_chars+']
+        bound_field = field.bind(self.context)
+        picker_widget = VocabularyPickerWidget(
+            bound_field, self.vocabulary, self.request)
+
+        # The widget name is encoded to get the widget's ID. It must only
+        # contain valid HTML characters.
+        self.assertEqual(
+            'show-widget-field-test_invalid_chars-ZmllbGQudGVzdF9pbnZhbGlkX2NoYXJzKw',
+            picker_widget.show_widget_id)
+        self.assertEqual(
+            'field.test_invalid_chars-ZmllbGQudGVzdF9pbnZhbGlkX2NoYXJzKw',
+            picker_widget.input_id)