Problem/Motivation

In #1824500: In-place editing for Fields, we added a new "Edit" module that provides a new "Edit" button in the toolbar. This is confusing because there is already one edit link on node pages, which are displayed as a tab (or "local task" in Drupal parlance). These two links, both with the same title, do different things. One provides inline editing capabilities, and the other goes to the node edit page.

Besides that problem, Drupal also has had a very, very long-standing problem that tabs appear specifically for administrators. These tabs don't show up for most end users, except on a few pages like their profile page. And more often than not, these tabs are not themed or styled in any way, leaving them clashing horribly against the site's custom theme.

This image recaps the problem:

Proposed resolution

  1. Make the "Edit" link in the toolbar act like the others links and provide a submenu.
  2. Make a new link for "Edit content" that goes to the traditional node edit form. This link would be contextual, letting the user know exactly what they are going to edit, such as "Edit view" or "Edit panel" (please excuse the D7 terminology).
  3. The Edit module's link becomes "Quick Edit" or similar.
  4. We could potentially add useful quick toggles to access common meta-data, specifically demonstrated here as toggling the published state.
  5. And because the edit option is displayed once already in the toolbar, we remove the local tasks entirely (in most places). Perhaps they could still be used for front-end tasks, such as editing your user profile. Or we could diff between toolbar menu links and local tasks, and display the difference. We could provide an alternative for them everywhere, such as adding "Edit profile" to the user menu. Or we could do some different option. The important thing is to remove the local tasks if they're not going to show up for normal users. Its confusing that they appear as part of the front-end theme when they are intended for administrators.

This image recaps these points:

Remaining tasks

Discussion and code.

User interface changes

All as described above.

API changes

The nature of MENU_LOCAL_TASK would either change (perhaps being moved to the toolbar) or they would be used less frequently throughout Drupal; instead using MENU_CALLBACK or creating a new menu type/property that is meant for displaying administrative links related to the current page.

CommentFileSizeAuthor
#145 1874664-145-followup.patch5.04 KBeffulgentsia
#143 1874664-132-followup.patch1.45 KBGábor Hojtsy
#136 1874664-136-followup.patch3.19 KBeffulgentsia
#132 1874664-132-followup.patch1.45 KBDavid_Rothstein
#120 1874664-120-followup.patch773 bytesDavid_Rothstein
#117 tasks.png40.91 KBsun
#103 pencils-1874664-103.patch64.8 KBWim Leers
#103 interdiff.txt11.7 KBWim Leers
#103 interdiff_101-point-2.txt1.42 KBWim Leers
#103 interdiff_remove-a11y.txt10.29 KBWim Leers
#102 Screen Shot 2013-02-11 at 15.54.30.png67.71 KBWim Leers
#101 pencils-1874664-101.patch55.65 KBWim Leers
#101 interdiff.txt2.9 KBWim Leers
#99 pencils-1874664-99.patch54.66 KBWim Leers
#99 interdiff.txt4.19 KBWim Leers
#97 interdiff_91-93.txt3.01 KBWim Leers
#93 pencils-1874664-93.patch54.86 KBjessebeach
#91 pencils-1874664-91.patch55.14 KBWim Leers
#86 pencils-1874664-86.patch56.5 KBjessebeach
#86 interdiff_85-to-86.txt5.03 KBjessebeach
#85 pencils-1874664-83.patch53.95 KBWim Leers
#85 interdiff.txt3.16 KBWim Leers
#79 pencils-1874664-79.patch53.43 KBWim Leers
#79 interdiff.txt10.01 KBWim Leers
#79 Screen Shot 2013-02-08 at 21.27.11.png23.8 KBWim Leers
#78 pencils-1874664-78-do-not-test.patch.txt49.35 KBjessebeach
#78 interdiff_73-to-78.txt2.36 KBjessebeach
#73 pencils-1874664-73-do-not-test.patch46.94 KBWim Leers
#73 interdiff_edit-overlay-removed.txt7.52 KBWim Leers
#73 interdiff.txt28.2 KBWim Leers
#70 pencils-1874664-70.patch27.47 KBWim Leers
#70 interdiff.txt1.38 KBWim Leers
#64 pencils-1874664-64.patch27.79 KBWim Leers
#64 interdiff.txt15.57 KBWim Leers
#64 pencils-1874664-64.mov_.txt2.2 MBWim Leers
#64 pencils-1874664-64.gif730.55 KBWim Leers
#65 pencils-1874664-65.patch27.29 KBWim Leers
#65 interdiff.txt1.68 KBWim Leers
#60 pencils-1874664-60.patch19.59 KBWim Leers
#60 interdiff.txt2.52 KBWim Leers
#60 Screen Shot 2013-02-05 at 13.00.12.png69.3 KBWim Leers
#58 1874664_pencils_58.patch16.37 KBGábor Hojtsy
#58 interdiff.txt1.21 KBGábor Hojtsy
#56 1874664_pencils_56-do-not-test.patch16.87 KBjessebeach
#45 quickedit-quick.png107.27 KBBojhan
#45 inline-edit-active.png100.72 KBBojhan
#42 1898020_unified-edit_10.patch16.58 KBGábor Hojtsy
#21 EditToggle.png82.85 KBGábor Hojtsy
#21 EditToggleOn.png251.6 KBGábor Hojtsy
#21 EditBody.png102.54 KBGábor Hojtsy
#21 EditMenu.png93.97 KBGábor Hojtsy
tabs-and-toolbars2.png186.12 KBquicksketch
tabs-and-toolbar.png136.67 KBquicksketch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jcisio’s picture

A great proposition! Only one question: should Devel still be there? I think only the Edit local task should be removed.

quicksketch’s picture

Only one question: should Devel still be there? I think only the Edit local task should be removed.

For something that is specifically administrative-only, I'd prefer to see it moved to the toolbar, perhaps as a top-level link with Devel providing a sub-menu: Database queries, Examine object, XHProf output, etc. Or potentially as a sub-link of "Edit", though overall that doesn't make a lot of contextual sense.

Another additional option is making the links provided by Contextual Menu module always available, even on the node pages themselves. Then Devel could place itself there.

And thirdly, yes we could keep the local tasks just to show the Devel link, but I think this would be the least preferable option (since again, this isn't usually shown for most users, breaking the administrative separation). We have the option to leave MENU_LOCAL_TASKs in place, and set expectations/standards for when they should be used that simply discourage them for administrative tasks, and direct modules to more appropriate tools, like the two options I just listed.

nod_’s picture

Issue tags: +d8ux

Really like that idea.

Bojhan’s picture

I am not sure, I honestly don't even like the current reason we have "Edit" in the toolbar. I think people want links that are close to the "thing", as further it is away from the main thing - the less obvious it is what you are about to edit. Since the current edit toggles a global inline edit mode, using it for links beyond that might create a very real disconnect between what you want to edit and where you need to go to do that.

From initial feedback, we noticed people do not see the "Edit" tab - why? Well because we introduced tray's, and we never tested whether that is actually a good idea. So placing more there, might cause even more issues down the line. So we might need some more testing, before we assume this is the best place to put more stuff. Personally I'd rather have a nice icon, that toggles the Quick edit mode, rather than putting more contextual links in there. I also still would love to see quick edit to be toggled near contextual links, rather than having to go to the top of page menu.

Wim Leers’s picture

  • We evaluated this and had reasons to not do it this way. IIRC, the major reason to not do this, is that it breaks down when you have multiple entities/nodes on the same page: then it is no longer clear which specific one the buttons/state relate to. Or you'd need to have a different UI/UX on pages where multiple entities live versus pages where a single entity lives — which is also bad.
    I've asked Kevin (@tkoleary) to comment here.
  • In-place editing of fields is the first step. The end goal is that everything becomes in-place editable. Once more things are in-place editable, the current design makes more sense.
tkoleary’s picture

I think all the comments above are driving in the same direction which is a simpler, more consistent and unified approach to in-context editing. Dries has asked us to unify the interaction patterns for this which has implications in several different issues. From both the user and the core developer perspective they look something like this:

1)

  • User: "I may not know the difference between a block and a field but when I am looking at my site I want one kind of "button" for everything I can click (or touch) and edit"
  • Core developer: We already have contextual links and edit module, we need to establish a single interaction pattern that applies to both and works on both point and touch devices.

2)

  • User: "I want to perform a single simple task like renaming a menu link, I go to the contextual link and click configure block but there is no place to edit the menu let alone re-name a link. If I am smart enough to discover that looking under "structure" I will find "menus" *and* if I know the name of the menu that I want to edit *and* if I know that the place where I can access menu links is under "list links" not "edit menu" *and* if I find the "edit link" operation *then* I can re-name my menu link and, once I have read the descriptive text on the other six fields in the form so I know I have not done anything wrong (users do this), *then* I can save my work. BUT WAIT!, where is the page that I was working on? I want to go back and see the menu. So *if* I know how to navigate back to that page *then* I can go there and see the how the menu looks.
    What I want is a to be able to click edit menu right from the contextual link, or better still click "edit link" from the link itself *then* re-name the link in a modal that shows me only the three most important fields, *then* click save and immediately be brought back to my page to see the effect of my change.
    What's more I don't just want this kind of immediate editing experience for menu links but for all kinds of operations on my site.
  • Core Developer: We can easily add items to contextual links, and contextual links to items that don't have them. We also now have modals in core so we can evolve the overlay towards a new model where only 80% tasks are presented in context and the user can click through to the full form only if they need to. We can also move to a more traditional modal approach where submitting the form automatically closes the overlay. The work is in creating a good programmatic solution for determining which fields from the form to present. Wim has some good ideas on this.

3)

  • User: "When I look at a page of my site I see a toggle at the top to go to edit mode but I also see a tab above my content that says "edit". I want to know the difference between them."
  • Core developer: Local tasks and contextual links in this case are essentially doing the same thing. We should do away with local tasks and add the local task links to a contextual link on content.

We have been working on designs for this and will be releasing a blog post within a day or two.

quicksketch’s picture

IIRC, the major reason to not do this, is that it breaks down when you have multiple entities/nodes on the same page: then it is no longer clear which specific one the buttons/state relate to. Or you'd need to have a different UI/UX on pages where multiple entities live versus pages where a single entity lives — which is also bad.

Right, I made two suggestions for this scenario:

1. The "Edit content" link changes depending on what you're editing. That is it would probably say "Edit view" if you were on pages with multiple nodes. Or perhaps "Edit layout" if you were on a panel/page.
2. We add contextual links even on the node pages (right now we only add them on teasers), so that there is something closer to the content for users to use.

In any case, right now we have a serious UI problem with two "Edit" links that do the same thing. I think killing the local tasks is a good way to solve this problem, since they have always caused a serious confusion about whether they show up for front-end users or not. Considering we have a lot of other tools at our disposal now, we should use them rather than having 3 (or more) ways to edit a piece of content on the same page.

David_Rothstein’s picture

Hm... "Edit as a tab" (our current paradigm) is nicely optimized for sites with lots of end-user-generated content. It's not the only use case, but we shouldn't forget about it either.

Having "Edit" only available in the toolbar + contextual links seems to carry with it the assumption that it is only higher-level site administrators who would be doing the editing?

Bojhan’s picture

@quicksketch As troublesome as local tasks are in terms of are they front-end or not, they are the most discoverable pattern so far - I am all for changing this but we need to measure the impact, as this is the single most important button for our content creators. The magic you describe of automatically switching based on whats on the page is to me just a guess game for users, they will want to get close to the "thing" and then click edit - like we do with contextual links and/or what Kevin's designs had with floating boxes on top of them.

@tkoleary I look forward to your hashed out designs, its hard from your story how it fits all of the more complicated parts. In 2) you describe an interaction that's already there, on menu blocks you can "List links" - if you can figure out an interaction, to edit individual links from the get go that'd be great - but its tricky to scale, without going to the "list links" interface.

