Moodle
  1. Moodle
  2. MDL-41485

Do not use string as value for action buttons in calendar manage subscriptions page

        Details

        • Type: Bug Bug
        • Status: Closed
        • Priority: Minor Minor
        • Resolution: Fixed
        • Affects Version/s: 2.4.5, 2.5.1
        • Fix Version/s: 2.4.6, 2.5.2
        • Component/s: Calendar
        • Labels:
        • Testing Instructions:
          Hide
          1. Login as admin.
          2. click on the month in a calendar block.
          3. on the resulting page click on "manage subscriptions"
          4. Add a calendar, you can use this url http://ical.mac.com/ical/US32Holidays.ics
          5. on the resulting page, make sure the update and remove buttons are "<button>" tags and not "<input>"
          6. After the import is done, trying changing the "update" frequency and click on update. Make sure the update works.
          7. Click on "remove". Make sure it works.
          8. Repeat all above steps in two other languages besides English including one RTL like Hebrew.
          Show
          Login as admin. click on the month in a calendar block. on the resulting page click on "manage subscriptions" Add a calendar, you can use this url http://ical.mac.com/ical/US32Holidays.ics on the resulting page, make sure the update and remove buttons are "<button>" tags and not "<input>" After the import is done, trying changing the "update" frequency and click on update. Make sure the update works. Click on "remove". Make sure it works. Repeat all above steps in two other languages besides English including one RTL like Hebrew.
        • Workaround:
          Hide

          Change language back to English to manage subscriptions.

          Show
          Change language back to English to manage subscriptions.
        • Affected Branches:
          MOODLE_24_STABLE, MOODLE_25_STABLE
        • Fixed Branches:
          MOODLE_24_STABLE, MOODLE_25_STABLE
        • Pull from Repository:
        • Pull Master Branch:
          MDL-41485-master
        • Pull Master Diff URL:
        • Sprint:
          BACKEND Sprint 4

          Description

          Donot use string as value for action buttons in calendar manage subscritions page

            Gliffy Diagrams

              Issue Links

              is duplicated by

              Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-41468 Unable to remove a calendar from "Manage Calendar subscriptions" page, When in RTL mode

              • Minor - Minor loss of function where workaround is possible
              • Closed
              will help resolve

              Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-41468 Unable to remove a calendar from "Manage Calendar subscriptions" page, When in RTL mode

              • Minor - Minor loss of function where workaround is possible
              • Closed

                Activity

                Ascending order - Click to sort in descending order
                Hide
                Permalink
                Ankit Agarwal added a comment -

                Using strings as values creates a lot of issues like MDL-41468. This happens all over moodle.

                The best way to solve this imo is to use <button />. I have not seen any usages of <button> so far in Moodle. So adding a few people here from Frontend, to get more feedback on accessibility related aspects.

                If we decide to go forward with this, I will create a issue to add a class for this in the forms lib.

                Thanks

                Show
                Ankit Agarwal added a comment - Using strings as values creates a lot of issues like MDL-41468 . This happens all over moodle. The best way to solve this imo is to use <button />. I have not seen any usages of <button> so far in Moodle. So adding a few people here from Frontend, to get more feedback on accessibility related aspects. If we decide to go forward with this, I will create a issue to add a class for this in the forms lib. Thanks
                Hide
                Permalink
                Ankit Agarwal added a comment -

                I am pushing this forward for peer review.
                Thanks

                Show
                Ankit Agarwal added a comment - I am pushing this forward for peer review. Thanks
                Hide
                Permalink
                Ankit Agarwal added a comment - - edited

                Ps:- It does break BC, but I don't think anyone outside core would be using calendar_process_subscription_row(). Hence it doesn't need to clutter up upgrade.txt
                I will backport it after the review.
                Thanks

                Show
                Ankit Agarwal added a comment - - edited Ps:- It does break BC, but I don't think anyone outside core would be using calendar_process_subscription_row(). Hence it doesn't need to clutter up upgrade.txt I will backport it after the review. Thanks
                Hide
                Permalink
                Mark Nelson added a comment -

                Hi Ankit,

                I totally agree that we definitely should not be passing language strings as the value for variables, since it can vary from language to language and also makes filtering not as reliable. So, what you have done so far looks great. I don't think it will really break older Moodle versions since, like you said, no one is going to be calling this function directly. I did a grep in a directory I have on my HD that contains the third party modules (a little outdated though) just to verify.

                I would suggest someone from FRONTEND verify that the replacement of the input tag with a button tag is acceptable/accessible, and if so, this can be pushed for integration. I understand that the input field needs to be changed to a button in order for you to pass the integer as the value and still keep the translated name, so I would say it is OK.

                Regards,

                Mark

                Show
                Mark Nelson added a comment - Hi Ankit, I totally agree that we definitely should not be passing language strings as the value for variables, since it can vary from language to language and also makes filtering not as reliable. So, what you have done so far looks great. I don't think it will really break older Moodle versions since, like you said, no one is going to be calling this function directly. I did a grep in a directory I have on my HD that contains the third party modules (a little outdated though) just to verify. I would suggest someone from FRONTEND verify that the replacement of the input tag with a button tag is acceptable/accessible, and if so, this can be pushed for integration. I understand that the input field needs to be changed to a button in order for you to pass the integer as the value and still keep the translated name, so I would say it is OK. Regards, Mark
                Hide
                Permalink
                Ankit Agarwal added a comment -

                Thanks Mark,
                For the review. I asked, Jerome's opinion on this as well. He agrees with the change. Pushing forward for integration.

                Thanks

                Show
                Ankit Agarwal added a comment - Thanks Mark, For the review. I asked, Jerome's opinion on this as well. He agrees with the change. Pushing forward for integration. Thanks
                Hide
                Permalink
                Ankit Agarwal added a comment -

                @integrators
                Please note that only one of MDL-41485 and MDL-41468 should go in. The other should be closed.
                Thanks

                Show
                Ankit Agarwal added a comment - @integrators Please note that only one of MDL-41485 and MDL-41468 should go in. The other should be closed. Thanks
                Hide
                Permalink
                Damyon Wiese added a comment -

                Thanks Ankit,

                I've integrated and tested this on 24, 25 and master.

                Show
                Damyon Wiese added a comment - Thanks Ankit, I've integrated and tested this on 24, 25 and master.
                Hide
                Permalink
                Damyon Wiese added a comment -

                Tested on all branches in integration. Passing test.

                Show
                Damyon Wiese added a comment - Tested on all branches in integration. Passing test.
                Hide
                Permalink
                Dan Poltawski added a comment -

                Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke:

                A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

                Show
                Dan Poltawski added a comment - Congratulations! This change has been integrated upstream and is now available from our git and download mirrors. To celebrate, here is a joke: A SQL query goes into a bar, walks up to two tables and asks, "Can I join you?"

                  People

                  • Votes:
                    0 Vote for this issue
                    Watchers:
                    5 Start watching this issue

                    Dates

                    • Created:
                      Updated:
                      Resolved:

                      Agile