Contextual links currently can't be disabled, see #607244: Decrease performance impact of contextual links and #473268: D7UX: Put edit links on everything. This isn't just a performance issue - you'll want to be able to browse your site as a 'normal' user without actually switching, and there's a chance they'll get in the way.

I think we probably need the following, roughly in priority:

1. A permission - so that, for example, you can completely disable the links (and the processing to create them) for anonymous users.

2. A per-user setting as to whether they're on or off. We might want to default that per-role in a setting somewhere too.

3. A toggle - probably a tokenized URL which switches them on/off and sends you back to where you were (since I can't see us adding the links by AJAX) - which can then be used for a toolbar button or whatever. This would trigger #2

Comments

Crell’s picture

Crell CreditAttribution: Crell commented

I was shocked to see that the patch went in without a kill switch, so I am very very much in favor of making it disableable.

Performance issues aside, there are two key reasons why a kill switch is a must:

1) There are perfectly viable use cases where edit-in-place is actually bad UI. It makes a great deal of sense during certain parts of a site build out, or for administering brochureware-type sites, but for many other use cases it's horrible UI. Those use cases are still important and a major portion of the Drupal install base.

2) For theming, EIP links break, well, everything. Especially inline links instead of hover links, they completely throw off all alignment. Theming for them is going to be a pain, and theming around them even more so. Essentially, as is you cannot ever develop a theme as an "admin". Of course, theming a site also involves a great deal of administrative futzing; setting formatters, filling in dummy content, configuring theme settings, etc. So we've just greatly raised the bar to theming a serious site.

I actually disagree with catch's priority list, and think having a "just turn this damned thing off" feature is more important than more fine grained control. I'll be perfectly happy if we get all of the above, though.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

The only problem with "just turn it off", which would be 0 on that list otherwise, is that it's possible comments and nodes will use this instead of hook_link() or MENU_LOCAL_TASK- if that's the case, unless we have some kind of weird fallback, it'd be near impossible to administer comments and nodes inline - you'd always have to go to the admin panel.

However that's not decided yet, comment module currently both has links AND contextual links, and node is still using tabs.

Crell’s picture

Crell CreditAttribution: Crell commented

Well that begs the larger architectural question of whether we're introducing an admin overlay feature here or creating yet-another-standard-screen-component-dojobby, a la $links. Now, $links has its problems but I didn't know anyone was planning to drop them in favor of EIP links. That is a much larger IA question I didn't know was even on the table at this point.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Yeah me neither, but in the case of comments it's currently causing c. 3 queries for every rendered comment in HEAD. So it needs to be resolved pretty fast.

Crell’s picture

Crell CreditAttribution: Crell commented

Absent any clear movement to make Drupal non-functional without EIP links, I say we move forward with making them disableable in as many ways as we can manage in the next 3 weeks.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

By the way, comment issue is at #611642: Roll back comment contextual links (temporarily).

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Issue tags: +Contextual links

This sounds like a duplicate of #601150: Improved UI for contextual links and #607244: Decrease performance impact of contextual links, no?

By the way, the code for most of the above suggestions already exists somewhere in #473268: D7UX: Put edit links on everything. It just wasn't in the patch that was eventually committed...

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#607244: Decrease performance impact of contextual links should be reserved from the impact of having the links switched on. Things like the block and comment queries due to using menu loaders, non-indexed queries etc. That's a different issue to a killswitch, although obviously not being able to switch them off exacerbates this

#601150: Improved UI for contextual links also looks more like the appearance when they're switched on, not sure if a killswitch/permission/toggle fits there or not.

dman’s picture

dman CreditAttribution: dman commented

Definately need a config switch. They get in the way a lot when trying to use the DOM explorer, and my first impression was that they were a bit overkill. Nice, but not always.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Well, let's just make not to duplicate anything at #601150: Improved UI for contextual links here - it is pretty closely related.

To clarify my above comment, the code that was already written:

  • Implements a "view contextual links" permission (so you can hide it completely from certain roles)
  • Implements a session-based toggle switch (so users who do have permission can turn them on and off when they want to).

So I think that already covers most of @catch's #1-#3.

yoroy’s picture

yoroy CreditAttribution: yoroy commented

#601150: Improved UI for contextual links seeks to design the behaviour of the links themselves, not this kill switch etc.

So yes, implement the permission and session based toggle in this issue (both sound indeed what you want to do to me).

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Closed (duplicate)

#607244: Decrease performance impact of contextual links, thanks.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

The patch at #607244: Decrease performance impact of contextual links currently adds a permission, but not a toggle.

However, I really think anything but the most basic functionality of a toggle is tied up with the default way in which these links will be presented (i.e., is the toggle a link? where and how is it displayed by default?) and therefore should be dealt with in #601150: Improved UI for contextual links, so from my point of view this is still a duplicate.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Closed (duplicate) » Active

sun, I don't see this as a duplicate. We want a way to clobber contextual links, period, and remove them, either selectively or no. That's a functionality question, not a performance question.

mcrittenden’s picture

mcrittenden CreditAttribution: mcrittenden commented

Subscribe.

mattyoung’s picture

mattyoung CreditAttribution: mattyoung commented

~

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#626286: Make contextual links a module

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
CreditAttribution: eojthebrave commented

subscribe

eigentor’s picture

eigentor CreditAttribution: eigentor commented

This might help out the current discussion about the contextual links: #601150: Improved UI for contextual links

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Active » Closed (duplicate)

This is kind of a not-very-focused meta issue, and there are a number of more specific issues where actual work is taking place:

#626286: Make contextual links a module
#629160: Allow contextual links to be disabled for particular themes
#648370: Implement a toggle for display of contextual links

I think it's best to close this for now.