David_Rothstein’s picture

In 2) you describe an interaction that's already there, on menu blocks you can "List links"

Yeah, just to clarify, that's already there in Drupal 7 - it has gone missing in Drupal 8 but that's due to a bug (#1304470: Add tests for module-provided contextual links on blocks).

Wim Leers’s picture

In any case, right now we have a serious UI problem with two "Edit" links that do the same thing.

This is not true. They do different things. One is for in-place editing, one is for back-end editing.

quicksketch’s picture

In any case, right now we have a serious UI problem with two "Edit" links that do the same thing.

I misspoke. Two "Edit" links that do the same thing would be less of a problem. We have two "Edit" links that do different things. And that's the problem.

quicksketch’s picture

Having "Edit" only available in the toolbar + contextual links seems to carry with it the assumption that it is only higher-level site administrators who would be doing the editing?

We also have a 4th paradigm available to us for administrative tasks: the is the node (or comment) links. Just like comments are editable on the front end via links, we could do the same thing for nodes. The node/comment links are much less intrusive into the design of a site than the local tasks.

Or like I mentioned above, we could do a diff between what's displayed in the toolbar and what's displayed as a local task to reduce the duplication... but then we end up with even more confusion about when tabs do or do not show up, so diff'ing between the toolbar and tabs probably isn't a good solution.

tkoleary’s picture

@david rothstien No, that's not the goal at all. The goal is to make the edit affordances much more obvious and accessible (in both the general and specific sense of that word).

tkoleary’s picture

it has gone missing in Drupal 8 but that's due to a bug

Ah, I was wondering why that was. I thought I had seen that functionality before.

sun’s picture

Just to provide some actual user/usage data points here:

The Administration menu module contains more or less exactly this user interface feature since ~D5 as the "Move local tasks into menu" configuration option. It is disabled by default and needs to be enabled manually in the module settings.

The feature was invented and developed for the use-case of simple/small brochure-alike sites, on which most pages represent simple, single content (node) pages. And also, on which the tabs/local-tasks may be hard to inject/position as interface elements within the theme (i.e., where the design does not leave sufficient/proper room for tabs nearby or between the page title and main content [because tabs/local-tasks are a Drupal phenomenon and Drupalism that many designers are unaware of]).

The feature is implemented poorly and did not see much love since its inception, since only very few users enabled it. That is, because the user interface pattern indeed works for very small and overly simple sites only. Anything larger requires multiple interaction points on a single page, which essentially translates into contextual links (as well as tabs/local-tasks).
[But I purposively designed contextual links to be based on local tasks, since they are the same, so it's sufficient to talk about contextual links with regard to the problem space of this issue.]

Furthermore, there are user experience issues that I've seen to cause confusion for end-users:

1) There are pages that do not have any local tasks, which in turn means that the (moved) list of links is suddenly empty on some pages. Users did not understand why they have links on some pages but not on others.

2) By moving the links into the toolbar, they suggest to be "page-level" administrative interaction links, but they are not. They are still contextual links that pertain to the block in the main content page region. Only on small brochure-ware sites, there is actually a 1:1 correlation between the page and the content for most pages, which explains why the concept works better there. As soon as that is no longer the case (for individual pages, but also in case of all pages), users had troubles to find the contextual links for the main page content in the toolbar, while all other contextual links are NOT found in the toolbar.

3) Local tasks/contextual links vary from content to content and from page to page, resulting in an always-changing and inconsistent user interface. Users tend to memorize the position of interaction elements in the interface, which is especially the case for primary and dominant navigation elements like an administrative toolbar. Users clicked the wrong links, since they did not expect the toolbar links to suddenly change between pages. Worse, users effectively did not trust the toolbar links at all anymore, which means that they stopped taking advantage of memorized interface positions; ultimately slowing down almost all of their administrative tasks and operations. Consistency is the heart and core and fundamental key for any kind of toolbar.

For these and further reasons, the feature has always been considered to be an option for a <10% use-case only (and hence, is disabled by default, and never saw a great amount of attention).

In summary, and to provide some more feedback on the OP:

A) Moving local tasks into the toolbar will not work out for the majority of Drupal site use-cases.

B) What could work, however, is to turn "local tasks" into a swappable service, which results in the current tabs/local-tasks like now by default - unless the service is overridden (swapped out) by a toolbar_tasks.module for the use-case of a small brochure-ware site. [Note: Service, not Module, because there always has to be an implementation; i.e., the architectural concept is similar to swapping out the default mail system with a different one, but there can only be one, and there must be one.]

C) Moving actual widgets and meta properties of content entities (or the main page content) into the toolbar will not work out, since there's not sufficient space/room in the toolbar, and splitting/clustering the user interaction elements from a single spot into multiple ones that are scattered across the screen will at most confuse users of all skill levels. 100% objection to that detail of the proposal here.

D) The toolbar's "Edit" link should indeed not cause an immediate action. The same problem exists for the "Home" link already, which similarly triggers an immediate action whereas all other links do NOT. The root cause is the additional layer of first-level "tabs" in the toolbar, which are a fundamental design flaw either way and should not have been part of the design in the first place.

In short, the only part I can agree with is D), but that seems to be a separate issue in any case — we either have to fix "Home" and "Edit" to behave like tabs ("trays"), or we have to get rid of the first-level tabs/trays entirely and re-implement and solve the underlying interface problem that the tabs/trays are trying to work around in a proper way. Someone should create an issue for that (major) bug, unless there is one already.

mallezie’s picture

D) The toolbar's "Edit" link should indeed not cause an immediate action. The same problem exists for the "Home" link already, which similarly triggers an immediate action whereas all other links do NOT. The root cause is the additional layer of first-level "tabs" in the toolbar, which are a fundamental design flaw either way and should not have been part of the design in the first place.

I fully agree that "edit" should not cuase an immediate action. I don't agree for the 'Home' link. I can follow it is not consistent with the other links, but in my opinion it's something people use a lot, and even expect this to be an immediate action. (I'm stuck, take me back home, where i know what to do, i'm gonna try again or do something else.) (While i'm typing this perhaps with a warning that you loose current form content, but that's probably another issue).

Wim Leers’s picture

RE D: Couldn't a solution be to have "Edit" be visually distinct from the rest, to convey it is an action, not a tab/menu link?

Bojhan’s picture

@sun Thanks for your comments, I agree largely on A), D). I think C) is a real challenge, but not something we should down immediately. I am going to wait a bit before giving in depth feedback, given that kevin is about to release some design ideas.

webchick’s picture

Cross-linking #1882482: [meta] Unify editing approaches in Drupal. These might now be duplicates, I'm not sure.

Gábor Hojtsy’s picture

FileSize
93.97 KB
102.54 KB
251.6 KB
82.85 KB

As per latest design proposals from @tkoleary the edit toggle would move to the right side of the sidebar (by the side of the not-yet-existing search box) and become a pencil icon:

EditToggle.png

When turned on, it gets a crossing line to signify now it can be used to turn off editing. Also, all "editable things" get a circled pencil icon. This combines actual in-place field editing provided by edit module with contextual menus. So wherever there is a pencil icon, it either is an in-place editable thing (currently in core signified by a blue bordered area while other areas are white-out); OR it is an area with contextual links that are shown when the pencil is clicked:

EditToggleOn.png

When clicking the pencil on an in-place editable field, it does in-place editing:

EditBody.png

When clicked the pencil on a non-in-place editable thing, such as a menu, it is contextual links from the block wrapping the content:

EditMenu.png

Then the individual buttons are context menu options (ignore the up/down arrows and the Live indicator for this issue). Clicking the buttons shows a modal/popup/overlay with the form to do the relevant action.

So this is a somewhat seamless integration of editing of things that avoid tabs and in fact exposes the contextual features in a much bolder way. The thinking of the design is Drupal menu tabs on the front end will go away entirely indeed (similar to @quicksketch's proposal above).

I believe this is a more complete solution to the problem. Any concerns/complaints/ideas/constructive feedback?

BTW if this design is good for implementation, I assume this issue would need to deal with the "pencilification" of the edit features and another one would need to deal with integration of the toggle with contextual module and the pencilifaction of that. The bigger click/touch targets for contextual menus is already in progress at #1879386: Increase target size of contextual links on touch devices.

Gábor Hojtsy’s picture

BTW a clickable version of the "pencilification" prototype is posted at #1882482: [meta] Unify editing approaches in Drupal, my screenshots above are based on that.

hass’s picture

Ok, the newer layout with the pencil on the right side looks a lot better. I just thought in the past that the grey stencil is somehow "hidden" in the black top toolbar. You can miss it very easily. It's a contrast thing, too and it's far away from what I'd like to do (editing the content) as it's above my header image (that could be very large/half page on some sites).

tkoleary’s picture

@Gabor Thanks. Just one quick note on the prototype, it is a fixed-size image based app built to be tested on an iPad. The interactions for desktop will be slightly different.

quicksketch’s picture

Status: Active » Closed (duplicate)

Seems like we should go ahead and close this issue if we're going to be pursuing a different route. I think @tkoleary's general approach is superior in several ways:

- Contextual links touch-compatible and not-distracting for admins at the same time.
- Local tasks go away.
- The word "Edit" is replaced with the pencil.

It's actually kind of funny that instead of reducing the number of places that say "Edit" from 3 to 1, he actually reduced it all the way down to 0 (though it's replaced by the pencil. ;)

However, his approach doesn't seem to address how to get to the normal node-edit form (at least in a clear way). We can discuss that in #1882482: [meta] Unify editing approaches in Drupal.

quicksketch’s picture

Title: Convert "Edit" toolbar option to replace tabs (local tasks) in most places » Reconcile "Edit" toolbar option with local tasks (tabs) and contextual links for editing
Status: Closed (duplicate) » Active

Or actually, it looks like the meta issue references this one already. Maybe just give it a better title? Our discussion has clearly fragmented, but I think it makes sense to have something that specifically tackles how Edit module's toolbar button behaves (and how that interacts with contextual links and tabs), so maybe this is the most appropriate spot. The meta issue deals with a lot of other issues (moving blocks around, editing menu links, etc.)

Gábor Hojtsy’s picture

Yes, I agree the META issue covers a *lot* more things, and this is the edit specific simplification. Would be good to see the desktop suggestion to cover all cases. @tkoleary?

tkoleary’s picture

@Gabor Hojtsy Yes. ETA on that is sometime later today.

catch’s picture

Enabling/disabling contextual links via the edit button would let us apply the same approach to #914382: Contextual links incompatible with render cache as was taken for inline editing.

Gábor Hojtsy’s picture

@catch: well, that is if we only ever support contextual links if edit module (and it's toggle) is enabled. I'm not sure we want to make contextual links unsupported if edit module is not enabled, do we? :)

tkoleary’s picture

Desktop version of the prototype is here: http://invis.io/FVB89STC

With edit mode tuned off, click on the main menu to see the hover state (there are no actual hover states in invision)

Wim Leers’s picture

#31: I think that's going to be a disconcerting (and worst-case even epilepsy-inducing :P) experience. Whenever users are navigating around their site on desktop browsers, these hover states will cause things to flicker in often unintentional ways, which will be disconcerting.
What could work is if only the pencil icons appear instantaneously, and the outlines fade in after e.g. 0.5 seconds and take 0.3 second to fade in. Then at least when you're moving your cursor all over the place, you won't see annoying outlines pop up all the time.

Gábor Hojtsy’s picture

@Wim, @tkoleary: the current contextual links work in an always-on edit mode, so as you browse around, the light gray gear appears. If you move to hover on the light gray gear, it becomes darker gray and a checkered border appears around the region you are editing. This minimizes the flickering and distraction. I agree that the desktop prototype's hover state(?) blue border is a lot more invasive.

