Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41485

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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 Master Branch:
      MDL-41485-master
    • Sprint:
      BACKEND Sprint 4
    • Sprint:
      BACKEND Sprint 4

      Description

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

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            ankit_frenz 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_frenz 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
            ankit_frenz Ankit Agarwal added a comment -

            I am pushing this forward for peer review.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - I am pushing this forward for peer review. Thanks
            Hide
            ankit_frenz 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_frenz 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
            markn 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
            markn 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
            ankit_frenz 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_frenz 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
            ankit_frenz 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_frenz 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
            damyon Damyon Wiese added a comment -

            Thanks Ankit,

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

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

            Tested on all branches in integration. Passing test.

            Show
            damyon Damyon Wiese added a comment - Tested on all branches in integration. Passing test.
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  9/Sep/13

                  Agile