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
- Propose a reviewable patch.
- 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
Comment | File | Size | Author |
---|---|---|---|
#53 | tabbingmanager-1913086-53.patch | 25.22 KB | Wim Leers |
#53 | interdiff.txt | 2.58 KB | Wim Leers |
#52 | core-js-tabbingmanager-1913086-52.patch | 26.08 KB | jessebeach |
#52 | interdiff_51-to-52.txt | 1019 bytes | jessebeach |
#51 | core-js-tabbingmanager-1913086-51.patch | 25.04 KB | nod_ |
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedThis is a first try at extracting the tab management from overlay. There are a still a few threads to sever.
Comment #2
jessebeach CreditAttribution: jessebeach commentedOk, 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.
Comment #3
mgiffordThis is a great initiative. I'll try to get more people involved.
Comment #4
Wim LeersThis 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.:
Comment #5
Wim LeersShouldn'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?
Comment #6
Wim Leers.
Comment #7
Wim LeersJesse asked me to work on this.
Comment #8
jessebeach CreditAttribution: jessebeach commentedI 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
Comment #9
Wim LeersChanges/notes
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.
Todos/discussion points
Comment #10
mgiffordThe 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
Comment #11
mgifford#9: tabbing-1913086-9.patch queued for re-testing.
Comment #12
jessebeach CreditAttribution: jessebeach commentedI refactored the code into a TabbingManager class and a TabbingContext class and removed the closure-scoped variables.
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.
Comment #13
nod_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.
Comment #14
Wim LeersFirst 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.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.)window
anddocument
are not yet listed in the file-level closure.Why do we detect these if we don't do anything with it?
s/TabbingManager/TabbingContext/Manager/ ?
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?
There's a new undocumented parameter: "id".
Why not *require* an id? It'd make debugging a lot easier, and it's not a lot to ask for.
You renamed
$tabbable
to$disabledTabbingSet
— THANK YOU! That's much, much better :) Glad to be rid of that very confusingly named variable from Overlay!What's "fresh" about this?
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")!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!
*HUH?*
1) Do we really want to be handling a browser-level behavior?
2) Do we really want to be specifically setting background colors?
1) Seems unrelated?
2) Does not get called?
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.)Why do we detect these if we don't do anything with it?
parent
object is not listed in file-level closure.You mean the parent frame, not the child frame, right?
Why a
switch
statement if you're only checking for one event code, and AFAIT, only ever will check for one event code?Excessive whitespace?
Why not have
constrainTabbing()
callreleaseTabbing()
automatically?I think this is an unrelated change?
'group' => JS_LIBRARY
Comment #15
webchickTalking 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)
Comment #16
sunWhich team? Please note that this does not come across inclusive; it sheds an exclusive/elitist light on the Drupal core queue.
Comment #17
webchickSorry, 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.
Comment #17.0
webchickUpdated issue summary.
Comment #18
jessebeach CreditAttribution: jessebeach commentedTests for this utility are being developed in the FAT module: http://drupalcode.org/project/fat.git/blob/HEAD:/tests/drupal.tabbingman...
Comment #19
jessebeach CreditAttribution: jessebeach commentedI'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.After enabling edit mode, only the contextual link triggers and the edit mode toggle should be reachable via the tab key.
Select the configure block contextual link for the Search block to launch the Overlay.
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 aTabbingContext
object for each set of tabbables: one for the set of contextual triggers and one for the links in the Overlay and Toolbar. TheTabbingManger
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'sTabbingContext
instance is released, the ContextualTabbingContext
instance is reactivated.The
TabbingManger
utility has complete test coverage in the FAT module: http://drupal.org/project/fatWim, there's one comment in #14 that I haven't addressed yet:
I hope the rest of your comments are addressed in this refactor.
Next steps
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.TabbingManager
needs something like areleaseAllSets
method to get back to the default document tabbing.Comment #20
Wim LeersUsage 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
This is misleading. There are many more methods, and even in this function we use more (
deactivate()
andactivate()
at the very least).Why not just say "A TabbingContext object."?
Extraneous newline.
1)
il
is not declared within this scope.2) why is this necessary? Isn't this a premature (micro)optimization?
andSelf()
is deprecated since jQuery 1.8.http://jquery.com/upgrade-guide/1.9/#addback-selector-replaces-andself-
s/current top/current height/
s/on top tabbingContext/on top of the stack/
Are we really getting a "fresh set" here? We're simply assigning the
$set
we retrieved just a few lines higher?(nitpick) Inconsistent phrasing wrt earlier comments.
Activate the tabbingContext; this will manipulate …
"release" is used here, but the function itself is called "unwind". We should make this consistent.
s/unnwind/unwind/
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:
(This is analogous to the code I was using in #9.)
s/_activateSet/_activateTabbingContext/
(same for the deactivate sister)
?
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 :)
Extraneous newline.
See other "il" remark.
s/have had/will have/
?
Missing trailing period.
s/constrain/unconstrain/
Remove "and create a new one".
This part of the comment is inaccurate.
New comment above: "Create a new tabbing context when edit mode is enabled."
(Or something along those lines.)
Why these changes?
s/TabbingManger/TabbingManager/
"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."?
jQuery UI core is a dependency, but it is not explicitly used. There should be a comment explaining why it is a dependency.
Comment #21
Wim LeersJesse 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.)
Comment #22
jessebeach CreditAttribution: jessebeach commentedWim, 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.
Comment #23
jessebeach CreditAttribution: jessebeach commentedIgnore 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.
Comment #24
jessebeach CreditAttribution: jessebeach commentedWim, 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.
This always catching me off at first, too. The
il
variable is declared by the same var statement wherei
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.I moved this line of the comment to the TabbingContext.release() method description.
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
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.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.
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 intotabbing-1913086-just-tabbingmanager-do-not-test.patch
andtabbing-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.
Comment #25
jessebeach CreditAttribution: jessebeach commentedThe 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.
Comment #27
jessebeach CreditAttribution: jessebeach commented#24: tabbing-1913086-24.patch queued for re-testing.
Comment #28
Wim Leers#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.
Comment #29
Wim LeersWe'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.
Comment #30
jessebeach CreditAttribution: jessebeach commentedAwesome, 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.
Comment #31
mgifford#29: tabbing-1913086-29.patch queued for re-testing.
Comment #33
mgiffordThere needs to be some text describing the Drupal.overlay.releaseTabbing function here:
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.
Comment #34
mgiffordreroll & interdiff but not the failed 1 out of 6 hunks /tmp/interdiff-1.Yqefo7.rej
Comment #35
mgiffordNope, 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.
Comment #36
jessebeach CreditAttribution: jessebeach commentedIndeed, 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.
Comment #37
webchickHm. How come http://drupal.org/project/fat didn't pick that up?
Comment #38
jessebeach CreditAttribution: jessebeach commentedThis 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.
Comment #39
webchickAh, ok. :)
Comment #40
Wim LeersAlso, 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).
Comment #41
jessebeach CreditAttribution: jessebeach commentedmgifford 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.
Comment #42
Wim LeersGreat point!
Comment #43
jessebeach CreditAttribution: jessebeach commentedmgifford, 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
Comment #44
mgiffordOk, 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
Comment #45
jessebeach CreditAttribution: jessebeach commentedTo 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.
Comment #46
jessebeach CreditAttribution: jessebeach commentedI wouldn't expect to see an alert. There's no provision in this patch for launching an alert box.
That's probably because of a JavaScript error. See #45 above.
We can adjust the DOM weighting of the tabs, but I'd rather not do that in this issue.
Comment #47
mgiffordThanks @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.
Comment #48
mgiffordOh ya, I should attach screenshots as I took a snapshot of what VoiceOver Says:
Comment #49
jessebeach CreditAttribution: jessebeach commentedWe'll get them fixed up, no doubt!
Let's get them logged now so we don't lose them. What specifically did you note?
Comment #50
nod_Feedback coming.Few things:
In tabbingmanager.js:
Drupal.TabbingManager
: name is misleading, it's not a constructor it's an object, not a capital T.Drupal.behaviors.drupalTabbingManager
is not doing what it's supposed to.TtabbingManager is unique, i'd much rather have the process information in there and not on the behavior object (so removingtabbingManagerProcessed
and adding processed toTabbingManager
)constrain(set);
set is strange to use. I'd be more comfortable withelements
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!_unwindStack
naming oftabbingContextToActivate
is overly verbose. Can'ttoActivate
work?_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._restoreTabindex
. It'll always be true, no need to check tabInfo. Or|| {}
needs to go away.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:to
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.
Comment #51
nod_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
Comment #52
jessebeach CreditAttribution: jessebeach commentedThis 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 :)
Comment #53
Wim LeersAwesome clean-up, Théodore! :) Thank you!
What was still forgotten though, was to also make the docs reflect that new, simpler, leaner, meaner reality.
Comment #54
nod_All right, consider that RTBC for me.
Thanks for the doc work and for catching my mistake too :)
Comment #55
nod_and back to RTBC :)
Comment #56
jessebeach CreditAttribution: jessebeach commentedIf #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.
Comment #57
Dries CreditAttribution: Dries commentedCommitted to 8.x. Awesome accessibility improvement.
Comment #58
Wim LeersHurray! :)
Comment #59
Wim LeersA change record for this should be created; this is a new API that Drupal developers should be aware of!
Comment #60
jessebeach CreditAttribution: jessebeach commentedComment #61
Wim LeersChange notice created: https://drupal.org/node/2014545.
Comment #61.0
Wim Leersadded a link to tests
Comment #62
mgiffordShould the Change notice be updated now that Overlay is no longer in Core?
Comment #63
mgifford