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
- Make the "Edit" link in the toolbar act like the others links and provide a submenu.
- 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).
- The Edit module's link becomes "Quick Edit" or similar.
- We could potentially add useful quick toggles to access common meta-data, specifically demonstrated here as toggling the published state.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#145 | 1874664-145-followup.patch | 5.04 KB | effulgentsia |
#143 | 1874664-132-followup.patch | 1.45 KB | Gábor Hojtsy |
#136 | 1874664-136-followup.patch | 3.19 KB | effulgentsia |
#132 | 1874664-132-followup.patch | 1.45 KB | David_Rothstein |
#120 | 1874664-120-followup.patch | 773 bytes | David_Rothstein |
Comments
Comment #1
jcisio CreditAttribution: jcisio commentedA great proposition! Only one question: should Devel still be there? I think only the Edit local task should be removed.
Comment #2
quicksketchFor 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.
Comment #3
nod_Really like that idea.
Comment #4
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #5
Wim LeersI've asked Kevin (@tkoleary) to comment here.
Comment #6
tkoleary CreditAttribution: tkoleary commentedI 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)
2)
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.
3)
We have been working on designs for this and will be releasing a blog post within a day or two.
Comment #7
quicksketchRight, 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.
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedHm... "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?
Comment #9
Bojhan CreditAttribution: Bojhan commented@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.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, 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).
Comment #11
Wim LeersThis is not true. They do different things. One is for in-place editing, one is for back-end editing.
Comment #12
quicksketchI 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.
Comment #13
quicksketchWe 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.
Comment #14
tkoleary CreditAttribution: tkoleary commented@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).
Comment #15
tkoleary CreditAttribution: tkoleary commentedAh, I was wondering why that was. I thought I had seen that functionality before.
Comment #16
sunJust 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.
Comment #17
mallezieI 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).
Comment #18
Wim LeersRE 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?
Comment #19
Bojhan CreditAttribution: Bojhan commented@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.
Comment #20
webchickCross-linking #1882482: [meta] Unify editing approaches in Drupal. These might now be duplicates, I'm not sure.
Comment #21
Gábor HojtsyAs 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:
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:
When clicking the pencil on an in-place editable field, it does in-place editing:
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:
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.
Comment #22
Gábor HojtsyBTW a clickable version of the "pencilification" prototype is posted at #1882482: [meta] Unify editing approaches in Drupal, my screenshots above are based on that.
Comment #23
hass CreditAttribution: hass commentedOk, 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).
Comment #24
tkoleary CreditAttribution: tkoleary commented@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.
Comment #25
quicksketchSeems 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.
Comment #26
quicksketchOr 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.)
Comment #27
Gábor HojtsyYes, 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?
Comment #28
tkoleary CreditAttribution: tkoleary commented@Gabor Hojtsy Yes. ETA on that is sometime later today.
Comment #29
catchEnabling/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.
Comment #30
Gábor Hojtsy@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? :)
Comment #31
tkoleary CreditAttribution: tkoleary commentedDesktop 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)
Comment #32
Wim Leers#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.
Comment #33
Gábor Hojtsy@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
Comment #34
catch@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.
Comment #35
Gábor Hojtsy@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).
Comment #36
tkoleary CreditAttribution: tkoleary commented@wim leers I agree. That would make the experience much smoother
Comment #37
tkoleary CreditAttribution: tkoleary commented@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.
Comment #38
tkoleary CreditAttribution: tkoleary commented@catch ok for the moment but ultimately the toggle *has to* be there or the touch experience falls apart.
Comment #39
LewisNyman@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.
Comment #40
jessebeach CreditAttribution: jessebeach commentedA very similar discussion with an in-progress patch is happening at #1882482: [meta] Unify editing approaches in Drupal.
Comment #41
Gábor HojtsyWell, 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?
Comment #42
Gábor Hojtsy#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*.
Comment #43
Gábor HojtsyComment #45
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #46
Gábor HojtsyThis sounds pretty good for me personally.
Comment #47
tkoleary CreditAttribution: tkoleary commentedYes, 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?
Comment #48
Dave ReidI 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'?
Comment #49
Damien Tournoud CreditAttribution: Damien Tournoud commentedWasn'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.
Comment #50
Wim LeersDave, 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/:
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.
Comment #51
Bojhan CreditAttribution: Bojhan commented@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.
Comment #52
tkoleary CreditAttribution: tkoleary commented@Bojhan I think so. Still editing that design though. My attention has been focussed on this one http://drupal.org/node/1898946 (just updated)
Comment #53
Gábor Hojtsy@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 :)
Comment #54
Bojhan CreditAttribution: Bojhan commentedCan we get an initial implementation for this going?
Comment #55
Gábor Hojtsy@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.
Comment #56
jessebeach CreditAttribution: jessebeach commentedI'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.
Comment #57
Gábor HojtsyComment #58
Gábor HojtsyI'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.
Comment #59
Wim LeersI'll continue working on this.
Comment #60
Wim LeersSmall 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).
Comment #62
Gábor HojtsyI 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?
Comment #63
Gábor HojtsyDug 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.
Comment #64
Wim LeersNotes about #60:
/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:
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:
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 is essentially what I implemented here.
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.
Comment #65
Wim LeersAddressed #63.
Comment #66
Wim LeersProblems reported:
Fixing these.
Comment #68
Gábor HojtsyI 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 :)
Comment #69
Gábor HojtsyFound 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.
Comment #70
Wim LeersFixed #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).
Comment #71
Wim LeersThis 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!
Comment #73
Wim LeersBackbone.Router
and disabledBackbone.history
, because without a global edit mode toggle, this makes far less sense.contextual.toolbar.js
runs beforeoverlay-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.)Still TODO:
(Spark D8 not yet updated because it's functionally identical.)
Comment #74
Wim LeersI 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.)
Comment #75
Bojhan CreditAttribution: Bojhan commented@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
Comment #76
Wim LeersThanks, that's very helpful! I'll answer everything tomorrow, and adjust the code where necessary :)
Comment #77
jessebeach CreditAttribution: jessebeach commentedre:#69, this is not the Toolbar module doing this little bit of magic, it's Overlay.
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.
Comment #78
jessebeach CreditAttribution: jessebeach commentedThis solves #69 issue with the Edit toggle.
Comment #79
Wim Leers#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!
Comment #80
Bojhan CreditAttribution: Bojhan commented@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?
Comment #82
Wim LeersRE: 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.
Funny that you should say that, it was raised earlier today at #1882482-77: [meta] Unify editing approaches in Drupal:
We can do that — we'll just use localStorage to persist it.
Comment #83
Wim Leers#79: pencils-1874664-79.patch queued for re-testing.
Comment #84
tkoleary CreditAttribution: tkoleary commentedHi Bojhan,
Just a couple of comments on your review:
Agree
Testing yesterday showed the opposite. More affordance was needed to find this than just the icon.
There is a ned design for this in the prototype
Updated in prototype and Wim added to the patch
Yes, not sure why this regressed.
Can't reproduce
I think that's a good idea. The shadow is obtrusive for smaller things.
I agree
I think it is a connectivity issue. I can't reproduce.
Comment #85
Wim LeersFixed both things I said I would fix in #82:
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!
Comment #86
jessebeach CreditAttribution: jessebeach commentedThis 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.
Comment #87
Wim LeersComment #88
Bojhan CreditAttribution: Bojhan commentedThis doesn't seem to apply at simplytest.me :(
Comment #89
Wim LeersThen try #85, #86 is visually identical; it's all about a11y. (Unless you're trying to review that, of course.)
Comment #90
Bojhan CreditAttribution: Bojhan commentedBoth didn't work, I did exactly what you suggested.
Comment #91
Wim LeersThey didn't apply anymore because patch machine Jesse Beach got both #1910008: When launching the overlay, a contextual menu remains visible when it should become hidden and #1908624: Update edit modules implementation of hook_toolbar; refresh the design to match current mockups committed :)
Straight reroll of #86.
Comment #92
Wim LeersOops; I shouldn't have set it to NR. Sorry :(
Comment #93
jessebeach CreditAttribution: jessebeach commentedAfter 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.
Comment #94
sunQuoting @Bojhan from #80:
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.)
Comment #95
jessebeach CreditAttribution: jessebeach commentedThe latest changes can (always) be tested at simplytest.me with the 8.x-1.x branch.
Comment #96
Wim Leers#94:
Comment #97
Wim LeersThanks, 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?
Comment #98
Gábor HojtsyI 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 :):
I'd rather make that not expect this than needing to add dummy structures.
Does this work for RTL? It does not seem like.
This would need an /* LTR */ marking.
These docs should use // ... no?
I'd explain why is this tied to a relatively obscure hook in a comment :)
Is this a pre-commit todo or postcommit?!
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?!
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"...
Comment #99
Wim LeersThanks, #98! All addressed, where necessary, I added more information here:
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.
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()
— seeEditEntityFieldAccessCheck::accessEditEntityField()
.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..
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.
Comment #100
Gábor HojtsyWell, 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 :)
Comment #101
Wim LeersI 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 aftercontextual.toolbar.js
and nodrupalEditModeChanged
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:
Thoughts?
Comment #102
Wim LeersI 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.
Comment #103
Wim LeersRE: the problems I outlined in #101:
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.
Comment #104
Wim LeersI 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).
Comment #105
Gábor Hojtsy@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!
Comment #106
Bojhan CreditAttribution: Bojhan commentedLets 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.
Comment #107
Gábor Hojtsy@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.
Comment #108
Gábor HojtsyOpened #1914966: Nested contextual links triggers don't work well for doing more user testing.
Comment #109
Wim LeersSo to address #106:
1) #1914966: Nested contextual links triggers don't work well (as per #108)
2)
A. #1914976: Dashed outlines around contextual region should only appear upon hover/focus of the pencil icon or links
B. No follow-up (as per #107).
C. #1914984: [Policy, no patch] contextual.module's "edit/show pencils mode" Toolbar tab toggle styling: one-off or new pattern?
With the proper follow-ups created, I believe we can go back to RTBC.
Comment #110
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.)
Comment #111
tkoleary CreditAttribution: tkoleary commented@David Rothstien:
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:
Contextual links on the other hand are really built for admin use. They are:
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:
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.
Comment #112
effulgentsia CreditAttribution: effulgentsia commentedI think this issue satisfies its scope, so setting to RTBC and assigning to Dries to evaluate whether #110 is a commit blocker.
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?
What makes contextual links unfit for non-admin use? I think they're way more elegant than local tasks.
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.
Comment #113
Dries CreditAttribution: Dries commentedCommitted 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! :-)
Comment #114
catchI 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.
Comment #115
Wim Leers#110: *great* points, but I wholeheartedly agree with this:
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.
Comment #116
effulgentsia CreditAttribution: effulgentsia commentedRe #110 / #111: #1915730: Decide what to do about important contextual links when that module is disabled or restricted.
Comment #117
sunI'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...
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.
Comment #118
effulgentsia CreditAttribution: effulgentsia commentedI'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.
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.
I don't think I follow. Can you elaborate?
Comment #119
David_Rothstein CreditAttribution: David_Rothstein commentedHow 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.
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.
Comment #120
David_Rothstein CreditAttribution: David_Rothstein commentedHaving 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
Comment #122
Gábor Hojtsy@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 linkslocal 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:
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.
Comment #123
Bojhan CreditAttribution: Bojhan commented@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.
Comment #124
catchI'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).
Comment #125
Gábor Hojtsy#120: 1874664-120-followup.patch queued for re-testing.
Comment #126
Gábor HojtsyGiven 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.
Comment #128
Dave ReidI 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.
Comment #129
effulgentsia CreditAttribution: effulgentsia commentedAnother related follow up: #1916516: Decide whether/how to implement contextual links associated with the main page content.
Comment #130
effulgentsia CreditAttribution: effulgentsia commentedComment #131
David_Rothstein CreditAttribution: David_Rothstein commentedGood 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".
Comment #132
David_Rothstein CreditAttribution: David_Rothstein commentedFixing test failure in #120.
Comment #133
jstollerI'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.
Comment #134
Gábor Hojtsy@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.
Comment #135
tkoleary CreditAttribution: tkoleary commented@jstoller
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.
Comment #136
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #137
David_Rothstein CreditAttribution: David_Rothstein commentedI 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).
Comment #138
Bojhan CreditAttribution: Bojhan commentedNo 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.
Comment #139
catchI'm tempted to roll the original patch back and start again at this point.
Comment #140
effulgentsia CreditAttribution: effulgentsia commentedIs 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.
Comment #141
sunIt 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.
Comment #142
Bojhan CreditAttribution: Bojhan commented@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.
Comment #143
Gábor HojtsyAll 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.
Comment #144
tkoleary CreditAttribution: tkoleary commented@Gabor Hojtsy
I think this is best for now. Redundancy is much less of a usability issue than missing functionality.
@sun
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.
Comment #145
effulgentsia CreditAttribution: effulgentsia commentedTo 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.
Comment #146
effulgentsia CreditAttribution: effulgentsia commented@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?
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.
Comment #147
sunI agree with #145, though with one minor exception:
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.
Comment #148
jstollerI 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.
Comment #149
Wim Leers#147: RE "misleading issue title" — absolutely. All of us in this issue failed to update it after its scope broadened. Apologies.
Comment #150
Bojhan CreditAttribution: Bojhan commentedI 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.Comment #151
swentel CreditAttribution: swentel commentedMinor 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.
Comment #152
Dries CreditAttribution: Dries commentedCommitted to 8.x. Moving to 'needs work', I guess.
Comment #153
effulgentsia CreditAttribution: effulgentsia commentedTo 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.
Comment #154
effulgentsia CreditAttribution: effulgentsia commented#153 is xpost. Back to needs work for all follow ups to be created and referenced in the issue summary.
Comment #155
jstollerI 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.
Comment #156
Gábor Hojtsy@jstoller: already reinstated. See above.
Comment #157
jstollerAre 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?
Comment #158
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
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.
Comment #159
Bojhan CreditAttribution: Bojhan commented@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.
Comment #160
David_Rothstein CreditAttribution: David_Rothstein commentedBojhan, 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 :)
Comment #161
Bojhan CreditAttribution: Bojhan commented@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).
Comment #162
David_Rothstein CreditAttribution: David_Rothstein commentedYes, 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.
Comment #163
Wim LeersThe "Edit" toggle in the toolbar is essential for keyboard-based navigation (i.e. a11y) and touch devices.
Comment #164
David_Rothstein CreditAttribution: David_Rothstein commentedIn 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.
Comment #165
Wim Leers#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?
Comment #166
webchickI 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.
Comment #167
Wim Leers.
Comment #169
Wim Leers