Closed Bug 1041959 Opened 10 years ago Closed 9 years ago

Turn all labels for input fields in user preferences into <label> elements

Categories

(Bugzilla :: User Accounts, enhancement)

Product:
Bugzilla
Component:
User Accounts
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Bugzilla 6.0

People

(Reporter: sebo, Assigned: alim94, Mentored)

References

Details

Attachments

(1 file, 3 obsolete files)

patch4.diff
4.49 KB, patch
LpSolit
: review+
Details | Diff | Splinter Review 
All input fields within the user preferences pages should have proper labels assigned to the via <label for=""> to improve the UI.

Sebastian
Whiteboard: [good first bug]
can i have this bug... (looking for first bug)
(In reply to Sushrut Bhalla from comment #1)
> can i have this bug... (looking for first bug)

Yes. Feel free to take this one and let me know if you have any questions.

dkl
Assignee: user-accounts → sushrutbhalla
Mentor: dkl
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
I'd like to work on this. Can you tell me where is this user preferences located? 
Thank you.
Flags: needinfo?(sebastianzartner)
You go to https://bugzilla.mozilla.org/userprefs.cgi (to see the preferences in this installation of Bugzilla), you could take a look at the HTML source code of that page, and you could search for the strings in your checkout of the Bugzilla codebase to see where to change the code.
Flags: needinfo?(sebastianzartner)
Note that this bug got assigned to Sushrut Bhalla about two weeks ago. So before you start you both should coordinate each other.

Sebastian
Sorry I had a lot on my plate the past few weeks. I am still working on this bug. I will have an update within the next few days.

My apologies and best regards,
Sushrut
No worries. Work on it. I'll work on some other bug.
Sushrut, are you still working on this?

Sebastian
Flags: needinfo?(sushrutbhalla)
Hello Sebastian,

Please give me a couple of days. I am sorry for the delay. I just finished my exams and now I working on this full time, as it is break time.

Thanks,

Sushrut
Flags: needinfo?(sushrutbhalla)
Hello,

I have tried install bugzilla on ubuntu 12.04 for the past 2 days but I wasn't able to.
I would like to give this bug back. I feel like I need to learn more about webserver and mysql.

Thank you,

Sushrut
Assignee: sushrutbhalla → user-accounts
I will love to work on this bug as my first...
Flags: needinfo?(sebastianzartner)
I will love to work on this bug as my first...
Sure, go ahead, I assigned this bug to you. Please, feel free to ask any questions from your friendly mentor, dkl. :) (See comment #2.)
Assignee: user-accounts → walexi4great
Flags: needinfo?(sebastianzartner)
I think am done with fixing the bug, where do I go from here?
Flags: needinfo?(dkl)
Please attach a patch here. See https://wiki.mozilla.org/Bugzilla:Developers for more information.
Flags: needinfo?(dkl)
Comment on attachment 8566672 [details] [diff] [review]
Fix for Bug 1041959 - Turn all labels for input fields in user preferences into <label> elements

r+ is reserved to official reviewers. You must ask for review instead (doing it for you). :)
Attachment #8566672 - Flags: review+ → review?(dylan)
Attachment #8566672 - Flags: review?(dylan) → review?(dkl)
Thanks
Comment on attachment 8566672 [details] [diff] [review]
Fix for Bug 1041959 - Turn all labels for input fields in user preferences into <label> elements

>+  <label for="login"></label>

Err... It doesn't make sense to add empty labels. The point is to correctly convert existing ones.
Attachment #8566672 - Flags: review?(dkl) → review-
You need to put the <label> tags around the text label itself instead of being an empty element.

For example instead of:

<tr>
  <th>New password:</th>	
  <td>
    <label for="password"></label>	
    <input type="password" name="password" id="password" required>
  </td>
</tr>

You will need to do:


<tr>
  <th><label for"password">New password:</label></th>	
  <td>
    <input type="password" name="password" id="password" required>
  </td>
</tr>

