Problem/Motivation

The overlay module manages tabindices on a page when the overlay is open. Users navigating page elements with a keyboard are constrained to the overlay container as the context of their tabbing.

As we enrich the accessibility profile of components of Drupal, it's becoming more necessary to managing the tabbing of page elements in other contexts. One of these needs is the unified editing efforts of contextual links and inline editing: #1874664: Introduce toolbar level "Edit" mode that shows all contextual links.

Ultimately we want one system that manages the tabbing of a page rather than multiple subsystems wrestling for control of tabbing elements on a page.

Proposed resolution

Pull the makeDocumentUntabbable and makeDocumentTabbable methods out of overlay and put them in their own object with a proper API. Make this new library a dependency of overlay.

Remaining tasks

  1. Propose a reviewable patch.
  2. Write tests: http://drupalcode.org/project/fat.git/blob/HEAD:/tests/drupal.tabbingman...

User interface changes

The goal is to make efforts to improve accessibility easier, so keyboard navigating users will have a better experience with Drupal.

API changes

Introduction of a new property on the Drupal object: Drupal.TabbingManager

Files: 
CommentFileSizeAuthor
#53 tabbingmanager-1913086-53.patch25.22 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 54,474 pass(es).
[ View ]
#53 interdiff.txt2.58 KBWim Leers
#52 core-js-tabbingmanager-1913086-52.patch26.08 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 54,458 pass(es).
[ View ]
#52 interdiff_51-to-52.txt1019 bytesjessebeach
#51 core-js-tabbingmanager-1913086-51.patch25.04 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 54,604 pass(es).
[ View ]
#51 intediff.txt13.23 KBnod_
#48 Tabbing-is-no-longer.png69.69 KBmgifford
#48 Tabbing is constrained.png86.42 KBmgifford
#45 testing-helper-patch.txt499 bytesjessebeach
#43 tabbing-1913086-43.patch28.62 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 54,482 pass(es).
[ View ]
#43 interdiff_36-to-43.txt6.31 KBjessebeach
#36 tabbing-1913086-36.patch25.71 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 54,529 pass(es).
[ View ]
#34 tabbing-1913086-34.patch12.75 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 54,193 pass(es).
[ View ]
#34 interdiff-29-34.txt12.6 KBmgifford
#29 tabbing-1913086-29.patch25.91 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tabbing-1913086-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#29 interdiff.txt4.39 KBWim Leers
#28 tabbing-1913086-28.patch26.03 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 54,102 pass(es).
[ View ]
#28 interdiff.txt3.39 KBWim Leers
#24 Screenshot_4_4_13_5_21_PM.png53.83 KBjessebeach
#24 tabbing-1913086-24.patch26.63 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 53,913 pass(es).
[ View ]
#24 tabbing-1913086-just-tabbingmanager-do-not-test.patch14.45 KBjessebeach
#24 tabbing-1913086-implementation-do-not-test.patch12.6 KBjessebeach
#22 tabbing-1913086-22.patch34.15 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 53,919 pass(es).
[ View ]
#22 interdiff_19-to-22.txt52.31 KBjessebeach
#19 Screenshot_3_31_13_5_22_PM-2.png20.18 KBjessebeach
#19 Screenshot_3_31_13_5_21_PM.png61.43 KBjessebeach
#19 Screenshot_3_31_13_5_24_PM.png54.33 KBjessebeach
#19 tabbing-1913086-19.patch24.31 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 53,894 pass(es).
[ View ]
#12 tabbing-1913086-12.patch23.92 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 53,241 pass(es).
[ View ]
#12 testswarm-tabbingmanager-tests-do-not-test.patch10.96 KBjessebeach
#12 interdiff_9-to-12.txt23.93 KBjessebeach
#9 tabbing-1913086-9.patch13.5 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 52,532 pass(es).
[ View ]
#9 interdiff.txt10.36 KBWim Leers
#2 tabbing-1913086-2.patch11.28 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 50,443 pass(es).
[ View ]
#1 tabbing-1913086-1.patch10 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 50,431 pass(es).
[ View ]

Comments

Status:Active» Needs work
Issue tags:+JavaScript, +accessibility
StatusFileSize
new10 KB
PASSED: [[SimpleTest]]: [MySQL] 50,431 pass(es).
[ View ]

This is a first try at extracting the tab management from overlay. There are a still a few threads to sever.

StatusFileSize
new11.28 KB
PASSED: [[SimpleTest]]: [MySQL] 50,443 pass(es).
[ View ]

Ok, this patch separates the management of tabbing order from the Overlay. It's not perfect by any stretch yet. I'll be improving it with another use case that requires a couple levels of tab management with popups.

This is a great initiative. I'll try to get more people involved.

This makes a lot of sense.

Especially when you add this functionality, which you want to add according to #1913214-1: Accessibility followup for Edit tab toggle of contextual links for in-place editing.:

Drupal.TabbingManager needs more work to handle stacked tabbing contexts. What I mean by this is, when a user clicks on the quick edit link of a contextual link set, the tabbing should be constrained to the editable fields. When an editable field is clicked, the tabbing should be constrained to the in-place editing dialog. Backing out of this path should reinstate the tabbing order of the previous UI state.

Shouldn't this also handle the "aural view" on the content, i.e. the ARIA live region? It sounds like it would make sense to have an aural message to be associated with entering and leaving a specific stacked tabbing context?

Issue tags:+in-place editing, +Spark

.

Assigned:Unassigned» Wim Leers

Jesse asked me to work on this.

I wanted to post a conversation that Wim and I had about the tabbing context stack management logic so it isn't lost in a private chat room.