@tkoleary: is it intended that the off-state of the edit mode shows all pencils? https://projects.invisionapp.com/share/FVB89STC#/4622203

catch’s picture

@catch: maybe we could factor out the toolbar toggle and use it for both edit and contextual links. I know there's several cases where I've wanted to be logged in and not see them - i.e. when there's minor bugs in a theme and they interfere with target area for actual links making them hard to click.

Gábor Hojtsy’s picture

@catch: definitely, we don't need to solve everything in this issue. Factoring out the toggle to be used for contextual links (when edit is enabled) sounds like the best first step. (I assume we don't want to make contextual dependent on edit, in which case we'd need a behaviour for when that toggle is not available or have that toggle in a 3rd place and make edit and contextual work with it).

tkoleary’s picture

@wim leers I agree. That would make the experience much smoother

tkoleary’s picture

@gabor No, all pencils appear in the "on" state of edit mode. The confusion may come from the toggle button. It's an action not a state. Pencil is "enter edit mode" crossed out pencil is "leave edit mode".

That's how it is now —unless there's an error in the prototype i'm not aware of.

tkoleary’s picture

@catch ok for the moment but ultimately the toggle *has to* be there or the touch experience falls apart.

LewisNyman’s picture

@tkoleay I don't think it does, the current behaviour in D8 if for the contextual link to be visible by default, maybe not ideal but hardly falling apart. I'm not saying we shouldn't toggle but I wan't to make sure our assumptions are correct.

jessebeach’s picture

A very similar discussion with an in-progress patch is happening at #1882482: [meta] Unify editing approaches in Drupal.

Gábor Hojtsy’s picture

Well, this is a sub-issue of that meta, so its not a similiar discussion, but rather a compound patch. This issue here should be used for the edit pencils unification patch as the title expresses clearly :) Jesse, care to post it here?

Gábor Hojtsy’s picture

#1898020: Proof of concept for unified edit pencils is now marked as a duplicate of this issue. Posting the latest patch from there (from comment #10). This should apply to Drupal 8 core *on top of #849926: links wrapped in .contextual-links-wrapper divs are not accessible at all via keyboard alone also problems with screen readers*.

Gábor Hojtsy’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1898020_unified-edit_10.patch, failed testing.

Bojhan’s picture

FileSize
100.72 KB
107.27 KB

I discussed this with WimLeers extensively during our mini-sprint, one realisation we had is that we should seek to unify all patterns not have an edit mode next to a contextual links, next to local tabs kind of pattern.

What I propose, is rather than having a separate "mode" to toggle the Edit module. We have this mode integrated into contextual links, that way you can always acces Edit module at the place that you are looking instead of having to navigate to the top. This way we also always accomodate the idea of more than one action on a object, the idea that we can toggle inline edit mode on just entities is a little silly. For example, inline edit mode on site name - can only accomodate changing the title, what if contextual links also exposes an option to change the logo? Same with blocks, you will always have several options next to Edit module its inline edit mode.

An example;

Clicking on that option would show the inline editing tools;

This would also accomodate when its a block in a view, because contextual links already group that and in the rare case it doesn't (e.g. panels) it finds other ways.

The actual "Global edit mode" toggle, would simply show/hide the contextual link icons. That way, clicking the global edit mode doesn't do anything special but toggle display of the contextual buttons.

The only real concern I have is that, there is no easy way to toggle out of the quick edit mode once you are in it. Since there is no "global edit" toggle. Wim suggested perhaps doing something in the entity toolbar that we expose per field.

Gábor Hojtsy’s picture

This sounds pretty good for me personally.

tkoleary’s picture

Yes, I think this could be breakthrough in several ways. My only question is what happens to the pencils on blocks when entity quick edit is turned on?

Dave Reid’s picture

I really like the concept of using the contextual links to display all available actions, and inline editing being one of them. I think a confusion may be if there are multiple things available to edit like in the screenshot shown in #45, what does 'Quick edit' actually edit? Both things? Should it be split for the different things? 'Quick edit block' 'Quick edit view'?

Damien Tournoud’s picture

Wasn't the "click to edit" pattern what ultimately caused the inline editing to fail for Plone? I think I remember that the global edit toggle is a strong design decision of CreateJS.

Wim Leers’s picture

Dave, Damien and others: you're missing implicit context. Please see http://buytaert.net/spark-update-unified-in-place-editing & #1882482: [meta] Unify editing approaches in Drupal.


#48: What's missing in Bojhan's mockup of #45 is an outline like the one for contextual links, and in fact, like you have for node contextual links today. That's how you see *what* you'd be editing.
Secondly, the "multiple things" aspect: most often, there is nesting going on, and we already deal with that by not dealing with it today (i.e. a panel containing a view in one pane and a block in another, results in 3 contextual link gears — here pencil icons — one for the panel, one for the view, one for the block). When there is no nesting going on, e.g. when a View is rendered as a block, then… you have a great point :) I'll leave it to the design gods to come up with a good answer to that.

#49: See http://bergie.iki.fi/blog/inline-editing-done-right/:

inline edit was a great feature. People was not complaining against inline editing itself and most of them appreciated it a lot.

What was the real problem with end users? Users just hated opening the inline edit when trying to click on a link on a document. This could be solved very easily and I'd like a future reimplementation of inline edit keeping in mind these usability issues.

So, it rather was the lack of "click to edit" that caused it to fail for Plone :) The global edit toggle was not influenced by Create.js at all, we were doing that right from the beginning, long before we were using Create.js. As per the related issue mentioned above: we're working on unifying Contextual Links + Edit. We will retain Edit mode, because it shows all editable things, which is very handy on desktop and essential on mobile. On desktop, however, we're working to make it possible to hover over anything and have a pencil icon/contextual gear show up that lists available operations. This indeed means you will first have to decide on a specific entity to edit, instead of immediately seeing all fields that are editable. But something to this effect will be necessary for #1678002: Edit should provide a usable entity-level toolbar for saving fields anyway: entity-level save action instead of field-level save action (the current field-level save action causes a new revision to be created for each field that you modify through in-place editing).


I hope the above clarified things and did not cause further confusion.

Bojhan’s picture

@Dave Well thats the key question :) I think it should always try to do both, there is no point to "Quick edit" when you need to figure out which part you are editing. Especially since on the front-end, there is often little perceivable difference between items. Obviously this is a can of worms, as it means one could be editing any kind of module provided thing in-line and therefor all modules atleast in core should support this mode. This is a concern that webchick shared a long time ago, and I think its more and more important to decide if we wish to support anything beyond just node entities for inline editing.

@Kevin I imagine those pencils in the "global edit mode" would still be there. Depending on how we integrate the closing out of quick edit mode button.

tkoleary’s picture

@Bojhan I think so. Still editing that design though. My attention has been focussed on this one http://drupal.org/node/1898946 (just updated)

Gábor Hojtsy’s picture

@Wim: well, reality is that "View in a block" is also nesting, it just happens that blocks bubble up the top-most object's contextual links to the block itself. So for menu blocks, you get the menu contextual actions with the block actions in one list, for Views blocks, same thing. However, your views block might display node teasers, which in turn also have contextual links. Now those are not bubbled up (even if only one node teaser). Bubbling them up would quickly become confusing, especially if there are multiple nodes. I think we might want to look at tweaking the bubbling up in separate issues if we find there are too many pencils and/or skip some of the contextual links when there are too many nesting levels. I don't really know, we should test with a complex page (eg. panel containing views, displaying nodes, etc :)

Bojhan’s picture

Can we get an initial implementation for this going?

Gábor Hojtsy’s picture

@Bojhan: the above posted rough pencil unification patch depended on #849926: links wrapped in .contextual-links-wrapper divs are not accessible at all via keyboard alone also problems with screen readers, and that was a good start to make the contextual links conditional on edit mode, restyle with pencils, so since #849926: links wrapped in .contextual-links-wrapper divs are not accessible at all via keyboard alone also problems with screen readers was practically RTBC, I tried to nudge that toward being committed just now, so we can quickly jump back on this.

jessebeach’s picture

I'm rerolling the mega-patch in #1882482-45: [meta] Unify editing approaches in Drupal. I need a place for the "unified edit pencils" patch to live. This is still very much prototype code as we continue testing efforts.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

FileSize
1.21 KB
16.37 KB

I've attempted to get the prototype patch closer to Bojhan's concept, but pretty much failed to track down how things relate let alone make the changes. I figured out two very simple changes though so posting a little update. Baby steps. We should find Wim or Jesse to help bring this forward. There is nothing wrong with testing this patch, so removing that suffix.

Changes:

- made the edit overlay z-index way down in the negatives, so clicking out of editing does not close it down; this was a common complaint with people testing this AFAIH
- made the menu model add "Quick edit" links to all node related contextual link lists (not wired up to anything yet)

This was the point I realised there is a global edit toggle coded into the model, so to make the move to per-entity edit toggles, that would be pretty much beyond little touchups territory. It is not just making it look a bit different :/

So sounds like we'd need to wire up the quick edit links to state changes on a per entity basis and make the state changes react by displaying the blue outlines like before (sans edit overlay). An exit method out of that was not defined, so not sure.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

I'll continue working on this.

Wim Leers’s picture

Small iteration: got rid of local tasks for nodes (in a VERY hacky way), now the "Revisions" tab also appears as a contextual link (but only on the full node page).

"Quick edit" contextual link is still not yet working, that's the next thing I'll be working on (I wanted to post this already because this part was simple).

Screen Shot 2013-02-05 at 13.00.12.png

Status: Needs review » Needs work

The last submitted patch, pencils-1874664-60.patch, failed testing.

Gábor Hojtsy’s picture

I think we'll need to adjust contextual module a little and include menu items that are not necessarily local tasks or actions. Eg. the Delete item shows on the screen but I don't think that would be a local task or action. In #1880168: Introduce top-level sections for all forms we also need to make "add link" back to a local task even though it is not currently one in core. That does not actually make it appear due to how it is placed in the menu structure already but that is just a lucky side effect. Since we want full objects to display contextual links where Drupal 7 would have displayed local tasks instead for them, we need to make them optionally separate. Adding a new menu item type?

Gábor Hojtsy’s picture

Dug some more. Reality is this feature already exists. The contextual types on menu items have an effect on how local tasks are rendered. For example, the node delete item has MENU_CONTEXT_INLINE (even though it is a MENU_LOCAL_TASK), so it will not appear as a tab, but will appear as a contextual link. The edit item is a MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE, which makes the task appear on the page. If you modify the items so they don't appear on the pagem they will go away properly and no need to hack anything in theme_menu_local_tasks() like you did.

Looking at the test failures, seems like the Edit and Translate links are missing (obviously with no local tasks). The contextual module would be needed for these links to appear now, so it will need to be added to these tests.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs review » Needs work
Issue tags: -in-place editing, -Spark
FileSize
730.55 KB
2.2 MB
15.57 KB
27.79 KB

