Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Not sure exactly when this started, but noticed it today...
If you start quick edit mode from e.g. node/1, while you see the blue outline and floaty bar on node body, node title is lacking it. Weirdly, it shows up fine on teaser view on the front page.
I don't notice any errors in the JS console.
Comment | File | Size | Author |
---|---|---|---|
#41 | entity_full_page_label_in_place-2216437-41.patch | 19.52 KB | Wim Leers |
Comments
Comment #1
webchickTalked about this with Wim some. It's a known issue, and related to #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title) but will have a different solution, so we decided to keep this one open.
Chat excerpt:
Wim Leers
we *did* make the title in-place editable, and that *does* still work. (Just tested + confirmed.) There's one huge caveat. Drupal has this crazy crazy crazy thing where if you're looking at "the full entity page" (e.g. node/5 or taxonomy/term/3), then the title is *removed* from the entity display, and is "promoted" to the *page* title. And then the necessary metadata is missing of course, hence breaking in-place editing.
Therefore, title in-place editing works everywhere except on "the full entity page".
Angela Byron
10:29 Wim Leers: ugh. can we fix that?
10:29 Cos as an end user it just looks busted
Wim Leers
Angela Byron: it's very very very difficult
10:29 Angela Byron: I know
Angela Byron: and so far, it's AFAIK impossible to set additional attributes on page title
Comment #2
Wim LeersTests still needed, but this fixes it — generically, for all entities. Major thanks to plach & Berdir!
Comment #3
Wim LeersComment #5
Wim LeersFix fatal.
Comment #7
Wim LeersNow Edit's JS is also smart enough to initially position itself against the title element (in fact, whichever the topmost element is).
Comment #9
Wim LeersChasing HEAD.
Comment #11
Wim Leers#9 rerolled #5 instead of #7…
#wimfail
Comment #13
dawehnerAn explicit webtest for this would be helpful.
Let's adapt the documentation a bit
Why do we use the generic EntityInterface in contrast to NodeInterface in the previous patches? All the code requires nodes anyway
Should we also move the title_callback onto the new class?
Comment #14
Wim LeersNodeInterface
instead causes a strict warning:Strict warning: Declaration of Drupal\node\Controller\NodeViewController::view() should be compatible with Drupal\Core\Entity\Controller\EntityViewController::view(Drupal\Core\Entity\EntityInterface $_entity, $view_mode = 'full', $langcode = NULL)
.EntityViewController
but also still specifies its own title callback. Fixing it here implies fixing it there, and then the scope growsLast test failures fixed.
Comment #16
Wim LeersComment #17
dawehnerSorry but I don't see the point. The title callback was easy to find previously, because it was simply in the same file. Well, I cannot RTBC this patch anyway as it contains javascript changes.
Comment #18
Wim LeersGood point regarding JavaScript. Tagging as such, to get sign-off from e.g. nod_.
What don't you see the point of?
Comment #19
Wim Leers16: entity_full_page_label_in_place-2216437-16.patch queued for re-testing.
Comment #20
Wim LeersCritical because blocking the critical #2091401: Add hook_help for Quick Edit module.
Comment #21
Wim LeersComment #22
Wim LeersComment #23
xjmNot really related to the beta criteria, so untagging. Let's reach out to specific folks for reviews.
Comment #24
Gábor HojtsyJust reviewed this. First, hats off! I think its a testament to the new rendering and the page system that this can be implemented with relatively small amount of code specifically crafted for the feature. Wow.
This is quite a bit too specific on the theme. Should we just assert on an xpath that an element with this data-edit-field-id exists and has the expected title? Or would that be not enough to ensure the editability?
Its not clear to me how these changes are related?
Nice that this is now its own controller :)
Because the review points are not necessarily things to be changed, not moving the issue status.
Comment #25
Wim LeersThanks for the review!
Comment #26
Gábor HojtsyWhile that full markup matching something I have seen elsewhere indeed, I've also seen xpath matching used much. Anyway, I don't want to hold back a critical bugfix on that.
I also looked into @dawehner's concern. The title callback here used to be on Node's on controller, from where the new view controller was born. I don't think moving over the title callback for the view page to the Node view controller implies needing to fix it for the user view controller either, which is what @dawehner means AFAIS. The view code is moved anyway and this patch works with the title :) Not seeing how that would not be in scope then.
Also still needs JS review :)
Comment #27
dawehnerIf you agree that relying on the HTML structure might cause issues in the future it IS always worth to put any effort in NOW when we know that a problem might exist.
Figuring out the problem later is always a order of magnitude harder.
My point is simple: We had the related code in a similar place before, with your patch we now longer have. It is as simple as that.
Comment #28
Wim LeersAll concerns addressed.
Comment #30
Wim Leers28: entity_full_page_label_in_place-2216437-16.patch queued for re-testing.
Comment #31
Gábor HojtsyCan you post an interdiff of the changes for easier review? Thanks!
Comment #33
Wim LeersI moronically uploaded the wrong patch in #28 — I simply reuploaded the same patch as in #16 :D
Here it is again, with interdiff this time!
Comment #34
Gábor HojtsyLooks great! Thanks for resolving those concerns.
Comment #35
catchThe comment could be more specific. Instead of what? (also see below)
Also won't this break once #pre_render caching is in? $page can't be guaranteed to hold $label_field then, it'll just be #cache and #pre_render etc.
I think this needs a $entity->$label_field->view() so that we really are just rendering the field itself.
Why setting up the title here, then overridding when the page is viewed. Is it because we can't do the full formatter render here? Could use a comment I think as to why we set the page title twice.
Comment #36
Wim Leers#35:
_title_callback
may be used in non-HTML circumstances as well: to get the plain text title for listings, for REST calls, and whatnot. OTOH, the$page['#title']
mechanism allows us to specify a "rich HTML title" in the case where we know it's going to be a HTML page.Also: we don't have access to the view mode in
NodeViewController::title
, which is essential.Comment #38
Wim LeersTest failures were due to simple whitespace mismatch, and a lack of translation introduced by #36.
Comment #39
Gábor HojtsyChanges look good. Still looks ready as per our prior reviews.
Comment #41
Wim LeersUgh, one new fail. In
Drupal\entity_reference\Tests\EntityReferenceFormatterTest
, which didn't exist previously, hence the new fail.The fail happens because there is now a new field formatter being rendered. Adjusted the tests to cope with that, and minimized the number of changes.
Comment #42
Gábor HojtsyWhile (as discussed above) I don't think the so exact (to the whitespace) rendering test is dangerous and prone to fail on unrelated things (as it did here :), I see the changed test already does that. Let's fix it then and get this critical done finally.
Comment #43
nod_Js is ok with me.
Comment #44
catchThat's reasonable on _title_callback.
Committed/pushed to 8.x, thanks!
Comment #46
Wim LeersYAY! :)