Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Core JavaScript is shifting to utilizing the Backbone framework where appropriate.
Comment | File | Size | Author |
---|---|---|---|
#36 | contextual-toolbar.patch | 484 bytes | tim.plunkett |
#29 | 21-28-interdiff.txt | 1.92 KB | alexpott |
#28 | contextual-backbone-1971108-28.patch | 37.86 KB | Wim Leers |
#27 | revert_accident-1971108-27.patch | 2.03 KB | Wim Leers |
#21 | contextual-backbone-1971108-21.patch | 40.74 KB | jessebeach |
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedComment #2
Wim LeersIMHO this patch should also make it possible to have "dynamic" AKA "client-side/JS only" contextual links, to allow for very dynamic interactions like Edit module's.
Essentially, I want to get rid of this hacky, ad-hoc solution:
And come up with a general solution. I see two possible ways to do that:
The first is hard to reconcile with the general architecture of contextual links, which is: "contextual links are generated automatically based on menu items marked as local tasks for the provided parent path". That's why I think the second is a better candidate.
This is necessary for #1966704-13: In-place editing for taxonomy terms and custom blocks.
Comment #3
Wim LeersReviewing thoroughly + rerolling.
Comment #4
Wim LeersReview of patch in #1. Overall: wow, this is already (mostly) working perfectly! :)
Size delta
Tested with http://refresh-sf.com/yui/
Review
element-focusable
class) until edit mode is enabled. Without edit mode, tabbing from one contextual link to the next is a very painful experience anyway (you have to tab *way* too much). Related: I think there should be a shortcut to trigger edit mode.Changes in attached reroll
.contextual.contextual-active { z-index: 501; }
made the contextual triggers (pencils) appear on top of the overlay. But this problem also existed before: #1910032: Contextual gears do not dismiss when a UA is touch-enabled and the overlay opens.Drupal.contextual.Model.regionIsHovered
(instead ofisActive
). "hovered", not "active", because on touch devices it is impossible to listen for hovering on the region.mouseenter
/mouseleave
events on the trigger versus on the region used to both affect were being applied toisActive
. Now they affect two different states:regionIsHovered
andhasFocus
. The "hasFocus" name makes a lot sense once you've taken #1914976: Dashed outlines around contextual region should only appear upon hover/focus of the pencil icon or links into account.// Hide all nested contextual triggers while the links are shown for this one.
— probably because this breaks the encapsulation. To do this in a non-DOM-dependent way, we need at least some level of one contextual link being aware of other contextual links. This introduces a whole new level of complexity, so I decided to keep doing this in the same old way — via the DOM. See the bottom ofDrupal.contextual.View.render()
..contextual-links a
), the contextual links were no longer being closed.contextual.js
now depends on Modernizr to detect touch devices.interdiff.txt
for all of the above.interdiff-dynamic_contextual_links.txt
.Also: We should write tests!
Attached patch size delta
Naming
Finally: I think we should revise the naming. What is a "contextual link"? Is it the whole? Is it one of the actions/options that are provided?
This becomes even more confusing when you start looking at the code! Well-defined structured concepts would immediately make this entire codebase (both CSS and JS) far more approachable.
An initial proposal:
<div class="contextual"><button class="trigger"></button><ul class="contextual-links"></ul></div>
. That means the top-level div would get acontextual-menu
class. One can say "it is a contextual menu", one cannot say "it is a contextual".<button class="trigger"></button>
<ul class="contextual-links"></ul>
. The total structure would be<div class="contextual-menu"><button class="trigger"></button><ul class="links"></ul></div>
. The word "contextual" is already in the containing div, then why do we need to repeat it again here?Comment #5
jessebeach CreditAttribution: jessebeach commentedThis is a great refactoring of #1. It's succinct and quite clever in places.
The attached patch updates some commenting styles on methods to match standards. I fixed some CSS styling as well.
The one thing that the patch in #4 removed was the dismissal of the contextual link list and the active trigger when a user tabs off the list. I added this back in and provided an interdiff that highlights the changes (blur-timeout-1971108-5-do-not-test.patch). Wim and I went back and forth on this in chat. I wanted to propose the code and see if we can live with a few more lines to continue existing behavior.
I tested this patch on ChromeVox and the spoken UI remains unchanged. Smoke testing the behaviors for mouse and tabbing also confirms that this patch does not regress them.
I think this patch is ready to go in (unless Wim or anyone else has further comment).
Responses to statements in #4
I'm a touch reluctant to hide contextual link triggers behind a click for keyboard users but leave them available so immediately to mouse users. In my opinion the interaction should be kept as similar as possible in terms of discoverability.
I wholeheartedly agree. We need to get away from using the region as a container that has any meaning or scope. It's there purely as a placement target for blocks.
Comment #6
Wim Leers#5 looks good to go to me, with the exception of the changes to the event bindings. We *must* exclude the hover events, because otherwise we require users to tap *twice* on the contextual links trigger on touch-only devices.
So, I looked for a solution to the problem of "touch AND mouse both" devices. I found:
I read all of them. The conclusion is: there is no good solution yet. For our use case, media queries level 4 will bring salvation. But they're not yet widely supported.
So, in the mean time, I think it's better to support the majority well (touch devices) rather than breaking the majority to support the minority (touch + mouse devices).
Unless you have magical insights here, I think the only way to deal with this for now is to bring back the changes I made :(
Comment #7
tkoleary CreditAttribution: tkoleary commentedI have a potentially magical insight which involves introducing a new interaction pattern but I need to actually prototype and test it first. Also I think it will be a real implementation challenge.
I'm calling it proximity hover. It basically works like this:
I understand that this is expensive but I think it can be done and it's worth exploring since the resulting experience would be amazingly fluid.
Comment #8
Wim Leers#7: woah! Very interesting, out-of-the-box thinking!
However, I think what you describe would need as many lines of code as the entirety of Contextual Links' JavaScript… :) It would also indeed be a battery drain most likely.
Plus, what you're saying does not actually solve the problem at hand: the fact that "hover" is supported on touch devices, but causes a crappy experience. What you describe, requires us to use "hover" on non-touch devices and "touchstart" on touch devices. We can reliably detect "touch" devices, but we cannot reliably detect devices that have both an actual mouse/trackpad *and* touch. Not to mention that you still don't know which of the two the user is using then. Worse, the user can even switch between the two.
I think the easiest way forward is to do what I suggested in #6:
Comment #9
tkoleary CreditAttribution: tkoleary commentedAdditional note, after reading the last link in #6 I am thinking about how to create a unified input-agnostic interaction pattern that combines the above with some adjustment of current mouse event behaviors.
Comment #10
tkoleary CreditAttribution: tkoleary commentedWim Leers,
I see. That makes sense. I'll add this one to the D9 ideation backlog. :)
Comment #11
jessebeach CreditAttribution: jessebeach commentedIf we start removing functionality based on the presence of Touch, then we're going to knock out functionality on devices like the Chromebook Pixel which supports Mouse and Touch interaction, but because of its form factor, seems to encourage Mouse interactions over Touch. I expect we'll only see more devices in the future that support both. This snippet of code from #4 would knock out hover on a Pixel, making contextual links unreachable with a mouse.
From the hack.mozilla.org article you linked to, it seems we really only need to be worried about touchend/click events. The mouseenter/mouseleave enters shouldn't bother us on touch devices. Am I misunderstanding or is there some bad combo of interaction that makes us want to knockout mouseenter/mouseleave on devices that also support touch? It seems we'll be leaving a gap in interactivity if we take this step.
https://hacks.mozilla.org/2013/04/detecting-touch-its-the-why-not-the-how/
Comment #12
Wim LeersYes, you are misunderstanding :)
As soon as you listen to hover/mouseenter on a touch device, you're going to force touch device users to tap *twice*. The first tap registers as a the hover/mouseenter, the second registers as a click.
So, the code example you cite is only relevant to improve the "clicking" experience on touch devices. That's worthwhile to do, of course.
That article does not offer any guidance on hover events, precisely because that is still under discussion, because there is no good way to handle that at all yet:
(emphasis mine)
So, hover-detection/handling is:
This strengthens me in my conviction set forth in #6 that we are not going to solve this in this small issue.
As per the above, in this reroll:
Drupal.contextual.RegionView.events
(consequence: on touch + mouse devices, one *must* use the edit mode toggle)Drupal.contextual.View
. The consequence is that in your patch, hovering the contextual trigger no longer causes it to get focus and thus to get the outlines. Bojhan specifically said he wanted it to work this way. Restored that. I kept the per-link focus/blur handlers that you added though.this.timer
inDrupal.contextual.View.initialize()
again (as in #1), it is now explicitly doc'd again.Points 1–3: see
interdiff.txt
. Bullet 4: seeinterdiff-better_clicking_on_touch.txt
.Comment #13
jessebeach CreditAttribution: jessebeach commentedWim, thank you for the explanation in #12. It does seem we're in an intractable situation regarding devices that support both touch and mouse events. This is unfortunate. We'll have to revisit these scripts in the future when the number of combo input devices increases and the technical ability to detect the hover feature is available.
These changes are a great win for simplification and maintainability and manual testing confirms that we've got all previous behavior in place. I'm marking RTBC.
Comment #14
jessebeach CreditAttribution: jessebeach commentednod_ has expressed reservations in chat. I'm setting this back to needs work until we address his comments. Either he will post his comments here or I will summarize them with a reroll.
Comment #15
Wim LeersAlso didn't apply anymore since #1943776: In-place editors (Create.js PropertyEditor widgets) should be loaded lazily just went in.
nod_'s reservations in chat were about model instances, collections and lists of views being set on
Drupal.behaviors.contextual
, instead of onDrupal.contextual
. This reroll fixes that.Comment #16
nod_not passing JSHint:
core/modules/contextual/contextual.toolbar.js: line 190, col 52, Missing semicolon.
Few details I want to change, moving code around, refactoring a few function calls this kind of things. I'll get to it this afternoon.
Comment #17
Wim Leers#16: That JSHint fail was not introduced by this patch, but I'll fix it :)
Comment #18
Wim LeersFixed #16.
I split up the single
View
into aVisualView
, anAuralView
and aKeyboardView
. Then if new screen reader features pop up, or we want it to behave differently, it's super easy to modify. I made zero logic changes, I only restructured. The end result is now 1495 bytes minified + gzipped.This also moved the rather ugly, but essential timer stuff to deal with tabbing off of an opened contextual link when edit mode is disabled from the
Model
(where it could be considered "polluting") to theKeyboardView
, where it can easily be modified without interfering with use cases where it is not necessary at all. (And in case we'd like to drop it, we'd lose 52 bytes.)All of the above is for the sake of making a step in the direction of code patterns to handle accessibility in a structured way.
Finally, I moved the Edit module modifications out of this patch (introduced in #4: http://drupal.org/files/interdiff-dynamic_contextual_links.txt) and into a patch at a newly created issue: #1981760-1: Edit should use the new drupalContextualLinkAdded event to cleanly instead of hackily insert its "Quick edit" contextual link.
Comment #19
nod_Moving code around, generally un-nesting code, removing redundant checks and uneeded overriding options (there are some things that you shouldn't override and be better off just rewriting the file).
Check the interdiff I haven't touched the logic or what the script does, just making things easier to read hopefully.
Comment #20
Wim LeersThanks nod_, very good refactoring :)
I only want to make very minor changes to your changes in
contextual.js
, attached in reroll:initContextual()
to the top of the file.So far, we had specifically avoided making changes to
contextual.toolbar.js
, but if the JS maintainer himself is introducing them, then we might as well go the whole way :)Hence I also changed this in
contextual.toolbar.js
:contextual.toolbar.js
didn't support these two scenarios yet, but now it does: 1) upon page load, zero contextual links exist on the page, post-page load they are loaded (edit mode toggle would never appear) 2) upon page load, >=1 contextual links exist on the page, post-page load they are all removed (edit mode toggle would never disappear).touchend
) support for edit mode toggle, identical code to the one incontextual.js
.drupalEditModeChanged
DOM event, instead I'm now updatingDrupal.contextual.collection
directly. Faster.Drupal.contextualToolbar.collection
, which was introduced by #19 but was unused and useless. There's only ever going to be one model, because there's only ever going to be one toolbar tab for contextual.module.AuralView
, cfr. #18.(All of these changes are facilitated by the work we've done in this issue on
contextual.js
.)Comment #21
jessebeach CreditAttribution: jessebeach commentedI love the VisualView, AuralView, KeyboardView breakdown. It finally pulls apart mixed-up initialization and render functions that mashed together these disparate interaction modes.
I made a few docs updates and fixed a small regression. When tabbing through the contextual links, a tab from a link to another link (not from the trigger to a link) dismissed the list of links. I learned something about Backbone in the process. We can't attach a single handler to multiple triggers using a comma-delimited list like this.
'focus a, focus button'
The
'focus a'
will trigger, but'focus button'
will not. I verified this by setting a conditional breakpoint in the handler that triggered on everything but the first declared event. This was the code before.And after
I added periods to the following sentences because they get read on run-ons without them. We had been concatenating them with a period before, but now we use newlines. I noticed this while putting together our Drupalcon Portland examples videos last night.
Someone should give the diff here a quick breeze through and verify my tabbing fix. Otherwise, we're ready to commit. Excellent work everyone.
Comment #23
nod_#21: contextual-backbone-1971108-21.patch queued for re-testing.
Comment #24
nod_no reasons that it doesn't come back green.
Comment #25
webchickI tested this pretty thoroughly tonight.
- Ran into #1983164: Entity Forms in ajax requests don't find the route on the "Edit view" contextual link, but that's a pre-existing issue.
- Did NOT run into #1960490: Random "flicker" with contextual links, depending on mouse direction when clicking (true story!), which was very nice.
- Did NOT run into #1910032: Contextual gears do not dismiss when a UA is touch-enabled and the overlay opens either, which was also very nice.
- Also didn't run into the problem related to that one which I can't find an issue for, where contextual links never shut off on touch devices.
The resulting code looks very clean and well-commented. Well done!
Committed and pushed to 8.x. Thanks!
Does this need a change notice? I didn't see much in the way of other module changes outside of Edit module, so I wasn't sure.
Comment #26
Wim LeersThanks!
No, it won't need a change notice IMHO :)
Comment #27
Wim LeersUnfortunately http://drupalcode.org/project/drupal.git/commitdiff/aad2d4a660ee6c477af0... contains changes to edit.module that weren't part of this patch, but were part of the #1962606-15: Quick Edit + "Field" Views (table, grid …) patch.
Attached patch reverts that accidental part of the commit. The rest is all good.
EDIT: apparently jessebeach's reroll at #21 introduced the edit.module hunk. Not webchick!
Comment #28
Wim LeersSo, http://drupalcode.org/project/drupal.git/commitdiff/aad2d4a660ee6c477af0... should be reverted and this patch (which is #21, minus the edit.module hunk) should be committed instead. Thanks in advance!
Comment #29
alexpottHere's the interdiff of 21 to 28
Comment #30
Wim LeersI didn't think it necessary because it really was just removing the edit.module hunk. Apologies — I'll provide an interdiff next time.
Comment #31
nod_Looks good.
Comment #32
alexpottCommitted 8498adc and pushed to 8.x. Thanks!
Comment #33
webchickD'oh, really sorry for the trouble folks. :(
Comment #34
Wim LeersNo worries :) We're all just humans!
Comment #35
Wim LeersPlus, this didn't *break* anything. Just an unnecessary, unrelated hunk that got committed :)
Comment #36
tim.plunkettI'm not sure what the difference between contextual-links and contextual-toolbar are, but with #111715: Convert node/content types into configuration applied, I'm getting:
This fixes it, since Drupal.contextual.collection is defined in the other file.
Comment #37
Wim LeersYou're absolutely right. I'd swear I once had that. Thanks!
Comment #38
Wim LeersI have that same hunk in the patch for #914382: Contextual links incompatible with render cache, so if this goes in first, I'll have to reroll the patch there.
Comment #39
Wim Leers#36: hrm, an alternative solution has been posted at #1988328: Contextual link JavaScript error when viewing a page such as /admin/* that doesn't contain any contextual links. Let's discuss it there!