Notes about #60:

  • significant a11y implications/changes (because we're essentially proposing to remove most if not all local tasks from Drupal core)
  • significant menu API change (see above) — though this may be in line with what Crell et al. were aiming to do
  • since we remove local tasks, we must also ensure that this works well with JS disabled (where we will not have "the global edit mode/show contextual links" toggle, so this implies that for administering on mobile via contextual links, you must have JS, if not, you're forced to use the /admin part of Drupal, i.e. the back-end)

@Gábor: Will address #63 in next reroll, to keep the interdiffs topical.


The direction we're heading in here has significant consequences for the way the Edit JS logic should work. The JS is currently architected around the assumption that there is an "Edit mode" toggle which will cause all editable fields to show up as editable. The direction we're currently heading in here imply that:

  • the "Edit mode" toggle no longer causes all editable fields to show up
  • the "Edit mode" toggle effectively becomes a "Show all actions" toggle, which will cause all pencil icons to show up, and each of the pencil icons has a list of potential actions (identical to contextual links before)
  • Edit's functionality just becomes another such potential action; but its action must be injected via JavaScript, because this action only makes sense to have when JavaScript is enabled — all other actions link to some kind of form, Edit's "Quick edit" action does not
  • because of all of the above, all previous a11y work done for Edit now also builds on the wrong assumptions and thus is inherently broken

So, for now, this is implemented in a rough manner, for prototyping purposes, to gather feedback ASAP. I'll work on making it core-worthy if we agree this is the right direction.

Code in this patch:

  • the code is not too bad
  • but we should definitely move the "Edit mode" toggle out of edit.module and into contextual.module (especially because right now it only shows up if at least one field is editable by the current users, whereas it should show up as soon as there at least one contextual link)
  • Edit's overlay is still there for now (because something like it might be deemed necessary after all)
  • contextual.module's JS should provide an API to cleanly inject dynamic/JS-only contextual actions
  • is specific to nodes, because both Entity API is lacking (no theme preprocess function for all entity types) and because contextual.module is lacking (see previous bullet)

Attached are a screencast (rename .mov_.txt to .mov) and an animated gif version of the screencast (right click, and open in a new tab).


From #58:

This was the point I realised there is a global edit toggle coded into the model, so to make the move to per-entity edit toggles, that would be pretty much beyond little touchups territory. It is not just making it look a bit different :/

So sounds like we'd need to wire up the quick edit links to state changes on a per entity basis and make the state changes react by displaying the blue outlines like before (sans edit overlay).

This is essentially what I implemented here.

An exit method out of that was not defined, so not sure.

This is a great point — for now it is implemented by changing the "Quick edit" contextual link to "Stop quick edit". To solve this better, this might require an entity-level toolbar to work nicely — see #1898946-11: [Meta] Simplify and reconcile the behaviors of edit-submit actions in node/edit and edit (edit-in-place) for that.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
Issue tags: +in-place editing, +Spark
FileSize
1.68 KB
27.29 KB

Addressed #63.

Wim Leers’s picture

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

Problems reported:

  • cursor instead of hand when hovering over the pencil icon (Bojhan)
  • when pencil icons are toggled to be permanently visible, their z-index causes them to be displayed on top of the overlay (Gábor)
  • contextual links in footer look tiny and have different font — CSS scoping problem (Gábor)

Fixing these.

Status: Needs review » Needs work

The last submitted patch, pencils-1874664-65.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: -in-place editing, -Spark

I think the behavior is pretty much exactly as Bojhan designed above, looks pretty cool. There could certainly be more things to improve with the accessibility with contextual links but #849926: links wrapped in .contextual-links-wrapper divs are not accessible at all via keyboard alone also problems with screen readers improved it a lot lately. Also if you don't have JS, they are not even hidden from the page, so you can see them inline. So the accessibility for inline edit will depend/inherit somewhat from the accessibility of contextual links. That is not necessarily a problem, if contextual links are accessible that is :)

Gábor Hojtsy’s picture

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

Found one more bug with edit toggle: You are NOT in edit mode, pick a contextual link, eg. edit menu, close the overlay => now the edit mode toggle visually looks like you are in edit mode, but you are not. If you try to toggle it you get into edit mode, no visual change.

Wim Leers’s picture

FileSize
1.38 KB
27.47 KB

Fixed #66.1 and #66.2. #66.3 is unrelated to this issue; it exists in vanilla D8.

Next, I'll be working on fixing the tests and code clean-up (including fixing #69).

Wim Leers’s picture

This is now included in Spark 8.x-1.x — go to http://simplytest.me/project/spark/8.x-1.x to give it a try!

Status: Needs review » Needs work

The last submitted patch, pencils-1874664-70.patch, failed testing.

Wim Leers’s picture

  • Edit's "overlay" removed (which was the white transparent layer that prevented you from clicking links when the old "Edit mode" was enabled). A consequence is that Edit's modals (which should be replaced with core modals anyway — see #1872296: Edit should use core-provided Dialog (instead of its own)) are now no longer the only thing you can interact with if there's an Edit modal — you can simply choose to click on other things besides it.
  • Moves the "Edit mode" toggle/button from edit.module to contextual.module.
  • Includes #1908624: Update edit modules implementation of hook_toolbar; refresh the design to match current mockups, but that issue should still be committed, since this one is not yet close to RTBC.
  • Removed Edit's Backbone.Router and disabled Backbone.history, because without a global edit mode toggle, this makes far less sense.
  • Set a weight for Backbone & Underscore, analogous to jQuery & jQuery Once. (I had to ensure contextual.toolbar.js runs before overlay-parent.js, otherwise the former cannot listen to the latter's events in time to detect an overlay being opened directly upon page load. But since the former uses Backbone, and Backbone would not yet be available, it would just fail.)
  • edit.module depends on contextual.module

Still TODO:

  • Make tests pass.
  • Restore a11y.
  • #69 is caused not by edit.module/contextual.module, but by toolbar.module — after all, it was not designed to have its tabs behave as toggles. Closing the overlay causes the base page to reload; so if a tab was active before, it is active again. This needs changes in toolbar.module's JS.

(Spark D8 not yet updated because it's functionally identical.)

Wim Leers’s picture

I just realized that the direction we've taken in #64 and later also solves #1862784: Improve Edit module's labeling of "editable things"! :) Precisely because there no longer is an "ocean of editable fields" — only the fields for a single entity are editable — it has become a non-issue. (It's possible to define complex views where this could become tricky nonetheless, but that's never going to be equally clear.)

Bojhan’s picture

@WimLeers gave me homework, and I totally did the wrong two issues - so here is my review, most of it is on detail level - which is a good sign! Taking into account yoroy's review @ http://drupal.org/node/1882482#comment-7035532. I played with the quick edit, and man thats fast :D - it took me like 10 seconds to change a piece of text.

1) The "contextual dotted line" should only appear on hover of the contextual link. This will also make it much less invasive, when you toggle the display all pencils.

2) We should probably remove the word "Edit", and just have the pen icon. Then its much more clear that you dont go into some "edit" mode but just toggling the display of the pencils. Yoroy, was confused about this.

3) I am still worried about the way we now toggle out of the mode. I think people might not notice, that you can do it this way. I would personally also expect that clicking "Quick edit" would close the contextual link menu. Something worth testing, and or further iterations.

4) Our indicator for "display all pencil icons" mode, is really kinda poor. Ideally the pencil would like turn Drupal blue, that would be a really clear signal - now its just a different background shade.

5) If you open a contextual menu, it should stay open until you move outside of its zone (just like D7). This is simply to make it easier.

6) When an error occurs on quick edit loading, you are left with an eternally with "loading...."

7) The dotted line, does not return on pencil hover when you are in Quick edit mode. This is when I first hover an "inline field" and then go to the pencil icon.

8) Should we step away from using a shadow when you hover over an inline field? It feels like its too much of a transition perhaps it can be a slightly lighther shadow or actual color change. This overall still feels a little clunky, where you first get the body field label and then the shadow - there seems to be something wrong with the timing of transitions here.

9) Inline still feels quite jarring, I noticed this when edit.module got into core and it seems its not really gotten closer to fixing those issues see screencast: http://screencast.com/t/jceB7HIICk. I know these are hard to fix; the jumping around of text, blue bars and the wysiwyg bar, but I am worried no one made notice of these issues and that there are few other contributors helping out on edit.module.

10) I keep getting erros when I go to edit items inline, perhaps this was because of a time-out? My wifi was a little buggy.

An AJAX HTTP request terminated abnormally.
Debugging information follows.
Path: /edit/form/node/2/field_tags/und/teaser
StatusText: error
ResponseText:
ReadyState: 0

Wim Leers’s picture

Thanks, that's very helpful! I'll answer everything tomorrow, and adjust the code where necessary :)

jessebeach’s picture

re:#69, this is not the Toolbar module doing this little bit of magic, it's Overlay.

/**
 * Reset the active class on links in displaced elements according to
 * given path.
 *
 * @param activePath
 *   Path to match links against.
 */
Drupal.overlay.resetActiveClass = function(activePath) {
  var self = this;
  var windowDomain = window.location.protocol + window.location.hostname;

  $('.overlay-displace-top, .overlay-displace-bottom')
  .find('a[href]')
  // Remove active class from all links in displaced elements.
  .removeClass('active')
  // Add active class to links that match activePath.
  .each(function () {
    var linkDomain = this.protocol + this.hostname;
    var linkPath = self.getPath(this);

    // A link matches if it is part of the active trail of activePath, except
    // for frontpage links.
    if (linkDomain === windowDomain && (activePath + '/').indexOf(linkPath + '/') === 0 && (linkPath !== '' || activePath === '')) {
      $(this).addClass('active');
    }
  });
};

I put a breakpoint on DOM attribute modifications for the link and it turns out its Overlay. Still trying to figure out how to avoid this code.

jessebeach’s picture

This solves #69 issue with the Edit toggle.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
23.8 KB
10.01 KB
53.43 KB

#75:

1) I disagree. By showing those outlines, it's immediately clear to what thing a pencil icon applies. It provides additional context. Not implemented.

2+4) Done. Together, these two make it *a lot* better :)

3) Agreed; but that's apparently a bug in contextual.module :) I pointed this out; Jesse Beach posted a patch that fixes it over at #1910008: When launching the overlay, a contextual menu remains visible when it should become hidden — already RTBC'd by yoroy :)

5) That behavior is not introduced by this patch, it already exists in vanilla Drupal 8. Please create a separate issue for that :)

6) That's unrelated to this patch as well; that belongs in the category of "Torture Edit and make it still behave well".

7) Works fine for me, if you mean this: 1) don't enable edit mode, 2) go to the pencil icon of some node, note that the dashed outline around the node appears, click "quick edit", 3) hover one of the fields of the node, 4) hover the pencil icon, the dashed outline around the node should appear.

8+9) This, too, is off-topic. That's really focusing on Edit's styling, which we can do later; we shouldn't introduce (too many) non-essential changes here.

10) That would indeed be because of bad network probably, also see my answer to 6) :)

#78: Thanks :)


Tests pass now! RTL styles added for right-aligned "Edit mode" toggle.


Try the patch below at http://simplytest.me/project/drupal/8.x, click "Advanced options" (hahahaha cfr. #1880168: Introduce top-level sections for all forms :D), click "Add a patch" and paste the link to the patch!

Screen Shot 2013-02-08 at 21.27.11.png

Bojhan’s picture

@WimLeers I dont think you are right on 1) the reason contextual links are implemented as they are now, is because there goal was not to stand out. That's the whole point, it should seamlessly fit inside the act of just navigating around on your site. We made a conscious decision to make it small and not have the dotted line appear on hovering the zone, this because for much of your site building/content creating activities you are just browsing around - especially after the site is build. Having that additional effect is useful, but not really necessary.

We have not found in any of the testing we did since Drupal 7 came out, that people required this additional signalling as they moved around the site (it never came up people needed to know what zone a cog related to, they saw that as they hoverd the cog). And lets keep in mind these effects, have a big impact - just the fact that we now have a larger pencil icon and not a small cog will mean that with every single movement on your site - you will see a significantly more noticeable icon, this will be distracting, having dotted line to me have an even bigger impact because it creates zoning - which is a landmark visual, and therefor even more distracting (for example, thats why parking zones often have a color outline, its a highly noticeable visual even more so than a icon). So your reason, that it provides additional context although true, we decided in Drupal 7 - that it was too distracting, and that people did not require it to match the pencil to a given zone. It was a conscious design decision. I think these kind of details matter, and I think to say "its provides additional context" and therefor its good - is not going into the fact that its also a major distraction. The redesign already provides additional distraction with the bigger pencil icon, no need to add more - and certainly not for showing what zone the pencil is related to, as we have already seen that providing that information upon hover of the pencil works just fine.

