Updated: Comment #0

Problem/Motivation

Title says it all. This screencast says the rest: http://screencast.com/t/biBnsJxhMM. Thanks to Bojhan for that video.

(Note that that message only shows up when the dialog takes a long time to load; it's there to indicate to the end user that something is still happening.)

Proposed resolution

Place the "Loading" message better.

Remaining tasks

TBD.

User interface changes

TBD.

API changes

None.

#1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Issue summary: View changes
Issue tags: +Novice, +JavaScript
Wim Leers’s picture

Issue tags: +js-novice
azinck’s picture

Assigned: Unassigned » azinck
Wim Leers’s picture

azinck: Thanks! :) Ping me on Twitter if you don't get a review within 24 hours.

azinck’s picture

Sounds good. Unfortunately this is going to require a change in how the loading indicator is handled for quick edits. I'll probably need to visually fade it in rather than sliding it down.

Wim Leers’s picture

Or, alternatively, slide it up from underneath the field being in-place edited? Or from the side? Or from the entity toolbar, which contains the CKEditor UI? Many options here :)

Wim Leers’s picture

Assigned: azinck » Unassigned

It looks like azinck didn't get around to do this. Any other takers? :)

azinck’s picture

Yeah, someone else should take a look. I poked around with it for a bit and don't know enough about all the situations in which this behavior might be invoked to see a clear path forward that will take care of all use-cases.

chrlvclaudiu’s picture

Assigned: Unassigned » chrlvclaudiu

Let me give it a try :)

chrlvclaudiu’s picture

is this issue fixed !? i get a different behavior than the one on the video :-/

azinck’s picture

I haven't tested it lately so I can't confirm if the prob still exists, but you're testing with in-place editing, right? This isn't a problem on the node edit page.

chrlvclaudiu’s picture

yes, i'm testing it with the in-place editing. I'll come back with a video to show you how it's on my side. I cloned the 8.0.x branch, is it the right branch ? in the right side of this page I see version: 8.0.x-dev

Wim Leers’s picture

Yes, 8.0.x is the right version.

yvesvanlaer’s picture

Status: Active » Reviewed & tested by the community
Issue tags: +Amsterdam2014
FileSize
1.61 KB
49.64 KB

Hey all

Please find the patch attached.
I just love fixing stuff by deleting code.

Problem: the loading div started with a "position: absolute" and a "top: -40px".
I noticed that there also was an animate function which pulled the div back into the textfield (going from top: -40px to 0).
By removing the animate function and the top: -40px where the element started out with, it now just shows in the field, with no animation though.

Result:

yvesvanlaer’s picture

Status: Reviewed & tested by the community » Needs review

Wrong status, my bad.

Wim Leers’s picture

Issue tags: +Needs usability review

This one will need a usability review from Bojhan or yoroy, and preferably also from Kevin O'Leary.

Wim Leers’s picture

My personal concern is that the "Loading…" indicator now appears instantaneously, which is rather annoying. If the usability folks are fine with it though, then I'm fine with it also.

Otherwise we should keep the current behavior for CKEditor instances that run in iframe mode and apply a different behavior for instances running in in-place editing mode.

Bojhan’s picture

Issue tags: -Needs usability review

We should always wait a bit before initiating a loading bar. It should ideally rarely be seen, only on very heavy widgets - where you need this feedback to make sure you are doing it correctly.

Wim Leers’s picture

Agreed. That's exactly what I thought you were going to say :)

tkoleary’s picture

It looks a little better but I would have preferred a solution in which the "loading" message appears inside the toolbar in the space normally occupied by the save button. To me that makes more logical sense. It would be more prominent and discoverable as well as not interfering with the contents of the field or the cursor.

If placed inside the toolbar the "loading" message should have the same throbber as "saving" does, which is a current design inconsistency.

Additionally, I think that the patch in #14 introduces a regression in which, in some instances, the toolbar obscures the field being edited (I believe that may have been the reason for the 40px: top attribute).

There is also another strange behavior that may be a regression where on saving title the toolbar "flies away" up and to the left.

Bojhan’s picture

Could you perhaps explain a little bit why its a design inconsistency, I never understood - why we do this in edit, but nowhere else in core (replacing buttons).

