Description

The purpose of this module is simple; bring the free, open source, and accessible HTML5 media player "Able Player" to Drupal.

Able Player is a jQuery plugin that acts on existing HTML5 audio and video elements to provide a high level of accessibility, particularly with the display of captions, audio descriptions, and interactive transcripts. It is unique among existing HTML5 media players in that it is designed first and foremost for accessibility, with a professional technology accessibility specialist as its primary developer.

This module works with File Entity to integrate Able Player as a file formatter, allowing Able Player to work with any applicable media anywhere in a Drupal site. Users simply enable 'Able Player' as the default file display for audio and video files and Able Player will be used to render the supported file formats.

This module leverages the Libraries API, Modernizr API, and File Entity API for a simple and extensible implementation.

Project Links

Project Page
Able Player Home Page

Clone the Repository

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/npacker/2508441.git ableplayer
CommentFileSizeAuthor
#6 2508441-1.patch14.11 KBtheMusician
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

npacker created an issue. See original summary.

npacker’s picture

Issue summary: View changes
PA robot’s picture

Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxnpacker2508441git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

npacker’s picture

Fixed some issues found in automatic code review.

npacker’s picture

Status: Needs work » Needs review
theMusician’s picture

Status: Needs review » Needs work
FileSize
14.11 KB

Running the automated project application review script generated a few minor formatting suggestions.

I have attached a patch which I think cleans up the formatting portion of the review.

A few warnings were generated about undefined variables as well in the theme.inc file.

Explicitly declaring those should clean it up and we can move to reviewing the code for potential security issues.

npacker’s picture

Patch applied to fix remaining significant automated review issues.

npacker’s picture

Status: Needs work » Needs review
npacker’s picture

Issue summary: View changes
npacker’s picture

Issue summary: View changes
Amerie’s picture

I looked through the code for security issues and found nothing wrong. I did notice a couple of very minor things that could be improved if you have time:

  1. The project page says to rename the custom Modernizr file to "custom.modernizr.js", but I had to rename mine to "modernizr.min.js" to make it work (this name came from the site's status report).
  2. The language switcher in the transcript box for videos is not aligned properly due to display:block being applied to the auto scroll label in system.theme.css.
  3. The player works on a fresh install of Drupal 7.x-3.8 with only the dependencies listed on the project page, but there is no way to add captions to a video as far as I can tell without enabling the Media module and using the Media Browser widget for the video field. This might be worth mentioning somewhere.
Amerie’s picture

Status: Needs review » Reviewed & tested by the community

Issues above should not stop this from being promoted to a full project.

Sumit kumar’s picture

Thanks for contribution @npacker
code is working for me

mgifford’s picture

Happy to see this is moving along. I haven't had time to work with it, but know that this module will benefit a lot of organizations who need an accessible media player. Happy it's RTBC!

kattekrab’s picture

@npacker - this is RTBC - looks like you need to review 1 more project to get the PA bonus - and then this project can be approved and promoted!

npacker’s picture

Issue summary: View changes
npacker’s picture

Issue tags: +PAreview: review bonus
klausi’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -PAreview: review bonus

manual review:

  1. "'access arguments' => array('administer site'),": the 'administer site" permission does not exist, did you mean "administer site configuration"? This is currently not a security problem, because only user 1 will be able to access the settings page for a permission that does not exist.
  2. ableplayer_form(): doc block is wrong, this is not hook_form(). See https://www.drupal.org/coding-standards/docs#forms on how to document form constructors.
  3. "Impelemtnts hook_file_formatter_FORMATTER_view()'": typo, should be "Implements".
  4. "t('The file') . ' ' . t('@file', array('@file' => $path)) . ' ' . t('was not found.');": do not split up a translatable sentence like that. This should be one t() call with the "%" placeholder which will add em tags for you.
  5. theme_ableplayer(): this is vulnerable to XSS exploits. The transcript title is user provided text and needs to be sanitized before printing to HTML. Ideally that should be done in a preprocess function or similar in the field API. Make sure to read https://www.drupal.org/node/28984 again. I think this is not a security issue because it requires some "administer file types" permission that already says it should only be given to trusted users. In any case this should still be fixed.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

klausi’s picture

The XSS test string I used for the exploit: a"><script>alert('XSS');</script><a class="

npacker’s picture

@klausi: Thank you for the review!

npacker’s picture

Issue summary: View changes
npacker’s picture

Status: Needs work » Needs review
andrewmacpherson’s picture

Status: Needs review » Reviewed & tested by the community

All of @klausi's issues from comment #18 have been addressed, so back to RTBC?

  1. Permission name - fixed by fce54dd
  2. Form constructor docblock - fixed by 1c9f354
  3. Typo "impelemtnts" - fixed by 414d628
  4. Use of t() % placeholder - fixed by 21b7b1c
  5. XSS addressed with use of check_plain() in 5d4943a

It's frustrating that the product review bonus has been lost, and another 3 reviews are needed, but that seems to be the current policy.

@npacker is already making good use of the issue queue for the module. At the moment there are a couple of open issues there, but I don't see them as blockers to project promotion, and both issues are making good progress.

MiSc’s picture

Status: Reviewed & tested by the community » Fixed

Doing some clean up in the queue of old issues, and see nothing that should stop you from getting the git vetted user role, so:

Thanks for your contribution, npacker!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

npacker’s picture

Thanks to @MiSc and to all who reviewed!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.