In combination with inline edit, I think its even more distracting - because now you have two border/outline effects dotted and blue lines - as you are doing quick editing.

5) Strange, its annoying how these little bugs sneak into core.

7) I was able to reproduce this time and time again, I would enable quick edit hover from the right, to the left straight to the inline editable field and then to the contextual link and the dotted line would never reappear.

6, 8, 9) Sure, but lets keep in mind that we are building a holistic experience. As long as edit module, still has so many quirks it will continue to interfere with this being a smooth experience.

Whoa being able to use simplytest.me is great, I think the edit mode toggle is much more clear (with the blue + no label). I wonder since its a "mode" that ones toggles, it should maintain itself across pages? Its kinda freaky every time you click "Edit" (that opens in the overlay) that something in the corner disappears, and then when you return to the page you don't see the edit mode enabled anymore. I guess the question is whether we should really disable edit toggle - its going to occur that you have these toggles in your admin theme too (e.g. for when you use views for your content listing - how do we make that pencil mobile accessible?). I guess this is a larger question, not so much suggestion.

We should also find a better label, since its not really "edit mode" anymore, maybe "contextual show mode" :P?

Status: Needs review » Needs work

The last submitted patch, pencils-1874664-79.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

RE: dashed lines — that's in theory unrelated to this issue, but because we're trying to nail the experience here, I think it's acceptable to change it here.

I think the edit mode toggle is much more clear (with the blue + no label). I wonder since its a "mode" that ones toggles, it should maintain itself across pages?

Funny that you should say that, it was raised earlier today at #1882482-77: [meta] Unify editing approaches in Drupal:

It seems odd that when I toggle the upper right-hand "Edit" link that it does not persist when I change to a different node. My initial reaction was that this should either be always on when I select it or off when I don't.

We can do that — we'll just use localStorage to persist it.

Wim Leers’s picture

#79: pencils-1874664-79.patch queued for re-testing.

tkoleary’s picture

Hi Bojhan,

Just a couple of comments on your review:

1) The "contextual dotted line" should only appear on hover of the contextual link

Agree

2) We should probably remove the word "Edit", and just have the pen icon...

Testing yesterday showed the opposite. More affordance was needed to find this than just the icon.

3) I am still worried about the way we now toggle out of the mode...

There is a ned design for this in the prototype

4) Our indicator for "display all pencil icons" mode...

Updated in prototype and Wim added to the patch

5) If you open a contextual menu, it should stay open until you move outside of its zone

Yes, not sure why this regressed.

6) When an error occurs on quick edit loading, you are left with an eternally with "loading...."

Can't reproduce

7) The dotted line, does not return on pencil hover when you are in Quick edit mode.

8) Should we step away from using a shadow when you hover over an inline field?

I think that's a good idea. The shadow is obtrusive for smaller things.

9) Inline still feels quite jarring,..

I agree

10) I keep getting erros when I go to edit items inline, perhaps this was because of a time-out? My wifi was a little buggy.

I think it is a connectivity issue. I can't reproduce.

Wim Leers’s picture

FileSize
3.16 KB
53.95 KB

Fixed both things I said I would fix in #82:

  1. dashed outlines gone
  2. persistent edit mode, across pages, and after overlay is toggled on/off

P.S.: I love localStorage as of today. Bliss.

EDIT:
Try the patch below at http://simplytest.me/project/drupal/8.x, click "Advanced options", click "Add a patch" and paste the link to the patch!

jessebeach’s picture

Status: Needs review » Needs work
FileSize
5.03 KB
56.5 KB

This update adds a audio queue to the edit toggle. When edit mode is enabled, it announces the enabling of the mode and gives a count of the number of contextual items on the page. It also announces that tabbing will move between them, which doesn't work yet. That'll be the next update.

Wim Leers’s picture

Assigned: Wim Leers » jessebeach
Bojhan’s picture

This doesn't seem to apply at simplytest.me :(

Wim Leers’s picture

Then try #85, #86 is visually identical; it's all about a11y. (Unless you're trying to review that, of course.)

Bojhan’s picture

Both didn't work, I did exactly what you suggested.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
55.14 KB
Wim Leers’s picture

Status: Needs review » Needs work

Oops; I shouldn't have set it to NR. Sorry :(

jessebeach’s picture

FileSize
54.86 KB

After going back and forth on how to deal with the tabbing constraint issues for a keyboard user who has turned on edit mode, I decided to leverage the tabbing constraint code in overlay.

But in order to do that, I had to genericize that code first.

#1913086-2: Generalize the overlay tabbing management into a utility library

And now I don't want to make this issue dependent on a core change like that. So I pulled out the accessibility code in #91 and have placed it in a separate issue that is dependent on this issue.

#1913214: Accessibility followup for Edit tab toggle of contextual links for in-place editing.

The patch in the accessibility followup above is dependent on the tabbing changes in #1913086.

I realize it is not ideal to separate the accessibility changes from this patch. But here's my logic. If we just go ahead and implement a tabbing management system in this patch for contextual, then we have tabbing management in overlay and contextual. Managing that tabbing order of elements on a page is really an application level activity, not a module level activity, since changes to tabbing order affect everything on the page. So it makes sense, now that we have two modules that want to affect tabbing order, to move that code into a library and out of overlay. But I don't want this patch in this issue to depend on us agreeing to changes in overlay, especially when those changes are a little raw right now.

We're building a complex little interaction here and we can't just sprinkle accessibility on it. I want to make sure that we're considering the changes as we would any UI changes, in a forum that's focused on the efficacy of those changes. That's why I moved the accessibility work to #1913214.

I also realize that testing these changes will require apply three patches, which isn't ideal. So I will wrap them up into the Spark 8.x-1.x build shortly and provide an explanation of how to launch a demo build in the following comment.

sun’s picture

Quoting @Bojhan from #80:

@WimLeers I dont think you are right on #79.1) the reason contextual links are implemented as they are now, is because there goal was not to stand out. That's the whole point, it should seamlessly fit inside the act of just navigating around on your site. We made a conscious decision to make it small and not have the dotted line appear on hovering the zone, this because for much of your site building/content creating activities you are just browsing around - especially after the site is build. Having that additional effect is useful, but not really necessary.

I couldn't agree more. Thanks for highlighting that design tidbit, @Bojhan.

(btw, I wonder how we can document such details, since the lack of documentation clearly causes us to walk in circles and developers to "re-think" whole ideas ignoring prior art and history.)

jessebeach’s picture

The latest changes can (always) be tested at simplytest.me with the 8.x-1.x branch.

Wim Leers’s picture

#94:

  • Glad to hear you, Bojhan and tkoleary all agree on this. I'm clearly the exception :) Note that this is already implemented as of #85.1.
  • As I already said earlier, the dashed outlines on hovering the zone are already in D8, we weren't attempting to introduce that here! In D7, the outlines only appeared when hovering the contextual gear; do we want to make it work again like that in D8? As of #85, the outlines have been removed entirely!
Wim Leers’s picture

Assigned: jessebeach » Wim Leers
Status: Needs work » Needs review
FileSize
3.01 KB

Thanks, Jesse, for moving this patch forward on the a11y front! :) Attached is the interdiff for #91 -> #93.

Re-assigning to me since we're probably going tho shift focus back from a11y to the general UX changes, which I've been working on.

How can we move forward here? What still needs to be done?

Gábor Hojtsy’s picture

I think the patch looks generally pretty good on a code review, especially like that we remove lots of code from the "edit overlay" :) Some remarks (none of these are major per say, but many would block commit, eg. big chunk of code committed out :):

+++ b/core/modules/contextual/contextual.moduleundefined
@@ -6,6 +6,41 @@
+      // TRICKY: toolbar_pre_render_item() expects this.
+      '#options' => array('attributes' => array()),

I'd rather make that not expect this than needing to add dummy structures.

+++ b/core/modules/contextual/contextual.theme.cssundefined
@@ -10,39 +10,39 @@
+  right: 2px;
+  width: 28px;

Does this work for RTL? It does not seem like.

+++ b/core/modules/contextual/contextual.toolbar.cssundefined
@@ -0,0 +1,36 @@
+.js .toolbar .bar .contextual-toolbar-tab.tab {
+  float: right;
+}

This would need an /* LTR */ marking.

+++ b/core/modules/contextual/contextual.toolbar.jsundefined
@@ -0,0 +1,116 @@
+    /* Indicates whether the toggle is currently in "view" or "edit" mode. */
+    isViewing: true,
+    /* Indicates whether the toggle should be visible or hidden. */

These docs should use // ... no?

+++ b/core/modules/edit/edit.moduleundefined
@@ -39,42 +39,16 @@ function edit_permission() {
 /**
- * Implements hook_toolbar().
+ * Implements hook_contextual_links_view_alter().
  */
-function edit_toolbar() {
+function edit_contextual_links_view_alter(&$element, $items) {
   if (!user_access('access in-place editing')) {

I'd explain why is this tied to a relatively obscure hook in a comment :)

+++ b/core/modules/edit/edit.moduleundefined
@@ -174,6 +145,16 @@ function edit_preprocess_field(&$variables) {
+ *
+ * @todo Move towards hook_preprocess_entity() once that's available.

Is this a pre-commit todo or postcommit?!

+++ b/core/modules/edit/js/app.jsundefined
@@ -116,18 +111,30 @@
       // Manage the page's tab indexes.
+      /*
       if (newState === 'candidate') {
         this._manageDocumentFocus();
         Drupal.edit.setMessage(Drupal.t('In place edit mode is active'), Drupal.t('Page navigation is limited to editable items.'), Drupal.t('Press escape to exit'));
@@ -136,6 +143,7 @@

@@ -136,6 +143,7 @@
         this._releaseDocumentFocusManagement();
         Drupal.edit.setMessage(Drupal.t('Edit mode is inactive.'), Drupal.t('Resume normal page navigation'));
       }
+      */

Is we intend to remove this code, then remove it for good, don't comment it out :)

I understand this does not apply anymore and contextual module would get to introduce similar solutions in a different patch?!

+++ /dev/nullundefined
--- /dev/null
+++ b/core/modules/edit/js/views/contextuallink-view.jsundefined

The naming of this file is not very nice. Not sure what to do about it and if there are constraints we should follow which lead to "contextuallink"...

Wim Leers’s picture

FileSize
4.19 KB
54.66 KB

Thanks, #98! All addressed, where necessary, I added more information here:

I'd rather make that not expect this than needing to add dummy structures.

Agreed; fix is at #1908906: Remove unused code from toolbar_pre_render_item that throws a warning on custom themed tabs :)

Code comment updated to reflect that.

Is this a pre-commit todo or postcommit?!

Post. In-place editing of anything other than nodes is not yet possible because not a single entity type implements Drupal 8's access API. That's why edit.module currently still uses node_access() — see EditEntityFieldAccessCheck::accessEditEntityField().

Is we intend to remove this code, then remove it for good, don't comment it out :)

I understand this does not apply anymore and contextual module would get to introduce similar solutions in a different patch?!

See #93. This code is not going to be removed for good; it's going to be refactored in #1913214: Accessibility followup for Edit tab toggle of contextual links for in-place editing., but only after #1913086: Generalize the overlay tabbing management into a utility library is done. I clarified the comment there to refer to #1913214: Accessibility followup for Edit tab toggle of contextual links for in-place editing..

The naming of this file is not very nice. Not sure what to do about it and if there are constraints we should follow which lead to "contextuallink"...

True. All of edit.module's Backbone Views follow this naming scheme though. I'd much rather keep them all analogously named, so that we can introduce a better naming scheme in a later patch. We don't want to introduce unnecessary (non-topical) changes here.

Gábor Hojtsy’s picture

Well, I still don't think leaving code around commented out just because it will be refactored in other issues is a good idea. We can keep the code in a patch elsewhere on the other issues if we need it to be present for the refactoring effort. I don't think it has ever been common practice in Drupal to comment out code. If we really need to dig it up, we are using a version control system after all :)

Wim Leers’s picture

FileSize
2.9 KB
55.65 KB

I noticed a bug: the persistency introduced at #85 worked for the edit mode toggle, but it didn't actually cause the pencil icons to show up if edit mode was enabled, because contextual.js runs after contextual.toolbar.js and no drupalEditModeChanged event was being triggered because I was setting the initial value of the model to the target value, instead of setting it after initialization, which would trigger the event.

Another problem I noticed: the pencil icon does not disappear when you go into quick edit mode, because you keep hovering the node. If edit mode is enabled, then none of the pencil icons go away, ever, while doing in-place editing. Is this desirable? Shouldn't these two behaviors be implemented:

  1. If edit mode is enabled (i.e. all pencil icons visible), then as soon as you click "Quick edit", then all pencil icons beside the one you're doing quick edit on disappear. Reason: the user should *stop* quick editing before he can start manipulating something else.
  2. While editing a specific field in-place (i.e. after clicking "Quick edit" and then clicking on one of the fields), the pencil icon should disappear. Only when you're not in-place editing any specific field, the pencil icon is visible.

Thoughts?

Wim Leers’s picture

I forgot to attach the screenshot that goes with #101.

As usual, the latest patch is easily testable at http://simplytest.me/project/spark/8.x-1.x.

Screen Shot 2013-02-11 at 15.54.30.png

Wim Leers’s picture

RE: the problems I outlined in #101:

  • Bojhan and Jesse Beach told me they agree with the second point.
  • Bojhan agrees with only the second, Jesse agrees with both.

So, for now, I've implemented only #101.2. (Note that this also prevents the problem demonstrated in the screenshot at #102.)


As per the a11y follow-ups described at #93, which have mgifford's approval, and #98/#100, I'm also removing the current a11y code in edit.module.


As usual, the latest patch is easily testable at http://simplytest.me/project/spark/8.x-1.x.

Wim Leers’s picture

I talked to Bojhan yesterday afternoon about this issue. I said I found problems (the ones outlined in #101) that would be solved by implementing two behaviors; in his mind only the second one had to be implemented, because the first one would prevent fast switching between contextual tasks (and users wanting to do very fast context switching is very probable).

I asked whether besides the above, anything else was preventing this issue from being RTBC in his eyes.
His answer: yes, it's RTBC from a UX point of view, but it's incomplete (meaning it needs to be refined further).

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

@sun's @Bojhan's and my concerns were addressed, followups present to keep working on perfecting this interaction, so we reached as far as we could go here. Let's go with this!

Bojhan’s picture

Status: Reviewed & tested by the community » Needs review

Lets actually make followups first, I dont want to end up having loads of unfinished interactions with no followup issues. The next steps for this issue are:

1) Get this new concept usability tested, its such a different concept to "edit mode" we previously had, this needs new verification. Which I guess will need a e-mail to dharmesh, who leads up most UX testing around Drupal core.
2) Adress important details such as, A) dotted line upon hover of pencil, B) stress testing the current CSS in different themes, given that we have now eliminated local tabs - we better be sure, that people don't mess about with contextual links as much as they do in D7 - making it unpossible for content editors to edit. C) align edit mode toggle with other styling in the top menu bar

I feel, we need followups for 1, A, B, C and possibly also more for issues raised in this issue. I know 2 - B will be hard, but also critical if it introduces a regression when we employ this is in the real world.

I think with those follow ups, I am fine with this moving into core (RTBC) - since this is so inter-wrangled, I think it makes sense to commit things before we move to testing.

Gábor Hojtsy’s picture

Component: edit.module » other
Category: feature » task

@Bojhan: on your point 2B I'm not sure there are any Drupal 8 themes that we can test with. Themes listed at http://drupal.org/project/themes/?f[0]=drupal_core%3A7234&f[1]=bs_project_sandbox%3A0 either did not actually have 8.x dev releases even of very old (eg. 2011 nov). Given the pace of change in core and the level of rewrites needed for themes in Drupal 8 (especially given twig and the CSS cleanup resulting from HTML 5 and mobile changes) I don't think we should take any state of current themes as indicative of what is going to happen in Drupal 8 land. Testing with hypothetical future themes is not really possible though :)

Also moving to task since we are not actually adding or removing any features here. We are changing UI affordances for existing features. Also moving to "other" since we are dealing with edit module, toolbar module and contextual and tabs all at once, and could not find a generic user interface component to tie this to.

Gábor Hojtsy’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

I don't see an answer to the basic question I asked in #8: How will this work for non-administrators?

Case in point, I just applied the patch, installed Drupal, gave authenticated users permission to create and edit their own articles, then logged in as an authenticated user. I can create content, but don't see a link anywhere to edit it anymore.

If I give those users contextual links permissions, then they can edit it. However, (a) as a site builder the need to grant this permission to end users is very difficult to discover, and (b) contextual links really don't seem like they're designed with non-admin use in mind. Especially when you start thinking about things like wiki content...

This can also be a problem for admins, on a site with the contextual links and toolbar modules turned off. (For a simple example, try installing with the Minimal profile - there is no longer any way to edit content contextually, so basic use cases such as going back and forth between "view" and "edit" while making multiple adjustments to a piece of content can't be done anymore.)

tkoleary’s picture

@David Rothstien:

contextual links really don't seem like they're designed with non-admin use in mind

I think that is a fair point. They were not designed for non-admin use. Local task tabs are ideally suited to non-admin use for several reasons:

  1. They appear in the flow of the content on the users site
  2. They inherit styles from the users front-end theme
  3. They are visible at all times
  4. They are very discoverable for users unfamiliar with a site or with Drupal
  5. They use a ubiquitous interaction pattern

Contextual links on the other hand are really built for admin use. They are:

  1. Positioned out of the way of the normal flow of content
  2. Agnostic to the front-end theme
  3. Invisible until they are needed (hovered or toggled on)
  4. Less discoverable for non admins or non-Drupalists
  5. Less ubiquitous and in their interaction pattern

So it seems to me that the preferred method for providing editing options to unauthenticated users is local task tabs. However, if we want to provide site owners a flexible solution that does not conflict with admin tasks we need to:

  1. Provide the admin with a way to enable local task tabs on a per-node basis.
  2. Provide the admin with a way to customize tab visibility and order
  3. Automatically enable local task tabs globally if contextual module is disabled

If we can do that then I think we would have a solution that covers both the unauthenticated user use case and the disabled toolbar/contextual use case.

effulgentsia’s picture

Assigned: Wim Leers » Dries
Status: Needs review » Reviewed & tested by the community

I think this issue satisfies its scope, so setting to RTBC and assigning to Dries to evaluate whether #110 is a commit blocker.

as a site builder the need to grant this permission to end users is very difficult to discover

We can fix that in a follow up. Maybe even remove the "access contextual links" permission. We never had a "access local tasks" permission; why do we need an "access contextual links" permission?

contextual links really don't seem like they're designed with non-admin use in mind

What makes contextual links unfit for non-admin use? I think they're way more elegant than local tasks.

This can also be a problem for admins, on a site with the contextual links and toolbar modules turned off.

You can still go to admin/content, and edit content from there. If you've turned off contextual links, that's a reasonable consequence: i.e., you can't edit a node from the context of viewing one.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Committed to 8.x! Yay. I think all of the follow-ups have been created, except for a follow-up for the permission stuff. Let's also update the summary with all of the follow-ups. I'm setting this 'needs work' until that has been done.

Thanks everyone! :-)

catch’s picture

I didn't review this before it was committed, but it looks like it makes contextual links considerably more likely to be enabled than they currently are in 8.x, and on more things, so it'd be great to see more activity on #914382: Contextual links incompatible with render cache.

Wim Leers’s picture

Assigned: Dries » Wim Leers

#110: *great* points, but I wholeheartedly agree with this:

What makes contextual links unfit for non-admin use? I think they're way more elegant than local tasks.

What this means though, and which we hadn't considered before in this issue, is that contextual.module is now a de facto dependency if you don't want to go to admin/content first.

I'll create a follow-up issue for that tomorrow.

#114: Yes, I've kept that in mind all along, and I intend to make it work well and similar to how I solved it for Edit.module. I added that to our meta issue at #1858210: [meta] Content editing experience follow-ups — in-place editing and WYSIWYG to guarantee we don't forget about it *and* assigned it to myself to indicate I intend to work on this ASAP.

effulgentsia’s picture

sun’s picture

Category: task » bug
Priority: Normal » Major
FileSize
40.91 KB

I'm very negatively surprised how such a fundamental concern as #110 can be simply brushed away like this.

What's even more concerning is that the changed user interface focuses on a single Drupal use-case of editorial sites only. I cannot imagine this to work for any other use-cases. Like, community sites, like drupal.org?

I'm no longer able to edit a node. Unless I'm a developer and happen to know Drupal's URLs. How's that supposed to work?

We've added a dependency on contextual module for the most basic site operations. Why do we enforce the concept of contextual links on everyone?

Is our plan to make Drupal 8 the new Wordpress? Single-purposed? Full of assumptions? Tailored to a specific use-case? Can we do that in a distribution instead, please? Otherwise, how are distributions supposed to undo all of this? Will I have to install the UndoCore distro instead of Drupal core for all use-cases that can't make sense of these baked-in assumptions?

And when enabling contextual module...

tasks.png

Why are the contextual links attached to the node body, instead of the title, like everywhere else?

Also, if the idea was to turn the local tasks into contextual links, why did we recreate new ones instead of simply converting the existing?

It's a little beyond me how this patch could be marked RTBC, and, even get committed.

effulgentsia’s picture

Is our plan to make Drupal 8 the new Wordpress? Single-purposed? Full of assumptions? Tailored to a specific use-case?

I'm reading this as a rhetorical question, but for the benefit of people who might think it's serious, no, that's no one's plan. Any given issue might appear to move in that direction, but then we address those problems, with follow ups, like #1915730: Decide what to do about important contextual links when that module is disabled or restricted.

It's a little beyond me how this patch could be marked RTBC, and, even get committed.

This issue got quite a bit of review and quite a bit of support after incorporating many rounds of useful feedback. It's beyond me how Drupal UX could improve at all, let alone quickly enough to not be made irrelevant by systems with far better UX, if we required every issue to handle every ramification of a change.

Also, if the idea was to turn the local tasks into contextual links, why did we recreate new ones instead of simply converting the existing?

I don't think I follow. Can you elaborate?

David_Rothstein’s picture

I think this issue satisfies its scope, so setting to RTBC...

How does it satisfy its scope? In regard to local tasks, the issue summary says this: "The important thing is to remove the local tasks if they're not going to show up for normal users."

The patch here doesn't do that; instead it takes two particular local tasks and removes them for all users no matter what.

As you can see from @sun's screenshot above, even on nodes there are still going to be plenty of local tasks left over. Which is actually a usability problem of its own. Try turning on the Book module and creating a Book Page. Can you explain why "Edit" (where editing of the book takes place) is a contextual link, but "Outline" (where other editing of the book takes place) is a local task? That is extremely inconsistent.

What makes contextual links unfit for non-admin use? I think they're way more elegant than local tasks.

I'm sure there's room for debating this, but I think @tkoleary's list in #111 is a very good set of reasons. I'd like to say that I had every single one of those in mind already when I wrote #110, but not exactly :)

***

Anyway, I'm all for followups in general, but not here -- this is broken behavior and the fundamental problem was discussed the first day this issue was filed and there was agreement it needed to be dealt with, and the part that's causing the problem doesn't even help meet the original issue goal anyway.

Let's try to get to a position where the behavior is somewhat sensible, with followups to improve and polish it - not where things don't make sense and the followups exist to try to make sense of it. That just leads to confusion.

Working on a quick attempt to deal with this.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
773 bytes

Having trouble making a screenshot with contextual links open, but here's a textual description:

1. "Revisions" goes back to a tab.
2. "Edit" goes back to both a tab and contextual link.

Everything else remains the same; contextual links do show on the full page view (so "Edit" and "Quick edit" are both in there next to each other always) so it all works.

Clearly room for other followups (or continuation of the current issue, for things that were in the issue summary but never solved here), but I think this starts to get things to a sensible place to build off of.

Note related issue, by the way: #842328: No contextual links on full node view is confusing

Status: Needs review » Needs work

The last submitted patch, 1874664-120-followup.patch, failed testing.

Gábor Hojtsy’s picture

@David/@sun: David in fact pointed to a very good issue on the topic I wanted to elaborate on. (#842328: No contextual links on full node view is confusing). Reality is that Drupal displays these contextual links local tasks only for some high level objects and only if you view them on their own page. If you view a node in a Views view, in a block or as part of a Panels pane or a teaser in some other way, then you have no editing link. Also, if the high level object on the page (the main page object) is not a node or user, you pretty much don't get editing or other local tasks (eg. if your page is a view or panel). Also if you have anything displayed not as the main content object of your page (block, menu, etc.), you have no way to directly edit that either.

So the answer to "how do I edit this block", "how do I edit this view" or "how do I edit this node if its displayed with views (and the creator did not check to link title to the node page)" if you have no contextual links is the same: go to the admin page and try to find the object somehow to edit.

The proposed solution here makes it so regardless of where the node is displayed, whether it is a teaser or full node, and even whether it is a node page or view page or block or menu or panel or whatnot, you have the same way to do operations on it. You don't need to think "ah, this is a node, so I should go to it's page and edit it there" but "oh, this is a block so I'll need to manually find this in the admin UI". Instead of two competing affordances for operations on underlying page elements, people only need to learn one and can get to operations *faster* because these are even available on teaser views, etc.

@sun: this assumption that each Drupal site has nodes always linked to their main node pages, and nodes always have a main node page that the site builder wants to see publicly visible is in fact what is full of assumptions and tailored to a specific use case.

From the issue David linked:

Take this example - I have one node in a teaser listing - to me this is visually the same as looking at the full node view, but the widget/links I am forced to use are completely different. Pretty soon I learn that these *hover links* only appear in certain situations. This pisses me off because I now have to learn and use two different UI's to do the exact same thing - furthermore I cant find the option to turn them on all the time and disable these other annoying links that are messing up my node.

I fully agree the issue did not entirely cover removal of all local tasks for high level objects in favor of using contextual links all the time. I don't think stepping back to have them re-introduced is a step forward, because in-place editing is not discoverable in that case. It will obviously not become a local task, it is pointless to work on making it work as a local task. It should take for us less than an hour to come up with a patch to remove the rest of the local tasks, and that would bring us forward to this unified world where people only have one type of interaction for operations on Drupal things. Stepping back to have local tasks on nodes as before this patch I don't agree would be a sensible place. I don't think anybody would find the quick edit feature ever with the prominence of the local tasks present.

Bojhan’s picture

@David Could you explain "contextual links really don't seem like they're designed with non-admin use in mind. Especially when you start thinking about things like wiki content.."? I feel with this patch, that should enable contextual links for registered users - and all other roles but anonymous. It can still be optional given that you can decide to have a different way to navigate to these items; e.g. tabs. I agree that this might be a little hard of a pattern, for wiki sites - but I can imagine the theme decide to display these items as tabs?

@sun Sorry, but at #106 comments - the fact that we didn't get these kinds of comments is to me very revealing how disconnected core proceses is. That you often come in after commit, makes the whole process even more harder There was enough opportunity and criticality in this issue for earlier comments, watching the fixed queue is a bad practice - watch the needs review (major) queue instead :)

Anyways I am very open to discussing this further, but I am definitely not interested in stop-gap fixes like @David proposes. If there are concerns we should discuss them and solve them properly, rushing things is what tends to get us in these positions in the first place.

catch’s picture

I've re-opened #607244: Decrease performance impact of contextual links which I'd originally closed back in 2010 as not fixable for Drupal 7 without an API change. This is separate from the caching issue referenced above. Currently contextual links are tied to a menu_get_item() query for each link to check access, whereas they should really have been tied to objects and access checks on those objects (which are already available when creating the links).

Gábor Hojtsy’s picture

Status: Needs work » Needs review

#120: 1874664-120-followup.patch queued for re-testing.

Gábor Hojtsy’s picture

Given the extent of difficulty found with taxonomy terms, comments and users in #1915730: Decide what to do about important contextual links when that module is disabled or restricted, I give in that it is not actually a one hour job to convert them all to contextual links. In that light, I think moving all node related local tasks to contextual links (translate and book outline was not moved in core) is best to complete the direction we are taking. We'd get the crucial user feedback we need from developers involved with core.

If we'd go the other direction based on what David suggested, the actual contextual changes and the in-place editing would be obscured to close to invisible-ness. At minimum in that case I think revisions should go to contextual links too since that would round out contextual links.

The "Devel" example that @sun pointed out is merely a contrib module not properly changed yet, the same will be true for any other contrib module and any other change in core when the module is not yet up to date to latest core practices. So I don't consider that a viable argument.

Status: Needs review » Needs work

The last submitted patch, 1874664-120-followup.patch, failed testing.

Dave Reid’s picture

I strongly disagree with moving *everything* into contextual links for the issue brought up in #1915730-4: Decide what to do about important contextual links when that module is disabled or restricted. Local tasks are commonly used in the wild as navigation elements, and not administrative tasks. Views UI allows us to create arbitrary views at local tasks and they are commonly used as 'tabs' on nodes for more information, profile pages under users, etc. Replacing this pattern with contextual links seems contrary to the intent of contextual links.

effulgentsia’s picture

Status: Needs work » Needs review
effulgentsia’s picture

Status: Needs review » Needs work
David_Rothstein’s picture

I strongly disagree with moving *everything* into contextual links... Local tasks are commonly used in the wild as navigation elements, and not administrative tasks.

Good point. Furthermore, even if we only moved "actions" into contextual links (not everything) that would still be a huge number of links. If every time I click the contextual link icon anywhere on my site I am confronted with tons of choices, that makes it harder to use. The point of contextual links is to reveal the most common operations that you'd actually want to perform inline, not to reveal all of them. If I am looking at a node title outside of the book that contains it it's highly unlikely I'd want to adjust the book outline.

This is mentioned in Drupal's UI guidelines for contextual links too (http://drupal.org/node/1146108): "Minimize the number of operations".

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
1.45 KB

Fixing test failure in #120.

jstoller’s picture

I'd like to second @Dave Reid's point in #128. I'm guessing I use local tasks as navigation elements far more often than as administrative tasks and a don't want to see that go away. This patch just completely hosed what I have been working on in #1776796: Provide a better UX for creating, editing & managing draft revisions., and if you pull the rest of my local tasks into contextual links it would only make things much worse.

Gábor Hojtsy’s picture

@davereid, @jstoller: we are not proposing to remove the ability to add local tasks on nodes (or other objects). In fact both the patch committed here and the ones being experimented with in #1915730: Decide what to do about important contextual links when that module is disabled or restricted have the default View tab intact, so if any other tabs are added, the View tab will surface and the other added tabs will be there. The fundamental question I guess is whether they should definitely be there by default or not.

tkoleary’s picture

@jstoller

This patch just completely hosed what I have been working on in #1776796: Provide a better UX for creating, editing & managing draft revisions., and if you pull the rest of my local tasks into contextual links it would only make things much worse.

The addition of the entity toolbar to edit as proposed in http://drupal.org/node/1898946 and described in this prototype here: http://invis.io/FVB89STC reconciles this issue and I think we should work together to incorporate your work here into that.

It does not address the issue of "How do I create manage and edit draft revisions if I am an unauthenticated user" that's something we still need to think about.

effulgentsia’s picture

FileSize
3.19 KB

I haven't tested this patch yet, and I'd like to leave a more detailed comment here in the next day or two, but in the meantime, wanted to post this idea. It's a partial combination of #132 and #1915730: Decide what to do about important contextual links when that module is disabled or restricted. Essentially, the goal is to make things that make logical sense as contextual links into contextual links, but also fall back to tabs for users without access to contextual links. That still leaves open a discussion on what makes logical sense as contextual links.

David_Rothstein’s picture

I think the only problem with that is it means there will be situations where regular users see tabs but admin users don't. Since tabs can be an important part of the page layout, this could be confusing for the administrator.

However, it is not necessarily much worse than the opposite situation which frequently occurs now (admins see tabs but regular users don't).

Bojhan’s picture

No screens, so nothing to review - also still opposed to stop-gap fixes. We are almost in bug fixing mode, introducing stop-gap fixes is silly.

catch’s picture

I'm tempted to roll the original patch back and start again at this point.

effulgentsia’s picture

Is there any benefit to a full rollback vs. #132? I'm not crazy about #132, because I'd rather we move forward, not backward, but if people are feeling that HEAD's current lack of the Edit and Revision tabs is an unacceptable interim situation, I'd rather we just restore that (as #132 does) than roll back everything.

sun’s picture

It doesn't look like there is a concept, which explains why we're trying to duct-tape the effects of the committed patch, so I agree with rolling it back.

Bojhan’s picture

@sun Well the concept was to move everything to contextual links, everything. Arguably that has an impact, on the usability given that in many use cases local tabs are more discoverable. However I do not think the concept was wrong, however we did not convert everything - and we did not find a good way, to also expose these links as local tabs.

I however, do not really care about the interm state of core - changing navigation will forever be difficult, if it requires a rollback to get this to a nice state I am fine. However all the useful reviewers we have gotten in the past 30 comments, I would like those to remain involved - there is no point in our review system, if they only come in after commit, and solving these tricky UX issues is left to a team of two people.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.45 KB

All right, let's get #132 committed. That should give us some more peaceful space to figure out the border between contextual links and tabs and when to display which. I *personally* agree with Bojhan that Drupal does not need to be in an "always usable state" and it is better to experiment with a larger group of people than trying to always rely on 3-4 people on an issue to get everything right, but that always stable state and lack of experimentation on the mainline dev version is what our process is designed to achieve with its gates and thresholds.

Re-uploading David's #132 without any changes for testbot and to ensure there is no confusion as to which one I marked RTBC.

tkoleary’s picture

@Gabor Hojtsy

I think this is best for now. Redundancy is much less of a usability issue than missing functionality.

@sun

It doesn't look like there is a concept,

There is a concept (albeit arrived at late in the process in response to David's comments) this is it:

a) Local navigation and local tasks (actions) should be separate things, moved apart from one another and styled differently (per conversations yesterday between myself, Dries, Alex and Gabor). For example: on a d.o profile "edit" is clearly an action wheras "SSH keys" is navigation ("add a public key" is an action) thefore "SSH keys" could be on the left styled as a tab and "edit" could be on the left styled as a link or button.

b) Because local task tabs are used in many sites where users without admin privileges can edit their own content (like on d.o) they should be available for that purpose.

c) There should be a more granular way for site owners to configure where, how and for whom local tasks appear.

d) When an admin with contextual links privileges is authenticated the "action" local tasks automatically move in to the contextual links.

effulgentsia’s picture

FileSize
5.04 KB

To recap, the originally committed patch:
- Changed the contextual links gear icon to a pencil icon.
- Added a pencil icon to the toolbar that when toggled, makes all contextual region pencil icons visible (normally, they're only visible on hover over their region).
- Changed Edit module to launch Quick Edit mode via a contextual link rather than a toolbar tab.
- Made contextual links for nodes available from their primary node page in addition to other places that nodes appear.

Correct me if I'm wrong, but I don't get the sense that any of the above was controversial or is causing problems currently.

Additionally, the originally committed patch:
- Changed the node's Revision menu link from a tab to a contextual link.
- Changed the node's Edit menu link from a tab and contextual link to a contextual link only.

That's the part that's controversial, so that's what #143 reverts until we're able to address the problems raised. This patch also reverts a couple corollary test changes related to that. Leaving at RTBC since it's just a couple extra test roll backs.

effulgentsia’s picture

Good point. Furthermore, even if we only moved "actions" into contextual links (not everything) that would still be a huge number of links. If every time I click the contextual link icon anywhere on my site I am confronted with tons of choices, that makes it harder to use. The point of contextual links is to reveal the most common operations that you'd actually want to perform inline, not to reveal all of them. If I am looking at a node title outside of the book that contains it it's highly unlikely I'd want to adjust the book outline.

@tkoleary, @Bojhan: any thoughts on that? Do we want to distinguish between the set of contextual links that appears when the entity is displayed in some other context, vs. when it's displayed on its primary page?

This is mentioned in Drupal's UI guidelines for contextual links too (http://drupal.org/node/1146108): "Minimize the number of operations".

Specifically, it suggests an upper limit of 6. Meanwhile, http://drupal.org/node/1090012 suggests an upper limit of 4 for tabs. But with tabs, you can create groups and subtabs, with contextual links you currently can't, so something to consider.

sun’s picture

I agree with #145, though with one minor exception:

- Made contextual links for nodes available from their primary node page in addition to other places that nodes appear.

They're made available, but as mentioned in #117, at the wrong position in the page layout/output (somewhere within the content area instead of attached to the title where all other contextual links normally appear).

However, that's a minor issue in the grand scheme of things, which we can fix up separately.

Lastly, I also want to point out that the issue title is misleading. If it would have been "Replace all (administrative) local tasks with contextual links", then you could have been dead sure that I would have followed this issue much more closely. I and likely many others were not aware of the hidden scope of this issue, since it still talks about "Edit" stuff. As a consequence, the issues raised in #8, #110, and #117 would have been much more dominant in this discussion.

After committing #145, I almost guess it would make most sense to restart in a fresh issue that has a clearly defined scope right from the start.

jstoller’s picture

I know Drupal calls them "local tasks," but to me those tabs have always been more location than task. "View," "Edit," and "Revisions" are places first and actions second. The edit tab is where I go when I want to edit a node. The revisions tab is where I go when I want to see my revision history. Removing those tabs removes any visual indication of where I am and what other places I could go to. This is a huge blow to UX.

I'm not saying that contextual links are bad. Sometimes they're very appropriate, but they should be seen as enhancing, rather than replacing, the local tabs. I have no problem with there being an edit link in a contextual menu, but when I click on it and it takes me to an edit page, I need a visual indication that I am on the edit tab of this content. I also need to know how to click back to the view tab.

In the case of #1776796: Provide a better UX for creating, editing & managing draft revisions., I need to know at a glance that there is a published revision and a draft revision. I need to know which one I am currently on and how to click back and forth between the two. Pulling "Draft" into a contextual menu is ridiculous in this context.

I'm interested in the idea of separating local tasks (displayed as contextual links) and local navigation (displayed as tabs). In the end, I think we need to have some things that are just tabs, some things that are just contextual links and some things that are both tabs and contextual links.

I think it's also worth pointing out that redundancy isn't always bad. Some times its appropriate to provide multiple paths for accomplishing something. If I want to open a file on my computer, I can select the file, go to the "File" menu and select "Open." Or I can right-click on the file and select "Open." Or I can select the file and type command-O. Or I can double-click on the file. Or I can open it through a terminal application. Yet somehow, I have never been confused about how to open a file. Some people choose one method and some people chose another, but ultimately they end up in the same place. Having an edit tab on nodes that users can go to, while simultaneously having an edit link in a contextual menu, is not necessarily a bad thing.

Wim Leers’s picture

#147: RE "misleading issue title" — absolutely. All of us in this issue failed to update it after its scope broadened. Apologies.

Bojhan’s picture

I agree, moving on with the strategy outlined in #147. I guess its good the issue title wasn't solution centered but we did indeed fail to update it, we simply forgot as Wim Leers points out.

@effulgentsia We need categories in contextual links, with <hr> like separators, than the maximum doesn't apply anymore. Thanks for outlining that not everything we imagined was wrong :) Baby steps, in the right direction hopefully.

swentel’s picture

Minor bug with this: if you're now on node/x and you hit delete, the destination will bring you back to node/x which doesn't exist anymore at that point.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Committed to 8.x. Moving to 'needs work', I guess.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community

To clarify, #151 is a bug in HEAD unaffected by #145, but yes, that is an interesting side effect of showing contextual links on the node's primary page that we should deal with in a follow up.

effulgentsia’s picture

Category: bug » task
Priority: Major » Normal
Status: Reviewed & tested by the community » Needs work

#153 is xpost. Back to needs work for all follow ups to be created and referenced in the issue summary.

jstoller’s picture

Priority: Normal » Major
Issue tags: +Needs usability review, +Usability, +workflow

I still see this as a major UX issue. I hope the separation of contextual links from local navigation tabs, and reinstatement of the later, will be considered a high priority.

Gábor Hojtsy’s picture

Priority: Major » Normal

@jstoller: already reinstated. See above.

jstoller’s picture

Are the various menu callback functions aware of what context a local task is being rendered in? I can easily see cases where I may want the title, access, and page callbacks to behave differently, for the same item, depending on whether it is being used for a tab or in a contextual menu. If this is currently possible, then how would I access the context from within the callback?

David_Rothstein’s picture

To recap, the originally committed patch:
....
- Added a pencil icon to the toolbar that when toggled, makes all contextual region pencil icons visible (normally, they're only visible on hover over their region).
....
Correct me if I'm wrong, but I don't get the sense that any of the above was controversial or is causing problems currently.

I just watched some of the videos at #1678002-45: Edit should provide a usable entity-level toolbar for saving fields and noticed some things that I wasn't sure where to mention, but they're directly related to the above so I thought I'd leave the comment here for starters.

I didn't watch all of the videos in full, but I did watch the initial interactions for each one, which is what matters for this comment. Basically I just looked at what each participant did initially when they were faced with the task of editing content on the page:

  1. Participant #1 went straight for the contextual links on the node and the "full edit" option, making his way to the node edit screen. His initial inclination was not to use either the Edit link in the toolbar, or the inline editing functionality.
  2. Participant #2 tried to use the "Menu" link in the toolbar. Because the prototype didn't support that functionality, she wasn't able to, but I'm guessing if she had been she would have wound up on the Content Administration screen and edited the content via the regular edit screen from there. Her second choice was the Edit link in the toolbar; in the video, she looks like she's about to explain why she initially didn't choose that one, but then her explanation gets cut off: "The other reaction would be to go to 'Edit', but I feel that's so [inaudible]" :( After finding the Edit link, she goes down to the content and uses inline editing.
  3. Participant #3's first reaction seemed to be to hover over the text (i.e., perhaps looking for contextual links). However, they did not appear - probably a bug in the prototype that they appeared for participant #1 but not for participant #3. In Drupal itself, of course, they would have appeared. (And I am guessing based what transpired later in the video he would have found them if they had appeared, since later on when a pencil icon appeared next to the text on the page he went to it pretty quickly.) Anyway, after hovering there for a little bit trying to figure out how to edit, he says he has "no clue how to do it". At that point, he scrolls up to the top of the page and finds the Edit link in the toolbar right away. However, he then comments that although he has not used this interface before, he had previously seen a presentation on the Spark project, and he knew to look for it in the toolbar. Unfortunately that probably taints the result a bit; combined with the fact that his first inclination was to hover on the text, I'm pretty sure contextual links were his natural first choice. (I also wonder if any of the other testers were in a similar situation with regard to preexisting knowledge of the interface.)
  4. Participant #4 went straight for the edit link in the toolbar, and then used inline editing.

So the conclusions are a little muddied, but overall, only one participant (#4) clearly gravitated towards the Edit link in the toolbar. Contextual links (which also are consistently used in Drupal for other actions unrelated to editing) seem to be preferred. And this is despite the fact that the prototype which was tested has a much more visible "Edit" link in the toolbar than what's currently in Drupal 8 (it has the word "Edit" displayed next to the pencil icon, not just the icon itself).

I am not sure what to do with those observations, but thought they were worth mentioning.

Bojhan’s picture

@David Could you place this comment in the other issue? #1678002: Edit should provide a usable entity-level toolbar for saving fields I'd like to respond, but I dont want to have two super similar discussions in two issues.

David_Rothstein’s picture

Bojhan, I could, but the reason I didn't was that my observations were primarily focused on whether the pencil icon in the administrative toolbar is a useful interaction pattern or not.

That icon is already in Drupal 8, and as far as I understand the issue linked to above doesn't propose changing its behavior?... so that's why I didn't leave the comment there. However, if you think any part of what I wrote is related to that issue, definitely feel free to quote it in the discussion there and/or move it there :)

Bojhan’s picture

Title: Reconcile "Edit" toolbar option with local tasks (tabs) and contextual links for editing » Introduce toolbar level "Edit" mode that shows all contextual links

@David Ok, so I also noticed those results (I watched all the videos) but clearly they are too muddy to draw conclusions from. Hence why the report didn't.

I think the conclusion you draw, however is in line with what we have proposed here. Is to make the "edit inline" action linked to contextual links, because it is 1) not always common people go to "Edit" in the top toolbar, 2) people gravitate to find actions close to the thing they are looking at, which is why contextual links worked in the first place. Am I correct in this conclusion?

I am not sure if we are going to transfer over all the changes to contextual links as portrait in the prototype, to Drupal core - mostly notable the position and seemingly the removal of all other links (which I think is just a bug).

David_Rothstein’s picture

Yes, I agree those videos suggest that adding this to the contextual links was a good idea. (Though they don't really test whether people will understand the difference between "Edit" and "Quick edit".)

I was mainly curious if they also suggest that the Edit link in the top toolbar is unnecessary. But I agree that if so, it is not an ovewhelming conclusion.

Wim Leers’s picture

The "Edit" toggle in the toolbar is essential for keyboard-based navigation (i.e. a11y) and touch devices.

David_Rothstein’s picture

In Drupal 8, contextual links are already supposed to be keyboard-accessible and touch-accessible:
#849926: links wrapped in .contextual-links-wrapper divs are not accessible at all via keyboard alone also problems with screen readers
#1138844: Add touch support to contextual links

If not, I think one or both of those issues needs to be reopened.

Wim Leers’s picture

#164: they were barely usable on touch devices. Now they are properly/efficiently usable for both touch and keyboard users.

To clarify, please watch the screencast at http://wimleers.com/talk-drupal-speaks/#/7/9

@Bojhan, @yoroy: how do you think we should move this issue forward, what still needs to happen here?

webchick’s picture

Status: Needs work » Fixed

I agree, I don't really see anything else to do here. If I'm wrong, please file a follow-up and cross-link it here.

Wim Leers’s picture

Issue tags: -Needs usability review

.

Automatically closed -- issue fixed for 2 weeks with no activity.

Wim Leers’s picture

Component: other » contextual.module