Same for all places through your patch.
Hi, I would really like to work on this bug.
(In reply to Rishab Kalra from comment #21)
> Hi, I would really like to work on this bug.

The bug is currently already assigned to walexi4great@gmail.com, which is actively working on this issue. So you either need to contact him or her to take the issue over or choose another bug to work on.

(In reply to David Lawrence [:dkl] from comment #20)
> You need to put the <label> tags around the text label itself instead of
> being an empty element.

Note that <label> tags are used to allow clicking the label to focus the form field it references.

Also, as far as I saw your patch only covers a part of the form fields and also some, which don't need a label like buttons.

To be clear on this, here's a list of pages I (as the reporter of this bug) expect to have labels for their input fields, checkboxes and drop down lists:

https://bugzilla.mozilla.org/userprefs.cgi?tab=settings
 => All drop downs under "General Preferences"

https://bugzilla.mozilla.org/userprefs.cgi?tab=email
 => "Add users to my watch list (comma separated list):" labelling <input id="new_watchedusers">
    "Add bugs:" labelling <input id="add_ignored_bugs">

https://bugzilla.mozilla.org/userprefs.cgi?tab=account
 => All input fields

https://bugzilla.mozilla.org/userprefs.cgi?tab=bugmail_filter
 => All input fields and drop downs under "Bugmail Filtering"

https://bugzilla.mozilla.org/userprefs.cgi?tab=component_watch
 => "Product:" labelling <select id="product">
    "Component:" and "Select the component(s) to add to your watch list:" labelling <select id="component">
    "Or watch components starting with:" labelling <input id="add_starting">

https://bugzilla.mozilla.org/userprefs.cgi?tab=request_nagging
 => "Add users to my watch list (comma separated list):" labelling <input id="add_watching">

Because I do not have all privileges on Bugzilla, there may be other pages and fields I do not see. So the list above might not be complete.

Furthermore the patch contains file mode changes, which should probably not happen.

Sebastian
(In reply to Sebastian Zartner [:sebo] from comment #22)
> (In reply to Rishab Kalra from comment #21)
> > Hi, I would really like to work on this bug.
> 
> The bug is currently already assigned to walexi4great@gmail.com, which is
> actively working on this issue.

walexi4great, are you still working on this?

Sebastian
Flags: needinfo?(walexi4great)
Because I didn't hear back from walexi4great, I assume he or she doesn't work on this bug anymore. So I reset the assignment of the bug to allow somebody else to take it.

Sebastian
Assignee: walexi4great → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(walexi4great)
I want to take up this bug. Let me know if any concern. Also if somebody can provide the build and evn preparation it would be very helpful.
can u assign me this. Also let me know all the information required to get me started on this.
Flags: needinfo?(dkl)
can u assign this bug to me? I would like to contribute. Also if u can provide some info/links to get me started on this.
(In reply to Mukhtar Ali from comment #27)
> can u assign this bug to me? I would like to contribute. Also if u can
> provide some info/links to get me started on this.

Yes you may work on this bug and complete the previous work if desired. Let me know if you need any assistance.

dkl
Assignee: nobody → alim94
Flags: needinfo?(dkl)
(In reply to David Lawrence [:dkl] from comment #28)
> (In reply to Mukhtar Ali from comment #27)
> > can u assign this bug to me? I would like to contribute. Also if u can
> > provide some info/links to get me started on this.
> 
> Yes you may work on this bug and complete the previous work if desired. Let
> me know if you need any assistance.
> 
> dkl

Thanks David . If u have some links on how to get started that would be helpful. I m new on this but definately want to contribute.
Flags: needinfo?(dkl)
(In reply to Mukhtar Ali from comment #29)
> If u have some links on how to get started

See https://www.bugzilla.org/contribute/ - if you have specific questions, feel free to ask.
Flags: needinfo?(dkl)
this current issue is for version 4.5.4 . 
Now current bugzilla release available is 5.0.1 . 
So can I take the latest available release and code base ?
Flags: needinfo?(dkl)
(In reply to Mukhtar Ali from comment #31)
> this current issue is for version 4.5.4 . 
> Now current bugzilla release available is 5.0.1 . 
> So can I take the latest available release and code base ?

Development must be done based on master, i.e. 5.1. This code is not available as a tarball. You must use git to download it:

  http://git.mozilla.org/?p=bugzilla/bugzilla.git
Flags: needinfo?(dkl)
(In reply to Mukhtar Ali from comment #31)
> this current issue is for version 4.5.4 . 

I just filed it under 4.5.4, because there was no better version to select when I created the report. I now changed it to 'unspecified', because this enhancement request doesn't relate to a specific version of Bugzilla.

Sebastian
Version: 4.5.4 → unspecified
Also, please note that comment 22 suggests how this should be implemented.

Sebastian
THanks Seabastian for the clarification.

I now have installed Git and Active Perl on my system.
I also downloaded the git master code base using "git clone https://git.mozilla.org/bugzilla/bugzilla"
Besides that do i need any thing else for the build  ?
Flags: needinfo?(dkl)
These are general setup questions unrelated to this bug report. 
Please refer to http://bugzilla.readthedocs.org/en/5.0/index.html - thanks.
Flags: needinfo?(dkl)
Thanks Andre ..
My bugzilla setup doesn't have the following Preference TABS : 
1. Request Nagging , 
2. BugMail Filter and 
3. Component Watch.

I need to make changes to these as well.  Can u help if I m missing some configuration ?
Flags: needinfo?(dkl)
Or is it that 5.0 + version dont have these tabs anymore . ?
Mukhtar: I suggest you come on IRC to ask your question, or use the support mailing-list for installation and configuration questions. Every comment posted in this bug triggers tens of emails sent to various developers and users. The discussion in this bug should stay focused on development as much as possible. :)

https://www.bugzilla.org/support
Flags: needinfo?(dkl)
I have finished my development on this issue. Let me know the next steps.
(In reply to Mukhtar Ali from comment #41)
> I have finished my development on this issue. Let me know the next steps.

You will need to generate a patch of your work based on your git checkout.

https://wiki.mozilla.org/Bugzilla:Patches

Then attach the patch to this bug setting the review flag to ? asking for a review of the patch. If the reviewer finds issues with the patch, they will set the review flag to - and you will need to revise and attach again.

dkl
Attached patch patch.diff (obsolete) — — Splinter Review 
This should fix up all the fields in the preferences section.
Let me know if any thing more to be done.
Flags: needinfo?(dkl)
Attachment #8695103 - Flags: review?
Comment on attachment 8695103 [details] [diff] [review]
patch.diff

The patch introduces lots of unneeded whitespace between apostrophes and brackets, or in front of brackets, plus duplicated whitespace between parameters. The patch also includes typos: "new_passowrd1".

-          [% setting_descs.$name OR name FILTER html %]
+          <label for="[% name FILTER html %]" >
+             [% setting_descs.$name OR name FILTER html %]
+          </label>
Could you explain how you chose the for parameter for the <label> here, please?
Flags: needinfo?(dkl)
Attachment #8695103 - Flags: feedback-
(In reply to Andre Klapper from comment #44)
> Comment on attachment 8695103 [details] [diff] [review]
> patch.diff
> 
> The patch introduces lots of unneeded whitespace between apostrophes and
> brackets, or in front of brackets, plus duplicated whitespace between
I can remove the extra whitespace if that is an issue.

> parameters. The patch also includes typos: "new_passowrd1".
This is not typo. This is what the input field name is. ( i.e. While changing password , user has to enter new password (new_password1) and then user has to reconfirm the password @ _new_password2 )

> 
> -          [% setting_descs.$name OR name FILTER html %]
> +          <label for="[% name FILTER html %]" >
> +             [% setting_descs.$name OR name FILTER html %]
> +          </label>
> Could you explain how you chose the for parameter for the <label> here,
> please?

the drop down field name is  "<select name="[% name FILTER html %]" id="[% name FILTER html %]"  ".
And it same for both the IF and ELSE input fields
Flags: needinfo?(dkl)
(I don't understand why you set a needinfo request against dkl in your last comment.)(In reply to Mukhtar Ali from comment #45)
> I can remove the extra whitespace if that is an issue.

Yes please. The code style should be the same as the surrounding code.

> This is not typo.

Yes it clearly is. Please read carefully.
Flags: needinfo?(dkl)
Attached patch patch_2.diff (obsolete) — — Splinter Review 
All issues report fixed.
Attachment #8695103 - Attachment is obsolete: true
Attachment #8695103 - Flags: review?
Attachment #8695288 - Flags: review?
The new patch still has the typo 'new_passowrd1' as ID ('o' and 'w' are switched).

Sebastian
Attached patch patch4.diff — — Splinter Review 
removed typos.
Attachment #8695288 - Attachment is obsolete: true
Attachment #8695288 - Flags: review?
Attachment #8695300 - Flags: review?
Let me know if any changes required on the patch ?
Flags: needinfo?(dkl)
Do not use the needinfo flag to ask for review. Simply set the requestee accordingly.
Status: NEW → ASSIGNED
Flags: needinfo?(dkl)
Attachment #8695300 - Flags: review? → review?(dkl)
Attachment #8566672 - Attachment is obsolete: true
Comment on attachment 8695300 [details] [diff] [review]
patch4.diff

>+++ b/template/en/default/account/prefs/account.html.tmpl

>+          <label for="new_login_name">
>           [% IF Param('emailsuffix') %]
>             New login:
>           [% ELSE %]
>             New email address:
>           [% END %]
>+          </label>

The indentation should be fixed.



>+++ b/template/en/default/account/prefs/apikey.html.tmpl

>+  <label for="new_description">Generate a new API key with optional description</label>

This line is a bit too long (should not exceed 80 characters, ideally). I suggest that this label points to the checkbox instead of the text field. Or even better, split this text into two labels: "Generate a new API key" and "optional description".



>+++ b/template/en/default/account/prefs/email.html.tmpl

>+  <p><label for="watched_by_you">You are watching everyone in the following list:</label></p>
>+<p id="new_watched_by_you"><label for="new_watchedusers">Add users to my watch list (comma separated list):</label>

Both lines are too long and should be split.


>-  Add [% terms.bugs %]:<br>
>+  Add <label for="add_ignored_bugs">[% terms.bugs %]:</label><br>

I think <br> should go away, for consistency with the User Watching section.



>+++ b/template/en/default/account/prefs/settings.html.tmpl

>+          <label for="[% name FILTER html %]">
>+             [% setting_descs.$name OR name FILTER html %]
>+          </label>

In templates, the indentation is 2 whitespaces, not 3.


All these comments can be fixed on checkin, so no need to upload a new one.

Thanks for your patch, Mukhtar! :) r=LpSolit
Attachment #8695300 - Flags: review?(dkl) → review+
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   cb36753..e6ce904  master -> master
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]
Target Milestone: --- → Bugzilla 6.0
Shouldn't these changes already be available at https://landfill.bugzilla.org/bugzilla-tip/? Or is that page only updated manually?

Sebastian
Flags: needinfo?(LpSolit)
(In reply to Sebastian Zartner [:sebo] from comment #54)
> Shouldn't these changes already be available at
> https://landfill.bugzilla.org/bugzilla-tip/? Or is that page only updated
> manually?

They should be there automatically but unfortunately our automation has some problems due to switching to git. I've brought it up to date now.
Flags: needinfo?(LpSolit)
(In reply to Teemu Mannermaa (:wicked) from comment #55)
> (In reply to Sebastian Zartner [:sebo] from comment #54)
> > Shouldn't these changes already be available at
> > https://landfill.bugzilla.org/bugzilla-tip/? Or is that page only updated
> > manually?
> 
> They should be there automatically but unfortunately our automation has some
> problems due to switching to git. I've brought it up to date now.

Thank you! Now I could check it and it looks all very good to me.

Unfortunately some tabs mentioned in comment 22 are not available there. Are they BMO specific or are they just not enabled on Landfill?

Sebastian
Status: RESOLVED → VERIFIED
Flags: needinfo?(wicked)
(In reply to Sebastian Zartner [:sebo] from comment #56)
> Unfortunately some tabs mentioned in comment 22 are not available there. Are
> they BMO specific or are they just not enabled on Landfill?

Currently at least component watching, bugmail filter and request nagging are BMO only extensions.
Flags: needinfo?(wicked)
See Also: → 1232397
See Also: → 1232398
See Also: → 1232402
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: