Skip to content

Valera/bugfix tabbing video controls - #659

Merged
valera-rozuvan merged 7 commits into
masterfrom
valera/bugfix_tabbing_video_controls
Aug 14, 2013
Merged

Valera/bugfix tabbing video controls#659
valera-rozuvan merged 7 commits into
masterfrom
valera/bugfix_tabbing_video_controls

Conversation

@valera-rozuvan

Copy link
Copy Markdown
Contributor

This PR provides fixes for bugs:

1.) When tabbing backwards, focus should switch in succession ( https://edx-wiki.atlassian.net/browse/BLD-228 ).
2.) Tabbing from speeds to volume control doesn't close Speeds dialog ( https://edx-wiki.atlassian.net/browse/BLD-233 ).

Reviewers

  • @polesye
  • @peter-fogg
  • @talbs Please have a quick look from the point of view of user interface. Ease of tabbing from one control to the next.

@talbs

talbs commented Aug 13, 2013

Copy link
Copy Markdown
Contributor

@valera-rozuvan, this looks good in general. I do have one question: I noticed that once the speed menu is opened, I am able to tab through all of the speeds (which is great), but if I want to reverse tab back through the previous speeds (going frmo 0.25 to 2.o for example), I'm not able to (as I get jumped to toggling the speed menu in general again). Can you address this?

@valera-rozuvan

Copy link
Copy Markdown
Contributor Author

@talbs The cause of this is that we can't tell the direction of tabbing. We have the 'focus' and 'blur' events. Unfortunately, different means of focus switching use different mechanisms (keyboard, screen readers, etc.) - so we can't just assume that all backward tabbing will be done with a Shift+Tab. This is why I have to always assume that when the user is on the last speed (the one on the bottom, 0.25), he is tabbing forwards. You can go backwards (focus) from a speed one higher than 0.25 - please try this.

Maybe you can suggest a solution to this problem. I have thought about hard coding the order of controls, and then looking on each 'blur' and 'focus' events where we are coming from. The only problem with this approach is that when a new control is added, hard-coding will have to be redone.

@ghost ghost assigned polesye Aug 13, 2013
@talbs

talbs commented Aug 13, 2013

Copy link
Copy Markdown
Contributor

@valera-rozuvan, thanks for the explanation. I don't think this is a blocker for this work being merged in and I don't have time currently to dive in. The feature is definitely usable without it, I'd just note the extra tab work somewhere in the backlog (where it can be prioritized).

👍 from me.

@peter-fogg

Copy link
Copy Markdown
Contributor

Were these bugs present in the old video player? Or is it a new issue in Videoalpha?

Either way, looks good. 👍 when the tests pass.

@valera-rozuvan

Copy link
Copy Markdown
Contributor Author

@peter-fogg These bugs were not present in old video player because this is new functionality. Tabbing through controls forwards and backwards has been added only recently.

Valera Rozuvan added 4 commits August 14, 2013 10:51
An old TODO item was done. It turns out a simple case of calling
the method to bind handlers after the Spees dialog was re-rendered.
Now you can tab through all of the controls in Video forwards, and then tab backwards.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contrinue -> continue

@polesye

polesye commented Aug 14, 2013

Copy link
Copy Markdown
Contributor

@valera-rozuvan After addressing my comments, you can merge. 👍

valera-rozuvan added a commit that referenced this pull request Aug 14, 2013
@valera-rozuvan
valera-rozuvan merged commit 642a4fa into master Aug 14, 2013
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
mtyaka referenced this pull request in open-craft/openedx-platform Mar 25, 2016
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Mar 30, 2016
…z-for-merge-dashboard-link

Implements biz dashboard link openedx#606
dfrojas pushed a commit to eduNEXT/edx-platform that referenced this pull request Aug 31, 2017
* stv/style-guide:
  Explain commented code in style guide
  Explain trailing commas in style guide
  Explain collection definition in style guide
  Explain import sorting in style guide
  Explain docstrings in style guide
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
johanseto pushed a commit to nelc/edx-platform that referenced this pull request Jan 22, 2024
feat: backport HomepageRenderStarted and CatalogRenderStarted filters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants