Follow up for #1938062-43: Convert the recent_comments block to a view

Problem/Motivation

Cannot submit a comment on a node.

Proposed resolution

TBD

Remaining tasks

  • (optional) find the commit using git bisect that introduced this error.
  • discuss a proposed resolution
  • ?

Steps to reproduce

  1. add an article
  2. try either the save or preview button to add a comment.

User interface changes

No UI changes.

API changes

No api changes anticipated.

Original report by @jibran

In #1938062-43: Convert the recent_comments block to a view

While adding comment I am unable to press submit or preview button due JS error.
An invalid form control with name='comment_body[und][0][value]' is not focusable.
Dono about any active issue related this.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

FileSize
454.31 KB

cannot_submit.png

YesCT’s picture

Also, when using devel to generate content with comments, I get:

Error message
Drupal\Core\Entity\EntityStorageException: Unable to set a value with a non-numeric delta in a list. in Drupal\Core\Entity\DatabaseStorageControllerNG->save() (line 342 of /Users/ctheys/foo/drupal/core/lib/Drupal/Core/Entity/DatabaseStorageControllerNG.php).
The website has encountered an error. Please try again later.
jibran’s picture

Issue tags: +JavaScript, +CKEditor in core

It is a JS error and I think it has somthing to do with CKEditor so adding tags to get more eyes.

Wim Leers’s picture

Status: Active » Postponed (maintainer needs more info)

I'm pretty sure this (screenshot in #1) is not a JS error, but a HTML5 validation error.

Please try to reproduce it with JS disabled. If you still can reproduce it, then it's not a JS error :)

jibran’s picture

FileSize
38.97 KB

In firefox after disabling java script and after selecting "Restricted HTML" text format I am able to submit the form.

In chrome i am getting this error

1954968-5.png
and after disabling java script and after selecting "Restricted HTML" text format I am able to submit the form.

Wim Leers’s picture

Status: Postponed (maintainer needs more info) » Active
Issue tags: -JavaScript, -CKEditor in core

Indeed HTML 5 validation, as I suspected: http://stackoverflow.com/a/7264966.

This is probably not a JavaScript issue, and definitely not a CKEditor in core issue.

xjm’s picture

Priority: Major » Critical
webchick’s picture

Thought I was going absolutely insane.

webchick’s picture

Title: cannot add a comment. js error causes message: "please fill out this field" » Impossible to submit the comment form on Chrome (possibly other browsers)

Btw, Safari works. Chrome doesn't. Don't have any other browsers on this machine.

webchick’s picture

Title: Impossible to submit the comment form on Chrome (possibly other browsers) » Impossible to submit the comment form on Chrome (possibly other browsers) when CKEditor active
Component: comment.module » ckeditor.module

Hm. Well, something definitely seems CKEditor-related. If you switch to "Filtered HTML" with no CKEditor, the form submits fine. (thanks, andypost)

Damien Tournoud’s picture

Title: Impossible to submit the comment form on Chrome (possibly other browsers) when CKEditor active » Required CKEditor fields always fail HTML5 validation

Both errors mean that the (hidden) <textarea> control is empty at the time of the submit. This probably means that the browser validation happened too early (before CKEditor could update the field), and it is a problem only in the case of the comment form, because it's the only required text area that we have.

From #1, I conclude that this happens on both Firefox and Chrome (with slightly different symptoms), so retitling.

nod_’s picture

Issue tags: +JavaScript

tag

nod_’s picture

Status: Active » Postponed
alexverb’s picture

I also think this is a ckeditor issue. They just need to copy over the values to the actual textarea before any browservalidation can take place. I don't think it's Drupal's job to make all wysiwyg editors HTML5 compatible...

Duplicate issue on D7: http://drupal.org/node/1338956

Wim Leers’s picture

#10, #11, #13 and #14 are all right. Apologies.

It's not a general JavaScript issue: it's CKEditor's responsibility (or rather: WYSIWYG editors in general, when used with forms that use HTML5 form validation). The WYSIWYG module issue that alexverb pointed to (#1338956: [upstream] HTML5 "required" attribute prevents forms from submitting) contains more info.

I wonder if it might be better to do what alexverb suggested in comment 7 of that issue: use the novalidate attribute whenever a Text Editor is attached to a required form item. (Should be possible to do that in editor_pre_render_format().) Server-side validation would still kick in.
I think that'd be worse, but I'm not entirely sure. Thoughts?

andypost’s picture

Issue tags: +JavaScript

wysiwyg should call detach() before html5 validation, so leaving JS tag

Is it possible to make custom code execution before internal validation?

For example, when I use "etherpad" as wysiwyg backend I have to make additional call to server to get actual content and sometimes I need to make some text processing before a content is sent to validator

Damien Tournoud’s picture

I am not quite sure either that this is the responsibility of CKEditor itself. We do call updateElement() manually when serializing the form.

nod_’s picture

Fact is that it's a bug for us and for them. Check the testcase in their bug tracker, it breaks simple (Drupal-less) HTML5 forms.

I marked this one as postponed since a side-effect of fixing CKEditor for all HTML5 forms would fix our bug too. I pinged the CKEditor guys and they told me that someone from their team will have a look at it. It's just a matter of who has time to dig in and fix it.

Wim Leers’s picture

FileSize
849 bytes
2.06 KB

#16: that's a good point, but we're already doing that. In editor.js:

      // Detach any editor when the containing form is submitted.
      $this.parents('form').submit(function (event) {
        // Do not detach if the event was canceled.
        if (event.isDefaultPrevented()) {
          return;
        }
        Drupal.editorDetach(field, settings.editor.formats[activeFormatID], 'serialize');
      });

The problem is that the browser's HTML 5 validation kicks in even before that. See http://stackoverflow.com/q/7587511.

Now, it's possible to manually check the validity just before the submit event itself occurs and if that triggers the (new) invalid event, we can still do the serialization.
See option-a.patch.

But that doesn't solve the entire problem: what if CKEditor was in fact really left empty by the end-user? Then we're back to square one, where the browser either displays the HTML5 validation error message in the wrong location (e.g. in Firefox, see screenshot in #1) or not at all (e.g. in Chrome, see the OP). AFAICT this is an unsolvable problem, because HTML5 doesn't allow us to actually fix this.
See option-b.patch.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Postponed » Needs review
Issue tags: +sprint
nod_’s picture

Issue tags: -sprint

I'd rather wait a bit to see what comes up from the ckeditor issue before we commit something to core.

nod_’s picture

Issue tags: +sprint

tag

alexverb’s picture

I think the "novalidate" attribute would be a valid option if we did not have to give in on conformity. Because doing this only for WYSIWYG fields would mean you can let the user go through 2 different validators (browser vs drupal).

Just think about getting pulled over by a shiny browserpopup that's telling you to fill in the title field. Submitting the form again, doing a page load and getting pulled over by Drupal's famous red warning you missed a WYSIWYG field. This would be just plain confusing...

If this issue doesn't get resolved by the editors (ckeditor, tinymce, ..). I say we should seriously think about disabling the HTML5 validation entirely. I know it would be pretty controversial to do this, but if you want an identical user experience for everyone it should at least be a configurable option (core, theme, contrib?). And in my opinion the WYSIWYG feature in core outweighs the benefits of browser validation.

If we do decide to fix this problem through Drupal or the editors we should also find a solution for the positioning problem of the browser validation popup. I'm shure there must be a CSS way to let the browser know where our textarea (wysiwyg) is at...

Wim Leers’s picture

I forgot to mention: novalidate and noformvalidate (which is already present in Drupal 8 core whenever a submit button has the #limit_validation_errors property, for e.g. "Previous" and "Add another item" buttons) apply to the entire form, not specific form items!

So, this scenario is impossible by using those attributes:

Just think about getting pulled over by a shiny browserpopup that's telling you to fill in the title field. Submitting the form again, doing a page load and getting pulled over by Drupal's famous red warning you missed a WYSIWYG field. This would be just plain confusing...

That scenario is possible though by simply removing the required attribute from the hidden text area.


And AFAICT there is zero hope of being able to do this:

we should also find a solution for the positioning problem of the browser validation popup. I'm shure there must be a CSS way to let the browser know where our textarea (wysiwyg) is at...

(Which means HTML5 validation + WYSIWYG editors is an unsolvable problem until the spec makes it possible to be able to position HTML5 validation error messages.)

At the very least it's puzzling that they (the browser implementors and spec writers) did not take this into account explicitly.

webchick’s picture

Issue tags: +html5

Tagging HTML5 as well, in case those folks have any bright ideas.

aroq’s picture

Issue tags: -html5
aroq’s picture

Issue tags: +html 5

Added HTML 5 tag back.

aroq’s picture

Issue tags: -html 5, -sprint +html5, +a11ySprint

Updated tag.

aroq’s picture

Issue tags: -a11ySprint +sprint

updating tags again.

YesCT’s picture

re #26: could #1961908: Comments can't be saved/submited be a duplicate of this?

nod_’s picture

Wim Leers’s picture

Upstream moving based on our feedback/my patches in #19 :) Cross-project collaboration++

So… question then is: do we want to implement the same in editor.module to have it automatically apply to all WYSIWYG editors? Or do we leave the responsibility with the WYSIWYG editors, whom will have to solve this anyway? And if the latter, do we apply option-b.patch from #19 temporarily until we can pull in a version of CKEditor with the fix, to unbreak our comment form until then; or do we hotfix our CKEditor version?

Damien Tournoud’s picture

Option (b) is fine. Client-side validation brings very little value to begin with in that case (why would you submit the comment form without a comment?), and the requirements are checked on the server-side anyway.

Wim Leers’s picture

Precisely :)

Rerolled option-b.patch to now also restore the "required" attribute when switching to a text format that doesn't use a Text Editor.

jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

Patch still applies cleanly, and I'm able to submit comments again. Thank you!

nod_’s picture

+1 to RTBC

I don't think it should be the responsibility of drupal to take care of it but the specific editor implementation at least. Anyway let's see how it goes with other editors and the required attribute to see what we'll do with this bit of code.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 924be51 and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

.

Status: Fixed » Closed (fixed)

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

Pancho’s picture

Component: ckeditor.module » editor.module
Issue tags: +workaround, +Upstream bugfix

Retroactively tagging and moving this to editor.module because the workaround has been generically introduced there.
Which I actually believe should change after #2036253: Update CKEditor library to 4.2.

Pancho’s picture

Tags didn't stick.

Pancho’s picture

Issue tags: +Clientside Validation

And one more tag.

Garra’s picture

Hello and thanks for this help

but i have always the same problem

I have installed ckeditor module (last version 7.x-1.13) and ckeditor library 4.2 but the problem is still there
in Firefox, html5 required message is visible at the bottom of the window (even if i write something in my textarea field)

I don't understand your patch because i don't have "module/editors" but "module/ckeditor" or "module/wysiwyg" directories (before i tested with wysiwyg module)

so i don't know how this patch works

thanks a lot if you can help me?

Garra’s picture

Version: 8.x-dev » 7.23
Status: Closed (fixed) » Active
FileSize
93.4 KB
87.96 KB

In fact, what i found is :

1) The first time i do not write text i have the drupal core message (red message in the top of the page)
2) i write something and then i have the html5 message in the very bottom on the page (hidden buy the windows 7 bottom toolbar)
(i join images)

does anybody have the same problem?
could anybody can help me?

thanks a lot
Nora from Paris

Garra’s picture

Version: 7.23 » 8.x-dev

sorry
i wrote my comment on a 8.x-dev version post but i am in 7.23.
i am very sorry :-(
it was my firsts comments....

tim.plunkett’s picture

Status: Active » Closed (fixed)

This was a core D8 bug that was fixed. If you're having this problem on Drupal 7, it is probably due to a contributed module.

YesCT’s picture

Issue summary: View changes
Issue tags: -Upstream bugfix +Needs upstream bugfix

standardizing tag. sorry for noise. (https://drupal.org/needs-tags)