1015226 - When using field-label.html.tmpl there is not a way to underline the access key letter in the field description

Status: REOPENED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[David Lawrence [:dkl]]

When working on bug 1009216 we noticed that when using field-label.html.tmpl and designating an access key, you cannot embed a <u>W</u> or other letter in the field description to show the access key.

Patch coming

Comment 1

[David Lawrence [:dkl]]

This is actually not a trivial fix as I first imagined it as it will break localization of the labels on show_bug.cgi for fields that currently use field-label.html.tmpl. I will need to rethink how this is implemented so that it still works fine for that use case.

dkl

Comment 2

[David Lawrence [:dkl]]

Attachment: 1015226_1.patch

Comment 3

[:glob ✱]

Comment on attachment 8427815 [details] [diff] [review]
1015226_1.patch

Review of attachment 8427815 [details] [diff] [review]:
-----------------------------------------------------------------

i don't think that field-descs is the correct place to define html markup for field labels.
it's loaded by a lot of files, so should be kept lean, and it's also very easy to overlook when adding new fields.

it should be possible to automatically underline the first matching letter in the label.

this removes the redundancy, and will automatically display accesskeys for fields that you've missed (eg. classification, product, component, status, resolution, ..., have access keys on search forms).

Comment 4

[David Lawrence [:dkl]]

Attachment: 1015226_2.patch

Comment 5

[:glob ✱]

Comment on attachment 8435171 [details] [diff] [review]
1015226_2.patch

Review of attachment 8435171 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/bug/field-label.html.tmpl
@@ +18,5 @@
>  [% DEFAULT tag_name = "th" %]
> +[% DEFAULT field_name = field_descs.${field.name} FILTER html %]
> +
> +[%# Underline the accesskey if defined and found in the field name %]
> +[% IF accesskey %]

as you only need to do this if the field is editable, move this within the [% IF editable %] block

@@ +20,5 @@
> +
> +[%# Underline the accesskey if defined and found in the field name %]
> +[% IF accesskey %]
> +  [% matches = field_name.match("(?i)($accesskey)") %]
> +  [% field_name = field_name.replace(matches.0, "<u>${matches.0}</u>") IF matches.0 %]

this doesn't work if there are no matches.
you have to use
[% IF matches.0 %] [% field_name = ... %] [% END %]

Comment 6

[David Lawrence [:dkl]]

Attachment: 1015226_3.patch

Comment 7

[:glob ✱]

Comment on attachment 8440833 [details] [diff] [review]
1015226_3.patch

Review of attachment 8440833 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

Comment 8

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   0898458..a9081c2  master -> master

Comment 9

[Frédéric Buclin]

Comment on attachment 8440833 [details] [diff] [review]
1015226_3.patch

>+[% DEFAULT field_name = field_descs.${field.name} FILTER html %]

This syntax is broken, see bug 1085173.


>+      [% matches = field_name.match("(?i)($accesskey)") %]
>+      [% IF matches.0 %]
>+        [% field_name = field_name.replace(matches.0, "<u>${matches.0}</u>") %]
>+      [% END %]

You must not run this code against the HTML-filtered field description, because a field description of the form "H & M" with an accesskey = a breaks the output, because & is first converted into &amp; and then you run the regexp against it, converting &amp; into &<u>a</u>mp;.

I suggest to backout this patch.

Comment 10

[David Lawrence [:dkl]]

Backed out for now:

To ssh://[e-mail]/bugzilla/bugzilla.git
   cd3b8b4..2ebb8b6  master -> master

dkl

Comment 11

[David Lawrence [:dkl]]

Attachment: 1015226_4.patch

Comment 12

[Frédéric Buclin]

Comment on attachment 8508698 [details] [diff] [review]
1015226_4.patch

>-  >[%- field_descs.${field.name} FILTER html %]:</a>
>+  >[%- field_name FILTER html_light %]:</a>

If a custom field name has HTML entities in it (which is very unlikely), it would be rendered differently, which is not good. Couldn't you use a similar trick as in bug 998323, i.e. using \x{FDD2} and \x{FDD3} to represent <u> and </u> respectively, and substitute them after calling FILTER html?