Wim Leers: Jesse Beach: "if the set that is released is not at the top of the sets stack, it should be marked for release once all of the preceding stacks are popped." → I'd much rather see that it's explicitly not supported to pop anything but the thing at the top of the stack. Otherwise, this whole concept is "like stacks, but actually not really", which is super confusing. I think it's totally acceptable to do it that way? How else can we provide any correctness guarantees?
Jesse Beach: Wim Leers: my thought on this was, if the contextual module releases its tabbing constraint, but I"m still in the modal for editing a field, when I exit the modal, I don't want to go back to the contextual tabbing constraint, I want to go back to the default tabbing order
Jesse Beach: So in a sense, when a module releases the constraint, it essentially washes its hands of it
Jesse Beach: and no longer attempts to manage it
Jesse Beach: that might not work in practice
Jesse Beach: but it seemed plausible
Jesse Beach: what I wanted to avoid is that, if I'm in a dialog editing a field, and I click the Edit mode toolbar toggle, I shouldn't have to back out of the tabbing management sets in the right order
Jesse Beach: each module should be able to release its set(s) and walk away
Wim Leers: In short: I agree, but I think it's the code's responsibility to ensure the right things happen, the user won't have to do extra things.
Jesse Beach: if they release out of order, as long as all of them are released in some order, I'll return back to the default tabbing structure eventually
Jesse Beach: right,
Jesse Beach: it's these kinds of gotchas that had me spinning for two days on this problem 
Wim Leers: So can you come up with a concrete use case where this would be problematic?
Jesse Beach: toggling the edit tab
Jesse Beach: when I'm editing a field
Jesse Beach: I need to cancel out of the whole edit experience
Jesse Beach: and return to the default state
Wim Leers: So stack level 1 constraint: document, stack level 2: pencil icons, stack level 3: in-place editing.
Wim Leers: You disable edit mode
Jesse Beach: stack 4, editing a specific field
Wim Leers: Isn't that stack level 3?
Jesse Beach: we should have called this tabception
Jesse Beach: level three is tabbing between the fields
Wim Leers: oh, fair enough
Wim Leers: So as soon as you stop in-place editing altogether (so release levels 4 and 3), you want to go back to the document level, so you want level 2 to be released automatically
Wim Leers: So it should be *marked* as released, but not *actually* released until the time is there
Wim Leers: hm that's not true I guess
Wim Leers: Couldn't we just remove that one immediately?
Wim Leers: That wouldn't cause problems, I think?
Wim Leers: I think it's always possible to remove any non-top or non-bottom tabsets?
Jesse Beach: the release process could just skip over "released" sets
Jesse Beach: If I release #3, but #4 is active
Jesse Beach: three is marked to be released.
Jesse Beach: If I release #4, I just jump to #2
Jesse Beach: no need to munge the DOM to undo #3
Wim Leers: Jesse Beach: precisely, but doesn't that imply we can already get rid of the data?
Jesse Beach: so before #3 is released, it queries the set to see if there are any sets before it that are active. If so, just mark for release and exit
Jesse Beach: releasing #2 should clear the stack above it
Jesse Beach: or maybe we just clear the data?
Jesse Beach: that could work too
Wim Leers: Okay, that's what I was thinking 
Jesse Beach: once it's released, it can't be reversed
Wim Leers: y
Jesse Beach: ya, no need to keep it around

Assigned:Wim Leers» jessebeach
Status:Needs work» Needs review
StatusFileSize
new10.36 KB
new13.5 KB
PASSED: [[SimpleTest]]: [MySQL] 52,532 pass(es).
[ View ]

Changes/notes

  • I cross-checked what Drupal.overlay.makeDocument(Unt|T)abbable does with the resulting behavior in Drupal 7 versus Drupal 8 (D8 without this patch of course), to ensure that I wouldn't break anything (testing with Chrome + ChromeVox). It turns out that, sadly, a11y of the overlay is already broken in D8 HEAD. When you click anywhere in the overlay and continue tabbing, you won't end up on the underlying page, but in the address bar — this is good, and works identically to D7. However, click anywhere in the overlay and continue shift-tabbing, you won't end up at the end of your bookmarks bar, but on the page: you'll be tabbing through the different contextual links. Note that this is without #1874664: Introduce toolbar level "Edit" mode that shows all contextual links, thus it's an actual regression of D8 HEAD already.
    But… the patch in #2 already fixes that! Great job, Jesse :) The patch in #2 effectively already gets us back to parity with Drupal 7.
  • Clarified the "array as stack" data structure.
  • Several minor JS issues fixed.
  • Got rid of the initialization phase, which leads to less memory use and less performance impact.
  • Stacked tabbing contexts are working.
  • I investigated not doing (no-opping) this on mobile devices since it involves quite a lot of DOM operations, but there the tabindex is also used, so that can't fly.

Todos/discussion points

  • I strongly dislike how "unstructured" the JS currently is (when compared to non-JS code, such as PHP or C++). I prefer a more nested declaration instead of "pulling everything together". Do we have guidelines for this? Note that I didn't change a single thing about the public API in #2 though.
  • This does not yet deal with tabbable elements that are added to the page after the tabbing has been constrained. But then again, that's something Overlay in Drupal 7 also didn't do. That should be a follow-up issue.

The patch applies nicely. This is an important piece of work. Having a centralized, controllable tab order is important for keyboard only users.

I suspect that the JS should be reviewed before being marked RTBC.

Just created a stub for an issue for the todo - #1921218: Deal with tabbing elements that are added to the page after it has been constrained

#9: tabbing-1913086-9.patch queued for re-testing.

StatusFileSize
new23.93 KB
new10.96 KB
new23.92 KB
PASSED: [[SimpleTest]]: [MySQL] 53,241 pass(es).
[ View ]

I strongly dislike how "unstructured" the JS currently is (when compared to non-JS code, such as PHP or C++). I prefer a more nested declaration instead of "pulling everything together". Do we have guidelines for this? Note that I didn't change a single thing about the public API in #2 though.

I refactored the code into a TabbingManager class and a TabbingContext class and removed the closure-scoped variables.

Stacked tabbing contexts are working.

I'd like to write tests against these stacked contexts. I started a testing file. It's included as a patch to the 8.x-1.x branch of the testswarm module.

I tried to normalize the filtering of tabbable elements by introducing two jQuery pseudo selectors: :drupalTabbable and :drupalUntabbable. For example, one could write $('body').find(':drupalTabbable'); and find all the tabbable elements on the page.

These two pseudo selectors need tests.

My plan from this point out is to increase the test coverage as a way to verify the behavior of the TabbingManager.

Not entierly happy about the new selectors, see #1931632: [META] Make CORE compatible with jQuery native-API selector. A function somewhere would be better. At least the jquery extensions deserve it's own file.

Status:Needs review» Needs work

