Don't hide comments during in-place editing of an Article; position the entity toolbar more intelligently [#2137005]

Problem/Motivation

When we originally put Edit in Place in Drupal core, we made a UI concession regarding the placement of the Entity Toolbar. Since comments are part of an entity, and because, as a default, the Entity Toolbar is positioned against the dimensions of the entity container, the Toolbar would appear at times to fly to the bottom of the screen, below the comments, if it did not have enough space at the top of the screen.

We solved this by hiding comments when in-place editing is active.

But this solution is not ideal, since any field could be added to an entity that does not allow for in-place editing, including comments, which opt out now with #2136895: Comment settings are now a field, but not an editable one: breaks in-place editing .

So, what we should really be doing is position the Entity Toolbar only as far above the entity or below the entity as the last editable field.

Proposed resolution

Add logic to check for the position of the first and last editable fields in the entity and don't allow the Entity Toolbar to be positioned any farther than these boundaries.

Remaining tasks

Review the patch.

User interface changes

Comments are visible during in place editing and the Entity Toolbar positions itself in a more intuitive way.

API changes

None.

#2136895: Comment settings are now a field, but not an editable one: breaks in-place editing

Original report by @jessebeach

Comment	File	Size	Author
#11	comments-edit-patch-2.png	69.02 KB	jessebeach
#11	unhide-comments-2.png	229.34 KB	jessebeach
#11	comments-are-hidden-2.png	34.01 KB	jessebeach
#10	Screen Shot 2013-11-23 at 7.41.57 PM.png	37.14 KB	webchick
#8	interdiff.txt	749 bytes	jessebeach
#8	unhide-comments-2137005-7.patch	2.84 KB	jessebeach
#8
#4	interdiff.txt	2.42 KB	jessebeach
#4	unhide-comments-2137005-2.patch	2.9 KB	jessebeach
#4
#2	unhide-comments-2137005-1.patch	3.13 KB	jessebeach
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jessebeach CreditAttribution: jessebeach commented 15 November 2013 at 22:12

Issue tags:

+Spark, +sprint

Comment #2

jessebeach CreditAttribution: jessebeach commented 15 November 2013 at 22:17

File	Size
unhide-comments-2137005-1.patch	3.13 KB

Comment #3

jessebeach CreditAttribution: jessebeach commented 15 November 2013 at 22:17

Status:

Active

» Needs review

Comment #4

jessebeach CreditAttribution: jessebeach commented 15 November 2013 at 22:43

File	Size
unhide-comments-2137005-2.patch	2.9 KB

interdiff.txt	2.42 KB

Neglected to run jshint on the patch in #1. Not lint warnings now.

Comment #5

15 November 2013 at 23:30

Status:

Needs review

» Needs work

The last submitted patch, 4: unhide-comments-2137005-2.patch, failed testing.

Comment #6

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 18 November 2013 at 11:12

Status:

Needs work

» Needs review

4: unhide-comments-2137005-2.patch queued for re-testing.

Comment #7

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 18 November 2013 at 11:30

Tested manually, works flawlessly.

Code review

+++ b/core/modules/edit/js/views/EntityToolbarView.js
@@ -224,13 +224,39 @@ Drupal.edit.EntityToolbarView = Backbone.View.extend({
       info.element.element.css({
         left: Math.floor(suggested.left),
         top: Math.floor(suggested.top)
       });
...
+      // Position the toolbar.
+      info.element.element.css({
+        left: Math.floor(suggested.left),
+        top: Math.floor(suggested.top)
+      });

Shouldn't the first of these two identical statements inside refinePosition() go away?

Comment #8

jessebeach CreditAttribution: jessebeach commented 18 November 2013 at 15:59

File	Size
unhide-comments-2137005-7.patch	2.84 KB

interdiff.txt	749 bytes

Yes, copy-paste should have been cut-paste. Nice catch!

I removed the top bit of repetitive code.

Comment #9

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers commented 18 November 2013 at 17:27

Assigned:	Unassigned	» jessebeach
Status:	Needs review	» Reviewed & tested by the community

Tested manually again, still works flawlessly.

Comment #10

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 24 November 2013 at 03:43

Status:

Reviewed & tested by the community

» Needs review

File	Size
Screen Shot 2013-11-23 at 7.41.57 PM.png	37.14 KB

Hm. I can't quite figure out how to test for the bug in question, so I made a guess that it was making a super long body (like 30 lines of "asdasd" with newlines between them), in-place editing on it, and then scrolling to the bottom of the page.

Before, I felt like it was working, but with this patch I ended up with this, with the comment form showing the entity toolbar for the body field.

Body field entity toolbar showing on comment form

But, I can't figure out if this is:

a) A bug with this patch.
b) A totally unrelated bug.
c) Actually by design, by informing you you still have an in-place editing session happening off-screen.

So setting back to needs review for feedback. :)

Comment #11

jessebeach CreditAttribution: jessebeach commented 2 December 2013 at 22:58

File	Size
comments-are-hidden-2.png	34.01 KB
unhide-comments-2.png	229.34 KB
comments-edit-patch-2.png	69.02 KB

Maybe #2136895: Comment settings are now a field, but not an editable one: breaks in-place editing wasn't committed on your local build? This is what you should see. First, before the patch:

In 8.x, comments are hidden during quick-edit:

If we forcefully unhide the comments section, we see that the "entity" containing div grows to include this content, and the entity toolbar now floats to the bottom of the "entity" when it can't fit at the the top of the screen.

With this patch applied, the entity toolbar will only float as far down or up as the last candidate field. Since comments are not a candidate field, the entity toolbar ignores them.

Can you give it another try? I'm hoping your first experience was just a weird anomaly. I can't reproduce the behavior you documented. It's working as expected AFAICT.