Convert contextual.js to use Backbone (and support dynamic contextual links)

IMHO 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:

   // Add "Quick edit" links to all contextual menus where editing the full
   // node is possible.
   // @todo Generalize this to work for all entities.
-  $('ul.contextual-links li.node-edit')
+  $('ul.contextual-links').find('li.node-edit, li.taxonomy-edit, li.custom-block-edit')
   .before('<li class="quick-edit"></li>')

And come up with a general solution. I see two possible ways to do that:

1. Provide a (server-side) hook to insert a "hidden placeholder contextual link", which the JS can then fill in.
2. Provide a (client-side) event that allows other JS to insert a contextual link.

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.

Reviewing thoroughly + rerolling.

Review of patch in #1. Overall: wow, this is already (mostly) working perfectly! :)

Size delta

Tested with http://refresh-sf.com/yui/

• Minified bytes: 3620 vs. 4248; 17% increase
• Minified+gzipped bytes: 1207 vs. 1403: 16% increase (when aggregated this delta would become lower, since more Backbone-typical strings would consume no additional space)

Review

• Nice to see that tricky solutions for the old contextual JS like http://drupalcode.org/project/drupal.git/commitdiff/577a3ef841182475b6a6... are no longer necessary here, since per-instance state is maintained :)
• On tabbing: I think we should consider making the contextual link triggers unavailable for tabbing (remove the 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

CSS

All CSS significantly simplified; it now no longer attemps to duplicate logic implemented in a more refined manner in JS. CSS is now *solely* presentation instead of also behavior. Which only makes sense, given that contextual links don't work at all with JS.

The above also fixed the problem of the contextual triggers (pencils) being always visible on touch devices, instead of only when edit mode is enabled. I believe this was introduced some time ago as a temporary measure; thanks to edit mode that's no longer necessary now. So this is a significant improvement for Drupal 8 on touch devices. This solves #1973462: Contextual links constantly show up on touch devices.

Brought back the dotted outline upon hovering the region, but now it's better/smarter. It follows what Bojhan has said in #1914976: Dashed outlines around contextual region should only appear upon hover/focus of the pencil icon or links. And made it work on mobile (touch devices).

The patch in #1 added .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.

JS — concepts/structure

There's a 1:1 relationship between contextual regions and contextual links. It doesn't make sense to separate their models, as was the case in #1. Brought that back to a single model. Region hovering tracking is now done in Drupal.contextual.Model.regionIsHovered (instead of isActive). "hovered", not "active", because on touch devices it is impossible to listen for hovering on the region.

The mouseenter/mouseleave events on the trigger versus on the region used to both affect were being applied to isActive. Now they affect two different states: regionIsHovered and hasFocus. 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.

The timer handling was necessary for 2 reasons: 1) focus/blur event handling on the individual link level, 2) "hover intent". Thanks to simplification, I managed to get rid of the first reason of existence of the timer. The second I also got rid of, for the sake of simplification. As long as you hover away *within* the contextual region, the contextual links will now stay open; that's a lot simpler code-wise (i.e. leverage model state instead of timers + event metadata). I'd argue that if we *really* want to have hover intent, that that should be a follow-up issue, because that indeed requires timers, and timers bring a lot of inherent complexity with them.

I moved a lot of logic from the event handlers in the BB View onto the BB Model. They're very short, and we just call them from the event handlers; this makes the code a *lot* more understandable.

All this simplification solved extremely weird edge cases like this one: #1960490: Random "flicker" with contextual links, depending on mouse direction when clicking (true story!).

JS - unhandled edge cases

Regression in #1: // 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 of Drupal.contextual.View.render().

Regression in #1: when triggering a contextual link action/operation (.contextual-links a), the contextual links were no longer being closed.

Mouse hover event handlers (the one for the region and for contextual trigger) should not ever be registered on touch devices. Contextual module never did this correctly; but this patch finally fixes it. contextual.js now depends on Modernizr to detect touch devices.

Many more small, subtle edge cases.

See interdiff.txt for all of the above.

JS — dynamic (JS-inserted) contextual links

This is #2.

See interdiff-dynamic_contextual_links.txt.

Also: We should write tests!

Attached patch size delta

• Minified bytes: 3620 vs. 3704; 2% increase
• Minified+gzipped bytes: 1207 vs. 1343: 11% increase

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:

entity context