Also the regressions sadly have existed for ages :( I hope OCTO gets around to making this kind of cleanup a priority.

Damien Tournoud’s picture

Status: Needs review » Needs work
// Add a "Loading…" message, hide it underneath the CKEditor toolbar, create
// a Drupal.ajax instance to load the dialog and trigger it.

The obvious bug here is that there used to be a CKEditor toolbar that the "Loading..." message was hidden beneath. That's what the -70px were for. Let's fix the CSS to hide the message differently, and also update the comment.

Wim Leers’s picture

It looks a little better but I would have preferred a solution in which the "loading" message appears inside the toolbar in the space normally occupied by the save button. To me that makes more logical sense. It would be more prominent and discoverable as well as not interfering with the contents of the field or the cursor.

This only works when using CKEditor during in-place editing. i.e. it would cause a very different UX when using CKEditor on the back-end (in iframe mode) versus on the front-end (in in-place editing mode).

tkoleary’s picture

@wim-leers, @Damien-Tournoud

Given the fact that we were intentionally hiding it before does it need to be in the toolbar at all?

Put another way, how much of an edge case is it that a field would take so long to load that a "loading" message is necessary?

Wim Leers’s picture

Put another way, how much of an edge case is it that a field would take so long to load that a "loading" message is necessary?

Whenever latency is involved, assume it takes seconds. Design the UX so that it is still usable at all in that scenario.

On our local development machines, it indeed will only take seconds. Over 3G and with an overloaded server, it can easily take seconds.

Note that we were NOT intentionally hiding it. We were inserting it as hidden, and showing it after 1 second. That's a big difference.

Wim Leers’s picture

Priority: Normal » Critical
Related issues: +#2349547: Quick edit inserting 'Loading' message html into content

… and apparently this even makes it possible for the "Loading…" message to become part of the actual HTML — see #2349547-3: Quick edit inserting 'Loading' message html into content. That's both hilarious and worrisome, hence bumping to critical.

tkoleary’s picture

@wim-leers

This only works when using CKEditor during in-place editing. i.e. it would cause a very different UX when using CKEditor on the back-end (in iframe mode) versus on the front-end (in in-place editing mode).

Yes. And for that reason I suggest it only be used in the quick-edit context.

On the back end the common behavior IIRC is to display the throbber in-line with the field label

Wim Leers’s picture

On the back end the common behavior IIRC is to display the throbber in-line with the field label

I don't see how that makes sense for an action that's not related to the field, but to something inside the field widget.

Also: the field label can be hidden.

Also: this works absolutely fine on the back-end. It was designed for the back-end. The problem here is that because of that, it doesn't work well in the front-end.

Wim Leers’s picture

Issue tags: +DrupalCamp Ghent 2014, +sprint
Wim Leers’s picture

Issue tags: -sprint
Wim Leers’s picture

Issue tags: -DrupalCamp Ghent 2014
effulgentsia’s picture

Title: Poorly placed "Loading" message when using CKE in-place editing and a CKE plugin with Drupal dialog takes a long time to load » Quick edit inserting 'Loading' message html into content
Issue tags: +Needs issue summary update

Since this was raised to critical in #26, retitling to focus on what's critical. The issue summary could also use an update based on that.

catch’s picture

I'm struggling with the critical-ness of this - can't you just delete that bit of string from the textarea?

Wim Leers’s picture

#33: from #26:

… and apparently this even makes it possible for the "Loading…" message to become part of the actual HTML — see #2349547-3: Quick edit inserting 'Loading' message html into content. That's both hilarious and worrisome, hence bumping to critical.

Data corruption ⇒ critical?

catch’s picture

Is it data corruption or text area corruption though?

Wim Leers’s picture

Data corruption.

Gábor Hojtsy’s picture

I went to test how this works. Its strange. So the image plugin modal would never show up for me, I only see the loading thing... That is due to the following JS error:

Uncaught AjaxError: 
Path: /editor/dialog/image/basic_html
Fatal error:  Cannot access protected property Drupal\filter\Entity\FilterFormat::$format in /home/sa4d6841a73f658f/www/core/modules/editor/src/Form/EditorImageDialog.php on line 51

So the dialog never appears. That actually makes reproducing this bug trivial because the Loading... message never disappears.

So then you can click X out of the editing flow, when it asks if you want to save or discard. At that point the insertion of the Loading thing is not yet evident / displayed. So if you changed also something else, you would save of course.

Which then reloads the node and TADA you have the Loading... text at the end of your node text.

Which you can then of course remove with either another quickedit or a backend edit (which makes me think this may not be a critical)?

I mean that you cannot insert an image anymore sounds critical but that is a different problem.

Not sure it is worthwhile to update the summary to current state since you cannot even insert an image ATM.

Wim Leers’s picture

[…] the following JS error: […]

Indeed; see #2389381: Impossible to add images in WYSIWYG including in-place editing due to fatal error. Nice coincidence :)

Which you can then of course remove with either another quickedit or a backend edit (which makes me think this may not be a critical)?

If the system can corrupt your content… that seems critically bad for a CMS?

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Ghent DA sprint
FileSize
1.94 KB

So this should at least bring back the image :)

swentel’s picture

Ah crap nvm the patch

Gábor Hojtsy’s picture

Wim Leers: re #38 I think the distinction that @catch made was if the data in the textarea is modified but not automatically saved, then the user still has a chance to fix it vs. it is already wrong on the live version (cannot fix it without people possibly seeing it). In that sense this is kind of not a critical, but as I demonstrated you may not yet see that it happened (also the bottom of the text may be totally out of view, not even on the screen), so in that sense, its easy to run into this without a chance to know to need to fix it in the first place.

Status: Needs review » Needs work

The last submitted patch, 39: 2091855-38-Poorly-placed-Loading-message.patch, failed testing.

xjm’s picture

catch’s picture

Wim Leers: re #38 I think the distinction that @catch made was if the data in the textarea is modified but not automatically saved, then the user still has a chance to fix it vs. it is already wrong on the live version (cannot fix it without people possibly seeing it).

Yes this is what I was getting at.

Also that it's extra data as opposed to less data - i.e. if it replaced the textarea with that message that would be (much) more of an issue than appending since suddenly your work is lost.

For me that feels like a major bug - there's a workaround (disable inline edit, or know to check for this when editing content), and it's recoverable (edit the content to remove the extra line). Doesn't mean it's not a bad issue and extremely annoying, but given we have major issues that in some instances can lead to sites going down I have trouble seeing how it's critical.

Gábor Hojtsy’s picture

Priority: Critical » Major
Gábor Hojtsy’s picture

Wim Leers’s picture

Ok, that's fair :)

subhojit777’s picture

Assigned: chrlvclaudiu » subhojit777

By quick edit - I want to attach an image, but then I decide not to change anything and click "Save" button, the "Loading..." message becomes part of the content. This is issue may not be major or critical but its really very annoying.

Also we are changing the placement of "Loading..." message right?

I am assigning this to myself, lets see what I can do :)

subhojit777’s picture

This bug appears while you quick edit in node page. Suppose the node appears in front page, and you quick edit there - the bug does not appears.

disasm’s picture

Lets get these screenshots added to the issue summary.

tkoleary’s picture

Not sure where it happened, but the position of the loading message has changed placing it at the x,y of where the field editor itself will render.

I am personally fine with this. I attempted to force a slow load by packing many megabytes of images in the body field and it was still not particularly obtrusive.

I recommend closing this as fixed.

Wim Leers’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
2.41 KB

This is still a bug. Let's fix it by not adding the "Loading" indicator in case of an inline CKEditor instance. Then #2563081: Move the "loading" indicator into the CKEditor toolbar can offer a more comprehensive solution in 8.1.

Wim Leers’s picture

Title: Quick edit inserting 'Loading' message html into content » CKEditor inserting 'Loading' indicator's HTML into HTML while using Quick Edit
ULikeApples’s picture

Issue tags: +Barcelona2015

@casism @ULikeApples @veronicanerak and @Kgoel as mentor. We are working at DrupalCon Barcelona.

casismary’s picture

Status: Needs review » Closed (cannot reproduce)
FileSize
30.75 KB

We tested manually:
- We went to drupal 8 testing site locally.
- We added a 'basic page' content.
- The issue is indicating 'loading indicator' when doing quick edit on the content,
we clicked on the content created,
we clicked the edit icon and clicked on 'quick edit' on the dropdown,
we edited the content created and 'loading indicator' is not showing anymore.
- We have added a screenshot after creating and quick-editing the content.
- We are closing the issue.