First big chunk of feedback; I didn't go in detail on the TabbingContext code, I think it's important to get the higher-level stuff right first.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+    // once() returns a jQuery set. It will be empty if no unprocessed
+    // elements are found. window and window.parent are equivalent unless the
+    // Drupal page is itself wrapped in an iframe.
+    $(window.parent.document.body).once('drupalTabbingManager', function () {
+      // If this window is itself in an iframe it must be marked as processed.
+      // Its parent window will have been processed above.
+      // When attach() is called again for the preview iframe, it will check
+      // its parent window and find it has been processed. In most cases, the
+      // following code will have no effect.
+      $(window.document.body).once('drupalTabbingManager');

Does this mean that all code being introduced in this issue does not run if a Drupal site is running in an iframe?

(In other words: should we really implement it this way and not simply check for the Overlay module specifically? There, the iframe's window is accessible via Drupal.overlay.iframeWindow IIRC.)

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+      $(document).on('keydown.drupalTabbingManager keyup.drupalTabbingManager', keyHandler);

window and document are not yet listed in the file-level closure.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+    // Esc key.
+    case 27:
+      break;
+    // Enter or space keys.
+    case 13:
+    case 32:
+      break;

Why do we detect these if we don't do anything with it?

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+function TabbingManager () {

s/TabbingManager/TabbingContext/Manager/ ?

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+  // By default, browsers make a, area, button, input, object, select, textarea,
+  // and iframe elements reachable via the tab key.
+  this.browserTabbableElementsSelector = browserTabbableElementsSelector;

It's weird that the comment enumerates the full list but the code then refers to *another* variable instead of declaring them here. Why not declare them here?

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+   * @param {String, jQuery} set
+   *   The set of elements to which tabbing should be constrained.

There's a new undocumented parameter: "id".

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+    // If the id parameter is an object, then no id is provided for this set.
+    if (typeof id === 'object') {
+      set = id;
+      id = null;

Why not *require* an id? It'd make debugging a lot easier, and it's not a lot to ask for.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+    var $disabledTabbingSet = $(':drupalTabbable')

You renamed $tabbable to $disabledTabbingSet — THANK YOU! That's much, much better :) Glad to be rid of that very confusingly named variable from Overlay!

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+      // Get a fresh set of the tabbable elements so that the array will be
+      // enumerated in DOM order.
+      $tabbableElements: $tabbableElements,

What's "fresh" about this?

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+      $disabledTabbingSet: $disabledTabbingSet,

It's confusing that the function docs say "constrain to specified set of elements only" (note "set" in there), that we then have $disabledTabbingSet (again "set"), but $tabbableElements (no "set")!

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+    // Only allow the top of the stack to be unwound and only unnwind if the top
+    // of the stack is released.
+    var top = this.stack.slice().pop();
+    if (tabbingContext !== top || !top.isReleased()) {
+      return;
+    }
+
+    // Unwind as far as possible.
+    // If there are e.g. 3 stacked tabbing constraints, such as: 1) document/page,
+    // 2) contextual links' edit mode, 3) in-place editing, and the second stack
+    // level is released, then the if-test above (which only allows the top of the
+    // stack to be unwound) will cause the second stack level to be marked as
+    // released, but to not actually be released. This is what ensures that the
+    // top-level tabbing constraint remains active. Once that is marked as
+    // unactive, however, we must not only unwind the top-level tabbing constraint
+    // (number 3 in the example), but also the level below that (number 2).
+    var stackLevelToRestore = this.stack.length;
+    for (var i = 0, n = this.stack.length; i < n && this.stack[i].isReleased(); i++) {
+      stackLevelToRestore--;

You've changed the first chunk of this piece of code, without changing the second. The long comment for the second chunk is now no longer accurate.

And IIRC this was a crucial aspect, which you've now broken.

Or… are you saying that the scenario outlined in this long comment is not realistic? I remember not being convinced it was necessary, but then you convinced me it was.

In any case, this is a *big* functional change and you did not mention it in your comment, so let's make sure it really is intentional, and appropriate!

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,427 @@
+  tab: function (event) {
+    var context = this.getActiveContext();
+    // If the context has already been released, do nothing.
+    if (!context || context.isReleased()) {
+      return;
+    }
+    switch (event.type) {
+      // Record the current active element.
+      case 'keydown':
+        $(event.target).css('background-color', 'red');
+        context.setActiveElement(event.target);
+        break;
+      // Respond to the new current active element.
+      case 'keyup':
+        $(event.target).css('background-color', 'blue');
+        (event.shiftKey) ? context.prev() : context.next();
+        break;
+      default:
+        break;
+    }
+  },

*HUH?*

1) Do we really want to be handling a browser-level behavior?

2) Do we really want to be specifically setting background colors?

+++ b/core/modules/overlay/overlay-child.jsundefined
@@ -179,11 +179,35 @@ Drupal.overlayChild.behaviors.shortcutAddLink = function (context, settings) {
+Drupal.overlayChild.behaviors.handleOffsetTopChange = function (context, settings) {
+  // Workaround because of the way jQuery events works.
+  // jQuery from the parent frame needs to be used to catch this event.
+  parent.jQuery(document).bind('offsettopchange', function (event) {
+    // Fires an event that the child iframe can listen to.
+    $(document).trigger('offsettopchange');
+  });

1) Seems unrelated?
2) Does not get called?

+++ b/core/misc/tabbingmanager.jsundefined
@@ -7,178 +7,421 @@
+    // once() returns a jQuery set. It will be empty if no unprocessed
+    // elements are found. window and window.parent are equivalent unless the
+    // Drupal page is itself wrapped in an iframe.
+    $(window.parent.document.body).once('drupalTabbingManager', function () {
+      // If this window is itself in an iframe it must be marked as processed.
+      // Its parent window will have been processed above.
+      // When attach() is called again for the preview iframe, it will check
+      // its parent window and find it has been processed. In most cases, the
+      // following code will have no effect.
+      $(window.document.body).once('drupalTabbingManager');

Does this mean that all code being introduced in this issue does not run if a Drupal site is running in an iframe?

(In other words: should we really implement it this way and not simply check for the Overlay module specifically? There, the iframe's window is accessible via Drupal.overlay.iframeWindow IIRC.)

+++ b/core/misc/tabbingmanager.jsundefined
@@ -7,178 +7,421 @@
+    // Esc key.
+    case 27:
+      break;
+    // Enter or space keys.
+    case 13:
+    case 32:

Why do we detect these if we don't do anything with it?

+++ b/core/modules/overlay/overlay-child.jsundefined
@@ -179,11 +179,35 @@ Drupal.overlayChild.behaviors.shortcutAddLink = function (context, settings) {
+  parent.jQuery(document).on('keydown.overlayChild keyup.overlayChild', function (event) {

parent object is not listed in file-level closure.

+++ b/core/modules/overlay/overlay-child.jsundefined
@@ -179,11 +179,35 @@ Drupal.overlayChild.behaviors.shortcutAddLink = function (context, settings) {
+    // Fires an event that the child iframe can listen to.

You mean the parent frame, not the child frame, right?

+++ b/core/modules/overlay/overlay-child.jsundefined
@@ -179,11 +179,35 @@ Drupal.overlayChild.behaviors.shortcutAddLink = function (context, settings) {
+    switch (event.keyCode) {
+      // Tab key.
+      case 9:
+        parent.Drupal.TabbingManager.tab(event);
+        break;
+      default:
+        break;

Why a switch statement if you're only checking for one event code, and AFAIT, only ever will check for one event code?

+++ b/core/modules/overlay/overlay-parent.jsundefined
@@ -301,6 +298,14 @@ Drupal.overlay.loadChild = function (event) {
+
+      // Find the tabble elements in the iframeWindow document.

Excessive whitespace?

+++ b/core/modules/overlay/overlay-parent.jsundefined
@@ -301,6 +298,14 @@ Drupal.overlay.loadChild = function (event) {
+      Drupal.overlay.releaseTabbing();
+      Drupal.overlay.constrainTabbing($iframeTabbables);

Why not have constrainTabbing() call releaseTabbing() automatically?

+++ b/core/modules/overlay/overlay-parent.jsundefined
@@ -441,9 +446,11 @@ Drupal.overlay.eventhandlerAlterDisplacedElements = function (event) {
-    var iframeDocument = Drupal.overlay.iframeWindow.document;
-    $(iframeDocument.body).attr('data-offset-top', Drupal.overlay.getDisplacement('top'));
-    $(iframeDocument).trigger('offsettopchange');
+    if (Drupal.overlay.iframeWindow) {
+      var iframeDocument = Drupal.overlay.iframeWindow.document;
+      $(iframeDocument.body).attr('data-offset-top', Drupal.overlay.getDisplacement('top'));
+      $(iframeDocument).trigger('offsettopchange');

I think this is an unrelated change?

+++ b/core/modules/system/system.moduleundefined
@@ -1397,6 +1397,19 @@ function system_library_info() {
+      'core/misc/tabbingmanager.js' => array('group', JS_LIBRARY),

'group' => JS_LIBRARY

Priority:Normal» Major
Issue tags:+sprint

Talking to the team, this is a major task; it's a huge improvement for accessibility across multiple front-end features (e.g. toolbar, edit, and contextual links)

Talking to the team

Which team? Please note that this does not come across inclusive; it sheds an exclusive/elitist light on the Drupal core queue.

Sorry, the team who's involved in the development and maintenance of these pieces of core (in this case, the Spark team). I don't find this any more or less exclusive/elitist than other similar teams coming up with similar plans of attack and documenting it in the issue queue, but that certainly wasn't our intent.

Issue summary:View changes

Updated issue summary.

Tests for this utility are being developed in the FAT module: http://drupalcode.org/project/fat.git/blob/HEAD:/tests/drupal.tabbingman...

StatusFileSize
new24.31 KB
PASSED: [[SimpleTest]]: [MySQL] 53,894 pass(es).
[ View ]
new54.33 KB
new61.43 KB
new20.18 KB

I'm leaving this as needs work. But this patch is moving towards what I think the solution will be. Contextual and Overlay have been updated in this patch to use the TabbingManager. If you'd like to test, first enable Edit mode. You can use your keyboard and the tab key to do this.

Screenshot of a Drupal 8 site with an arrow pointing to the Edit mode toggle in the toolbar.

After enabling edit mode, only the contextual link triggers and the edit mode toggle should be reachable via the tab key.

Screenshot of a Drupal 8 site with the edit mode enabled. Arrows point to the edit mode toggle and the contextual link triggers.

Select the configure block contextual link for the Search block to launch the Overlay.

Screenshot of a Drupal 8 site. An arrow points to the Configure block action of the Search block contextual link set.

When the overlay launches, all of the tabbables in the Toolbar and the Overlay should be reachable via the keyboard. When the Overlay is closed, rather than the entire document becoming tabbable again, the active tabbing set is reconstrained to the contextual links. Disabling edit mode from the pencil icon in the Toolbar will restore all the tabbable elements in the document.

Under the hood, the TabbingManager is creating a TabbingContext object for each set of tabbables: one for the set of contextual triggers and one for the links in the Overlay and Toolbar. The TabbingManger stores these sets in a stack that is shifted and unshifted so that previously active TabbingContext sets are reactivated when more recent sets are deactivated. In this case, when the Overlay's TabbingContext instance is released, the Contextual TabbingContext instance is reactivated.

The TabbingManger utility has complete test coverage in the FAT module: http://drupal.org/project/fat

Wim, there's one comment in #14 that I haven't addressed yet:

+++ b/core/modules/overlay/overlay-parent.jsundefined
@@ -301,6 +298,14 @@ Drupal.overlay.loadChild = function (event) {
+ Drupal.overlay.releaseTabbing();
+ Drupal.overlay.constrainTabbing($iframeTabbables);
Why not have constrainTabbing() call releaseTabbing() automatically?

I hope the rest of your comments are addressed in this refactor.

Next steps

  1. Code review the refactor.
  2. Implement Drupal.announce() to inform a screen reader user that the tabbing context has changed. The new context should be described. For example, the number of tabbables should be given as well as the purpose of the constraint and how to exit it.
  3. Deeper integration with Contextual/Edit for in-place editables. I just haven't had time to get to this yet.
  4. We might need to implement some kind of "looping" behavior for modal-like containers where, when a user tabs off the first or last item in the tabbable set, the focus is loop to the last or first item.
  5. TabbingManager needs something like a releaseAllSets method to get back to the default document tabbing.

Usage feedback

When the menu tab's tray is open, tab to Contextual's "Edit mode", enable it, reload the page: now I'm still able to tab to the menu tab's tray. It seems like it doesn't detect the "edit mode enabled" event upon page load.

Other than that, it seems to work great.

Tests

The tests in the FAT module fail: all pass besides the contextual, tabbingmanager, reorderblocks and createpagecontent tests. For tabbingmanager, 15 out of 84 fail. Can you please check whether you have the same problem? If you don't, then we have to figure out what's different.

Note that I had to patch the Test Swarm module to get it to work at all: #1958896: Breaks when Drupal is installed in a subdirectory.

In-depth review

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+   *   An object with isReleased() and release() methods, respectively to check
+   *   whether the constrained tabbing has been released and to release it.

This is misleading. There are many more methods, and even in this function we use more (deactivate() and activate() at the very least).

Why not just say "A TabbingContext object."?

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+  constrain: function (set) {
+

Extraneous newline.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+    for (var i = 0, il = this.stack.length; i < il; i++) {

1) il is not declared within this scope.
2) why is this necessary? Isn't this a premature (micro)optimization?

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+    var $set = $(set).find(':tabbable').andSelf().filter(':tabbable');

andSelf() is deprecated since jQuery 1.8.

http://jquery.com/upgrade-guide/1.9/#addback-selector-replaces-andself-

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+      // The level is the current top of the stack before this new
+      // is pushed on top tabbingContext.

s/current top/current height/

s/on top tabbingContext/on top of the stack/

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+      // Get a fresh set of the tabbable elements so that the array will be
+      // enumerated in DOM order.

Are we really getting a "fresh set" here? We're simply assigning the $set we retrieved just a few lines higher?

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+    // Activating the tabbingContext will manipulate the DOM to constrain
+    // tabbing.

(nitpick) Inconsistent phrasing wrt earlier comments.

Activate the tabbingContext; this will manipulate …

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+   * Restores former tabbing context when an active tabbing context is released.
+   *
+   * Once a TabbingContext object is released, it can never be activated again.

"release" is used here, but the function itself is called "unwind". We should make this consistent.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+    // Only allow the top of the stack to be unwound and only unnwind if the top

s/unnwind/unwind/

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+    var stackLevelToRestore = this.stack.length;
+    for (var i = this.stack.length -1; i >= 0; i--) {
+      // As soon as a set in the stack is encountered that is not released,
+      // break out of the for loop.
+      if (!this.stack[i].isReleased()) {
+        break;
+      }
+      stackLevelToRestore--;

Flaws:
- line 1: We can only reach this line if the top tabbing context. Note that this variable is initialized to an non-existent level (e.g. if length is 3, then the maximum level is 2, but this would initialize to 3). This can be safely initialized to length minus 2.
- line 2: Same reasoning as above; this is unnecessarily checking whether the top level in the stack is released.

AFAICT this can be simplified to:

var stackLevelToActivate = tabbingContext.level;
while (stackLevelToActivate > 0 && this.stack[stackLevelToActivate].isReleased()) {
  stackLevelToActivate--;
}

(This is analogous to the code I was using in #9.)

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+  _activateSet: function (tabbingContext) {

s/_activateSet/_activateTabbingContext/

(same for the deactivate sister)

?

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+    var $hasFocus = $set.filter('[autofocus]').eq(0);
+    if (!$hasFocus.length > 0) {
+      $hasFocus = $set.eq(0);

This is … strange.

The first line already grabs the first element.

The second line then does a "!length > 0" — IDK what that means.

Seems like a remnant from earlier code :)

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+  _restoreTabindex: function ($set, level) {
+

Extraneous newline.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+    for (var i = 0, il = $set.length; i < il; i++) {

See other "il" remark.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+   * Stores the jQuery set of elements that have had their tabbing disabled.

s/have had/will have/

?

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+   *   The jQuery set of elements that have had their tabbing disabled

Missing trailing period.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+      // Allow modules to respond to the constrain event.

s/constrain/unconstrain/

+++ b/core/modules/contextual/contextual.toolbar.jsundefined
@@ -122,6 +126,26 @@ Drupal.contextualToolbar.views.EditToggleView = Backbone.View.extend({
+    // Always release an existing tabbing context and create a new one.

Remove "and create a new one".

This part of the comment is inaccurate.

+++ b/core/modules/contextual/contextual.toolbar.jsundefined
@@ -122,6 +126,26 @@ Drupal.contextualToolbar.views.EditToggleView = Backbone.View.extend({
+    if (!isViewing) {

New comment above: "Create a new tabbing context when edit mode is enabled."

(Or something along those lines.)

+++ b/core/modules/overlay/overlay-child.cssundefined
@@ -53,10 +53,6 @@
-.overlay #skip-link {
-  margin-top: -20px;
-}
.overlay #skip-link a {
   color: #fff; /* This is white to contrast with the dark background behind it. */
}
@@ -143,7 +139,7 @@
@@ -143,7 +139,7 @@
  */
#overlay-disable-message {
   background-color: #fff;
-  margin: -20px auto 20px;
+  margin: 0 auto 20px;

Why these changes?

+++ b/core/modules/overlay/overlay-parent.jsundefined
@@ -304,12 +298,14 @@ Drupal.overlay.loadChild = function (event) {
+      // Report these tabbables to the TabbingManger in the parent window.

s/TabbingManger/TabbingManager/

+++ b/core/modules/system/system.moduleundefined
@@ -1415,6 +1415,20 @@ function system_library_info() {
+  // Manages tab orders in the document.

"orders" is too specific (we don't actually manage order) and too vague (it doesn't cover the tabbing contexts.

How about: "Manges tabbing contexts in the document."?

+++ b/core/modules/system/system.moduleundefined
@@ -1415,6 +1415,20 @@ function system_library_info() {
+      array('system', 'jquery.ui.core'),

jQuery UI core is a dependency, but it is not explicitly used. There should be a comment explaining why it is a dependency.

Jesse said the test failures were likely due to edit mode being enabled while running the tests. She was right! After disabling it, all tests pass! :) (That's of course something that needs to be fixed: the host environment shouldn't be able to influence test results, but that's currently acceptable, given that we have zero JS test coverage in Drupal core.)

StatusFileSize
new52.31 KB
new34.15 KB
PASSED: [[SimpleTest]]: [MySQL] 53,919 pass(es).
[ View ]

Wim, thank you for the extensive review in #20. This patch doesn't yet take your comments into account. I've made changes implementing Drupal.announce so that behavioral testing can proceed. I want to know that these changes we're making are useful. Now, when Edit mode is enabled, a screen reader will announce that Edit mode is enabled and that tabbing is constrained to N number of contextual links.

When the Overlay is opened, a screen reader will announce the title of the overlay page and that tabbing is constrained to the administration toolbar and the overlay.

I had to make changes to Drupal.announce to get multiple near-simultaneous calls to announce to all be read. Those changes are isolated and a patch for them can be found here: #1959306: Drupal.announce does not handle multiple messages at the same time; Improve the utility so that simultaneously calls are read..

Because this issue is still needs work, I've included these changes in this patch. They will eventually be removed when the changes to Drupal.announce are committed.

Please follow the instructions in #19 to test this updated patch.

Ignore the interdiff in #22. I hadn't rebased the 19 patch on the latest 8.x head. It's not really important in this case.

Status:Needs work» Needs review
StatusFileSize
new12.6 KB
new14.45 KB
new26.63 KB
PASSED: [[SimpleTest]]: [MySQL] 53,913 pass(es).
[ View ]
new53.83 KB

Wim, thank you for the extensive review in #20. I have gone through all of the comments. The ones that require additional commentary from me are listed below.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+ for (var i = 0, il = this.stack.length; i < il; i++) {
1) il is not declared within this scope.
2) why is this necessary? Isn't this a premature (micro)optimization?

This always catching me off at first, too. The il variable is declared by the same var statement where i is declared. There's a little comma in between. I've seen nod_ writing for loops like this recently, so I'm following the style. It probably optimizes the continue condition evaluation a wee bit.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+ * Restores former tabbing context when an active tabbing context is released.
+ *
+ * Once a TabbingContext object is released, it can never be activated again.
"release" is used here, but the function itself is called "unwind". We should make this consistent.

I moved this line of the comment to the TabbingContext.release() method description.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+ var stackLevelToRestore = this.stack.length;
+ for (var i = this.stack.length -1; i >= 0; i--) {
+ // As soon as a set in the stack is encountered that is not released,
+ // break out of the for loop.
+ if (!this.stack[i].isReleased()) {
+ break;
+ }
+ stackLevelToRestore--;
Flaws:
- line 1: We can only reach this line if the top tabbing context. Note that this variable is initialized to an non-existent level (e.g. if length is 3, then the maximum level is 2, but this would initialize to 3). This can be safely initialized to length minus 2.
- line 2: Same reasoning as above; this is unnecessarily checking whether the top level in the stack is released.

AFAICT this can be simplified to:

var stackLevelToActivate = tabbingContext.level;
while (stackLevelToActivate > 0 && this.stack[stackLevelToActivate].isReleased()) {
stackLevelToActivate--;
}
(This is analogous to the code I was using in #9.)

Wim, I refactored this code so that we don't need a reference to the tabbingContext instance. The TabbingManger just unwinds the stack as far as it can each time _unwind is called. If you want to fiddle with this method, just make sure the tests still pass. Here's the method

_unwindStack: function () {
  // Unwind as far as possible.
  var lastReleasedTabbingContext = this.stack.length;
  for (var i = lastReleasedTabbingContext - 1; i >= 0; i--) {
    // As soon as a tabbingContext in the stack is encountered that is not
    //  released, break out of the for loop.
    if (!this.stack[i].isReleased()) {
      break;
    }
    lastReleasedTabbingContext = i;
  }
  // Delete all tabbingContexts starting at lastReleasedTabbingContext. They
  // have already been deactivated, so their effect on the DOM has been
  // reversed.
  this.stack.splice(lastReleasedTabbingContext);
  // Get topmost tabbingContext, if one exists, and activate it.
  if (this.stack.length && this.stack[lastReleasedTabbingContext - 1]) {
    this.stack[lastReleasedTabbingContext - 1].activate();
  }
}

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,409 @@
+ var $hasFocus = $set.filter('[autofocus]').eq(0);
+ if (!$hasFocus.length > 0) {
+ $hasFocus = $set.eq(0);
This is … strange.

The first line already grabs the first element.

The second line then does a "!length > 0" — IDK what that means.

Seems like a remnant from earlier code :)

This code is first checking for elements with the autofocus attribute. If no element has this attribute, then the first element in the set is used. I updated the comments to reflect this.

+++ b/core/modules/overlay/overlay-child.cssundefined
@@ -53,10 +53,6 @@
-.overlay #skip-link {
- margin-top: -20px;
-}
.overlay #skip-link a {
color: #fff; /* This is white to contrast with the dark background behind it. */
}
@@ -143,7 +139,7 @@

@@ -143,7 +139,7 @@
*/
#overlay-disable-message {
background-color: #fff;
- margin: -20px auto 20px;
+ margin: 0 auto 20px;
Why these changes?

Without these changes, the Overlay dismiss message appears under the Toolbar. This is a holdover issue from the changes made with Drupal.displace. I noticed it when testing tabbing with the Overlay.

Screenshot of the Overlay dismiss message hidden behind the toolbar because it has a negative top margin.

jQuery UI core is a dependency, but it is not explicitly used. There should be a comment explaining why it is a dependency.

jQuery UI Core provides the :tabbable pseudo selector so that we don't need to implement our own. This is the same pseudo selector used in the jQuery UI Dialog to control tabbing. I've added a comment to explain this.

Next Steps

The file tabbing-1913086-24.patch is the proposed patch. I've pulled the patch apart into tabbing-1913086-just-tabbingmanager-do-not-test.patch and tabbing-1913086-implementation-do-not-test.patch so that the introduction of the TabbingManager (which has unit test coverage) and the implementation of the TabbingManager (which does not yet have test coverage) is distinct and can be reviewed easily.

This is the patch I would like us to start considering for commit. The TabbingManager moves code from the Overlay module into a generic utility that we can unit test (done!). Now, all modules can leverage the tabbing management that was formerly locked away in Overlay. The behavior that TabbingManager provides is necessary for the complex in-place edit interactions and unified contextual links in D8. We will certainly find other uses. Just as we got Drupal.announce committed several weeks ago and we continu to improve it through iteration, getting the Drupal.TabbingManager committed will allow us to start using this feature and of course improving it. Accessibility work benefits greatly from incremental improvements because so much of it requires real-world usage to know if the behaviors are as effective as they can be.

The implementation changes in this patch will not be complete, meaning a screen reader won't read all of many near-simultaneous calls to Drupal.announce(), until #1959306: Drupal.announce does not handle multiple messages at the same time; Improve the utility so that simultaneously calls are read. is committed.

The function documentation in #24 is not consistent. I'm waiting on resolution (or at least the scent of resolution) in #1958246: Update JS documentation to follow Doxygen format (remove curly braces) to know how to clean these up.

Status:Needs review» Needs work
Issue tags:-JavaScript, -accessibility, -sprint, -in-place editing, -Spark

The last submitted patch, tabbing-1913086-24.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+JavaScript, +accessibility, +sprint, +in-place editing, +Spark

#24: tabbing-1913086-24.patch queued for re-testing.

StatusFileSize
new3.39 KB
new26.03 KB
PASSED: [[SimpleTest]]: [MySQL] 54,102 pass(es).
[ View ]

#24:

RE: il: I pinged nod_ about this and it's just about guaranteeing a fixed number to count up to. IMO we should only use this when it is really necessary, rather than using it everywhere to ensure that "whatever happens in the loop, it doesn't make things weird". He's going to add it to the JS coding standards at https://drupal.org/node/1778828#comment-7230352.

RE: _unwindStack(): I implemented my changes: IMO the loop is now easier to understand (1 variable instead 2 variables plus a break). I also fixed some JSHint errors.
I also fixed the tests, which had two broken assertions as of #24. There, too, I fixed JSHint errors.
I also discovered another bug in TestSwarm, patch posted: #1964250: Breaks when the POST request to testswarm-test-done fails.

RE: autofocus: I guess I mostly didn't understand the "!length > 0" thing, which seems like it negates length and then applies the greater than operator. Because negation does have higher precedence… I clarified that by changing it to "lenght === 0" :)

RE: jQuery UI Core dependency comment: I don't see the comment in the patch, so I added it myself.

StatusFileSize
new4.39 KB
new25.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch tabbing-1913086-29.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

We're moving away from curly braces in JS Doxygen at #1958246: Update JS documentation to follow Doxygen format (remove curly braces), so I've now also updated TabbingManger's Doxygen.

+++ b/core/misc/tabbingmanager.jsundefined
@@ -0,0 +1,394 @@
+  _unwindStack: function () {
+    // Unwind as far as possible: find the topmost non-released tabbingContext.
+    var tabbingContextToActivate = this.stack.length - 1;
+    while (tabbingContextToActivate >= 0 && this.stack[tabbingContextToActivate].isReleased()) {
+      tabbingContextToActivate--;
+    }
+
+    // Delete all tabbingContexts after the to be activated one. They have
+    // already been deactivated, so their effect on the DOM has been reversed.
+    this.stack.splice(tabbingContextToActivate + 1);
+
+    // Get topmost tabbingContext, if one exists, and activate it.
+    if (tabbingContextToActivate >= 0) {
+      this.stack[tabbingContextToActivate].activate();
+    }

Awesome, much more readable!!

Let's leave this up for review for a couple more days. I'd like to get input from some a11y testers and developers.

#29: tabbing-1913086-29.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+JavaScript, +accessibility, +sprint, +in-place editing, +Spark

The last submitted patch, tabbing-1913086-29.patch, failed testing.

Status:Needs work» Needs review

There needs to be some text describing the Drupal.overlay.releaseTabbing function here:

/**
- * Restore an element's original tabindex.
  *
- * Meant to be used as a jQuery.fn.each callback.
  */

There was also some conflict in core/modules/overlay/overlay-parent.js with a fragment with these functions:
Drupal.overlay.makeDocumentUntabbable
Drupal.overlay.makeDocumentTabbable
Drupal.overlay._recordTabindex
Drupal.overlay._restoreTabindex

Not sure what changed.

StatusFileSize
new12.6 KB
new12.75 KB
PASSED: [[SimpleTest]]: [MySQL] 54,193 pass(es).
[ View ]

reroll & interdiff but not the failed 1 out of 6 hunks /tmp/interdiff-1.Yqefo7.rej

Status:Needs review» Needs work

Nope, that patch doesn't work at all now. It applies nicely, but Overlay is toast and so is the position of the admin menu link.

Hope someone else can push this ahead. Also, something broken as badly as this (No Overlay shows up) really shouldn't pass the tests.

Status:Needs work» Needs review
StatusFileSize
new25.71 KB
PASSED: [[SimpleTest]]: [MySQL] 54,529 pass(es).
[ View ]

Indeed, the patch that upgraded us to jQuery 1.9 caused some troubles with the patch in 29. I've rerolled it. It should work fine now.

Hm. How come http://drupal.org/project/fat didn't pick that up?

Hm. How come http://drupal.org/project/fat didn't pick that up?

This was a simple patch apply problem. 8.x changed sufficiently since the patch in #28 (over just 2 days!) to invalidate the patch. It just needed a reroll.

Ah, ok. :)

Also, I think mgifford meant that this patch should not be able to pass testbot's testing. But testbot doesn't run JS (fat) tests. I understand his frustration though :)

The patch in #36 still applies cleanly; all FAT tests still pass; manual testing shows no problems. Pinging mgifford to give this a final round of testing and hopefully RTBC this one (I can't assign this issue to him, so tagging with "needs accessibility review" instead).

mgifford mentioned to me in chat that if the page state is in Edit Mode, then on subsequent refreshes, the tabbing will be constrained to just the contextual links, but the user will not have any indication that this is the case.

Contextual module needs to raise an announcement on the first tab action if Edit Mode wasn't explicitly enabled by a user action on a page load.

Great point!

StatusFileSize
new6.31 KB
new28.62 KB
PASSED: [[SimpleTest]]: [MySQL] 54,482 pass(es).
[ View ]

mgifford, I put in the changes to trigger the announcement about the edit mode being enabled. On a fresh page load, if the edit mode is sticky, the user will hear the tabbing constraint message as soon as they tab from the first item. I've also added a key handler for the esc key, which will now exit Edit mode as well as clicking on the toolbar toggle.

Unfortunately, this patch only works with the updated Drupal.announce patch: #1959306-8: Drupal.announce does not handle multiple messages at the same time; Improve the utility so that simultaneously calls are read.. We need the enhanced functionality to get the UA to read the message in a complex change situation.

I've started a documentation page here as well: http://drupal.org/node/1973218

Ok, so I should really do a screen capture of this, but let's start with text assumptions.

We have to consider blind people who use screen readers (and can therefore benefit from your alerts) and sighted people who use a keyboard (because they can't use a mouse).

We are assuming that people will scan the page from left to right (although does this change for languages like Arabic & Hebrew that are R2L?).

Given that:
- I didn't hear an alert in VoiceOver when tabbing over the "Edit button"
- I did hear "Edit button pressed" with ChromeVox, but that doesn't tell me that the page navigation is different.
- I didn't see an alert when just tabbing over the "Edit button"

Sighted users will expect the tab navigation to go:
"Home" "Menu" "Shortcuts" "User Name" "Edit button"

and not:
"Home" "Menu" "Shortcuts" "Edit button" "User Name"

This is with this patch & http://drupal.org/node/1959306#comment-7307768

StatusFileSize
new499 bytes

To test #43, we also need this little helper patch to add the drupal.announce library to contextual.

This tangle of patches is one of the reasons I'd like to get these patches in place so we can refine them from HEAD rather than juggling multiple somewhat interdependent patches. We'll get the interaction right over time. I think we agree on that.

- I didn't see an alert when just tabbing over the "Edit button"

I wouldn't expect to see an alert. There's no provision in this patch for launching an alert box.

- I didn't hear an alert in VoiceOver when tabbing over the "Edit button"
- I did hear "Edit button pressed" with ChromeVox, but that doesn't tell me that the page navigation is different.

That's probably because of a JavaScript error. See #45 above.

Sighted users will expect the tab navigation to go:
"Home" "Menu" "Shortcuts" "User Name" "Edit button"

and not:
"Home" "Menu" "Shortcuts" "Edit button" "User Name"

We can adjust the DOM weighting of the tabs, but I'd rather not do that in this issue.

Status:Needs review» Reviewed & tested by the community
Issue tags:-sprint

Thanks @JesseBeach - with the patch in #45 I was able to get this to work as expected with VoiceOver. I'm marking this RTBC although there are going to be a couple follow-up items from the testing.

StatusFileSize
new86.42 KB
new69.69 KB

Oh ya, I should attach screenshots as I took a snapshot of what VoiceOver Says:

Tabbing is no longer constrained screenshotTabbing is constrained screenshot

although there are going to be a couple follow-up items from the testing.

We'll get them fixed up, no doubt!

Let's get them logged now so we don't lose them. What specifically did you note?

Status:Reviewed & tested by the community» Needs work

Feedback coming.

Few things:

In tabbingmanager.js:

  1. Drupal.TabbingManager: name is misleading, it's not a constructor it's an object, not a capital T.
  2. Drupal.behaviors.drupalTabbingManager is not doing what it's supposed to.
  3. Since Drupal.TtabbingManager is unique, i'd much rather have the process information in there and not on the behavior object (so removing tabbingManagerProcessed and adding processed to TabbingManager)
  4. constrain(set); set is strange to use. I'd be more comfortable with elements or something. and in parties it'll be confusing: "— See that bit of code constrains to 'set' — No! you're setting the constrain". Fun ahead!
  5. in _unwindStack naming of tabbingContextToActivate is overly verbose. Can't toActivate work?
  6. _recordTabindex/_restoreTabindex I'd put the loop outside of them. They should be acting only on one element, they don't need context. The loop is distracting.
  7. In _restoreTabindex. It'll always be true, no need to check tabInfo. Or || {} needs to go away.
          var tabInfo = $el.data('drupalOriginalTabIndices') || {};
          if (tabInfo && tabInfo[level]) {
  8. TabbingContext: getLevel, getDisabledElements, setDisabledElements, getTabbableElements, isReleased, isActive those methods are just overhead. The temp variables can be created just as well from object properties. We're not making the DOM the API doesn't need to be verbose.
  9. I saw that in other Backbonned files but the whole onRelease, onXX I have a hard time with it. Smells like browser-war nostalgia or something. On that note the whole onRelease, onActivate is a lie. It's not the context that do anything it's the manager. so from:

      activate: function () {
        // A released TabbingContext object can never be activated again.
        if (!this.active && !this.released) {
          this.active = true;
          var args = Array.prototype.slice.call(arguments);
          args.unshift(this);
          this.onActivate.apply(null, args);
          // Allow modules to respond to the constrain event.
          $(document).trigger('drupalTabbingContextActivated', this);
        }
      },

    to

      activate: function () {
        // A released TabbingContext object can never be activated again.
        if (!this.active && !this.released) {
          this.active = true;
          // Currently that's the _activateTabbingContext method, that should be renamed too
          Drupal.tabbingManager.activate(this);
          // Allow modules to respond to the constrain event.
          $(document).trigger('drupalTabbingContextActivated', this);
        }
      },

    And now it's clear who does what. This whole onXX adds a level of complexity that's not needed. And we don't loose any flexibility because it's still possible to override what activate() means, globally and for individual stacks.

Status:Needs work» Needs review
StatusFileSize
new13.23 KB
new25.04 KB
PASSED: [[SimpleTest]]: [MySQL] 54,604 pass(es).
[ View ]

patch with the changes above. Still working as far as I can tell. removed somewhere around 90 lines.

If it's a problem to have tight coupling between the tabbingcontext object and Drupal.tabbingManager a good place to start is have them in separate files :D

StatusFileSize
new1019 bytes
new26.08 KB
PASSED: [[SimpleTest]]: [MySQL] 54,458 pass(es).
[ View ]

This might be the first time that FAT module tests have helped me out in a big way. I was able to establish that functionality has been regressed and THEN figure out exactly where. The diff to get the tests to pass was small: I changed a variable name and added a function call.

Otherwise, awesome changes nod_. You've got a magic touch! It's a lot cleaner now. If I wasn't the last person to touch it, I'd RTBC it :)

StatusFileSize
new2.58 KB
new25.22 KB
PASSED: [[SimpleTest]]: [MySQL] 54,474 pass(es).
[ View ]

Awesome clean-up, Théodore! :) Thank you!

What was still forgotten though, was to also make the docs reflect that new, simpler, leaner, meaner reality.

All right, consider that RTBC for me.

Thanks for the doc work and for catching my mistake too :)

Status:Needs review» Reviewed & tested by the community

and back to RTBC :)

If #1959306: Drupal.announce does not handle multiple messages at the same time; Improve the utility so that simultaneously calls are read. gets committed first, then I need to make a small adjustment to this patch to include drupal.announce as a dependency of Contextual and Overlay. Otherwise the JS for these two modules won't load.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Awesome accessibility improvement.

Hurray! :)

Status:Fixed» Active
Issue tags:+Needs change record, +sprint

A change record for this should be created; this is a new API that Drupal developers should be aware of!

Title:Generalize the overlay tabbing management into a utility library[Change notice] Generalize the overlay tabbing management into a utility library

Title:[Change notice] Generalize the overlay tabbing management into a utility libraryGeneralize the overlay tabbing management into a utility library
Status:Active» Closed (fixed)
Issue tags:-needs accessibility review, -Needs change record, -sprint

Change notice created: https://drupal.org/node/2014545.

Issue summary:View changes

added a link to tests