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

VERIFIED FIXED in Bugzilla 6.0

Status

()

Bugzilla
User Accounts
--
enhancement
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: sebo, Assigned: Mukhtar Ali, Mentored)

Tracking

unspecified
Bugzilla 6.0

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
All input fields within the user preferences pages should have proper labels assigned to the via <label for=""> to improve the UI.

Sebastian

Updated

4 years ago
Whiteboard: [good first bug]

Comment 1

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

Comment 3

4 years ago
I'd like to work on this. Can you tell me where is this user preferences located? 
Thank you.
Flags: needinfo?(sebastianzartner)

Comment 4

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

Comment 5

4 years ago
Note that this bug got assigned to Sushrut Bhalla about two weeks ago. So before you start you both should coordinate each other.

Sebastian

Comment 6

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

Comment 7

4 years ago
No worries. Work on it. I'll work on some other bug.
(Reporter)

Comment 8

4 years ago
Sushrut, are you still working on this?

Sebastian
Flags: needinfo?(sushrutbhalla)

Comment 9

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

Comment 10

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

Updated

4 years ago
Assignee: sushrutbhalla → user-accounts

Comment 11

3 years ago
I will love to work on this bug as my first...
Flags: needinfo?(sebastianzartner)

Comment 12

3 years ago
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)

Comment 14

3 years ago
I think am done with fixing the bug, where do I go from here?
Flags: needinfo?(dkl)

Comment 15

3 years ago
Please attach a patch here. See https://wiki.mozilla.org/Bugzilla:Developers for more information.
Flags: needinfo?(dkl)

Comment 16

3 years ago
Created attachment 8566672 [details] [diff] [review]
Fix for Bug 1041959 - Turn all labels for input fields in user preferences into <label> elements
Attachment #8566672 - Flags: review+

Comment 17

3 years ago
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)

Updated

3 years ago
Attachment #8566672 - Flags: review?(dylan) → review?(dkl)

Comment 18

3 years ago
Thanks

Comment 19

3 years ago
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.

Comment 21

3 years ago
Hi, I would really like to work on this bug.
(Reporter)

Comment 22

3 years ago
important
(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
(Reporter)

Comment 23

3 years ago
(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)
(Reporter)

Comment 24

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

Comment 25

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

Comment 26

3 years ago
can u assign me this. Also let me know all the information required to get me started on this.
Flags: needinfo?(dkl)
(Assignee)

Comment 27

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

Comment 29

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

Comment 30

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

Comment 31

3 years ago
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)

Comment 32

3 years ago
(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)
(Reporter)

Comment 33

3 years ago
(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
(Reporter)

Comment 34

3 years ago
Also, please note that comment 22 suggests how this should be implemented.

Sebastian
(Assignee)

Comment 35

3 years ago
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)

Comment 36

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

Comment 37

3 years ago
Thanks Andre ..
(Assignee)

Comment 38

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

Comment 39

3 years ago
Or is it that 5.0 + version dont have these tabs anymore . ?

Comment 40

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

Comment 41

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

Comment 43

3 years ago
Created attachment 8695103 [details] [diff] [review]
patch.diff

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 44

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

Comment 45

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

Comment 46

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

Comment 47

3 years ago
Created attachment 8695288 [details] [diff] [review]
patch_2.diff

All issues report fixed.
Attachment #8695103 - Attachment is obsolete: true
Attachment #8695103 - Flags: review?
Attachment #8695288 - Flags: review?
(Reporter)

Comment 48

3 years ago
The new patch still has the typo 'new_passowrd1' as ID ('o' and 'w' are switched).

Sebastian
(Assignee)

Comment 49

3 years ago
Created attachment 8695300 [details] [diff] [review]
patch4.diff

removed typos.
Attachment #8695288 - Attachment is obsolete: true
Attachment #8695288 - Flags: review?
Attachment #8695300 - Flags: review?
(Assignee)

Comment 50

3 years ago
Let me know if any changes required on the patch ?
Flags: needinfo?(dkl)

Comment 51

3 years ago
Do not use the needinfo flag to ask for review. Simply set the requestee accordingly.
Status: NEW → ASSIGNED
Flags: needinfo?(dkl)

Updated

3 years ago
Attachment #8695300 - Flags: review? → review?(dkl)

Updated

3 years ago
Attachment #8566672 - Attachment is obsolete: true

Comment 52

3 years ago
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+

Comment 53

3 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   cb36753..e6ce904  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]
Target Milestone: --- → Bugzilla 6.0
(Reporter)

Comment 54

3 years ago
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)
(Reporter)

Comment 56

3 years ago
(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
(Reporter)

Updated

3 years ago
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)
(Reporter)

Updated

3 years ago
See Also: → bug 1232397
(Reporter)

Updated

3 years ago
See Also: → bug 1232398
(Reporter)

Updated

3 years ago
See Also: → bug 1232402
You need to log in before you can comment on or make changes to this bug.