For what we currently call the "contextual region". How can one associate actions with a "region"? Can you delete or edit a region? No, you delete or edit a *thing*, and just about everything is and will be an entity in Drupal 8. Hence contextual.module is about "providing (contextual) links for an entity", i.e. "relevant links within an entity context".

contextual menu

For the whole: the <div class="contextual"><button class="trigger"></button><ul class="contextual-links"></ul></div>. That means the top-level div would get a contextual-menu class. One can say "it is a contextual menu", one cannot say "it is a contextual".

trigger

For the gear/pencil icon element. Unchanged. <button class="trigger"></button>

links

For the actual links. What today is <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?

#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:

• https://github.com/Modernizr/Modernizr/issues/770
• https://github.com/Modernizr/Modernizr/issues/548
• https://github.com/Modernizr/Modernizr/issues/869
• https://github.com/Modernizr/Modernizr/issues/873
• https://hacks.mozilla.org/2013/04/detecting-touch-its-the-why-not-the-how/

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 :(

I 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:

1. first we use js to traverse the dom, find all elements that trigger a hover event, and calculate the absolute position of those elements
2. For touch devices contact with the screen *anywhere* will suggest some kind of intent.
3. If the point of contact is a button or a link it will behave as expected
4. If the point of contact is *not* a button or link we calculate the absolute position of the point of contact
5. we compare the point of contact to the list of hover elements and hover the closest one
6. As long as the point of contact is maintained the hover persists, so the user can slide to the thing that is hovered and release on a link to trigger it
7. If the user slides away from a hover to a position where another hover is now closer we trigger mouseout for the first and hover the second

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.

#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:

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).

Additional 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.

Wim Leers,

I see. That makes sense. I'll add this one to the D9 ideation backlog. :)

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.

Yes, 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:

Current discussions […] now revolve around […] whether or not a particular browser/device supports things like hovering. And even beyond JavaScript, similar concepts (pointer and hover media features) are being proposed for Media Queries Level 4. But the principle is still the same: as there are now common multi-input devices, it’s not straightforward (and in many cases, impossible) anymore to determine if a user is on a device that exclusively supports touch.

(emphasis mine)

So, hover-detection/handling is:

1. currently under discussion
2. being proposed for media queries level 4
3. hard, often impossible to detect if a user is on a touch-only device

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:

1. restored code of #4 for Drupal.contextual.RegionView.events (consequence: on touch + mouse devices, one *must* use the edit mode toggle)
2. I'm not sure if it was accidental or intentional, but in #5, you removed the "mouseenter" handler for 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.
3. explicitly set this.timer in Drupal.contextual.View.initialize() again (as in #1), it is now explicitly doc'd again.
4. improved "clicking" experience on touch devices based on the snippet you posted above

Points 1–3: see interdiff.txt. Bullet 4: see interdiff-better_clicking_on_touch.txt.

Also 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 on Drupal.contextual. This reroll fixes that.

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.

#16: That JSHint fail was not introduced by this patch, but I'll fix it :)

Fixed #16.

I split up the single View into a VisualView, an AuralView and a KeyboardView. 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 the KeyboardView, 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.

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.

Thanks nod_, very good refactoring :)

I only want to make very minor changes to your changes in contextual.js, attached in reroll:

• Moved initContextual() to the top of the file.
• Fixed docs in a few places.

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:

• Again docs fixes.
• 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).
• Touch (touchend) support for edit mode toggle, identical code to the one in contextual.js.
• Got rid of the drupalEditModeChanged DOM event, instead I'm now updating Drupal.contextual.collection directly. Faster.
• Removed 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.
• There's now a separate AuralView, cfr. #18.

(All of these changes are facilitated by the work we've done in this issue on contextual.js.)

#21: contextual-backbone-1971108-21.patch queued for re-testing.

no reasons that it doesn't come back green.

I 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.

Thanks!

No, it won't need a change notice IMHO :)

Unfortunately 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!

So, 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!

Here's the interdiff of 21 to 28

I didn't think it necessary because it really was just removing the edit.module hunk. Apologies — I'll provide an interdiff next time.

Looks good.

Committed 8498adc and pushed to 8.x. Thanks!

D'oh, really sorry for the trouble folks. :(

No worries :) We're all just humans!

Plus, this didn't *break* anything. Just an unnecessary, unrelated hunk that got committed :)

I'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:

Uncaught TypeError: Cannot read property 'collection' of undefined      contextual.toolbar.js:53

This fixes it, since Drupal.contextual.collection is defined in the other file.

You're absolutely right. I'd swear I once had that. Thanks!

I 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.

#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!