When I enable this module the checkbox appears below the Save/Preview (S/P) buttons consistently on Safari 5.1.3 and IE 8.0.6001.18702. On Firefox 10.0 it renders properly. I have cleared every cache (Admin_menu module/flush all caches, Configuration/Development/Performance and even the CAPTCHA Placement cache) on the server side and caches on all browsers. I disabled captcha on the form with the problem, flushed the cache again and the Notify checkbox then appears above the S/P buttons. Turn captcha back on for the form and Notify checkbox and captcha appear below S/P buttons again. Disable comment_notify and captcha appears above S/P again.

Files: 
CommentFileSizeAuthor
#14 1434718_dont_mess_with_buttons.patch657 bytesgreggles
PASSED: [[SimpleTest]]: [MySQL] 46 pass(es).
[ View ]
#13 captcha_is_positioned_correctly.png21 KBAri Linn
#5 comment_notify-appears_below_submit-1434718-5.patch1.13 KBerikwebb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment_notify-appears_below_submit-1434718-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 comment_notify-appears_below_submit-1434718-3.patch1.31 KBerikwebb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment_notify-appears_below_submit-1434718-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
Screen Shot 2012-02-09 at 3.21.55 PM.png61.21 KBcrossfish

Comments

crossfish’s picture

Firefox DOES NOT render properly as I was logged in (captcha not required) when I checked the page, so there are no weird browser issues just output weights I guess.

endrus’s picture

I can confirm the bug. Is there a fix?

erikwebb’s picture

Version:7.x-1.0» 7.x-1.x-dev
Status:Active» Needs review
StatusFileSize
new1.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment_notify-appears_below_submit-1434718-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This appears to be a combination of issues. The Captcha issues appear to be somewhat unrelated. I've attached a patch that moves the comment notify checkbox above the actions section.

crossfish’s picture

I've gone through the steps to try and apply the patch, but it fails trying to work with a path of "vendor/drupal/sites...". I really need to spend some time working on documentation for using git at drupal.org (http://drupal.org/node/707484) because for a newbie it is missing many workflow steps and best practices for someone who just needs to simply apply a patch and not be a developer or doing commits. I did examine your code changes in the patch, but my site fails with an error as the code tries to reference ['actions']['#weight'] in this line which does not exist:
+ '#weight' => $form['actions']['#weight'] - 1,

Because this problem is related to weights, I TEMPORARILY fixed it by changing the '#weight' => 19 to a higher number and that pushed the save/preview buttons back to the bottom. I'm sure that using a hard coded weight in this context is not an acceptable solution to controlling the output flow of this form, but I'm not at a level yet to offer the right solution. Thanks so much for pointing me in the direction of the problem and getting me a work-around for the time being.

erikwebb’s picture

StatusFileSize
new1.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment_notify-appears_below_submit-1434718-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Yes, my patch was just created incorrectly. Here's a better patch to try.

endrus’s picture

Did the patch work and was it added to the release?

greggles’s picture

@endrus - if the patch is applied then the issue will be changed to a status of "Fixed". See http://drupal.org/patch/apply and http://drupal.org/patch/review for how to apply and review a patch. Even "it works for me" would help :)

There's an alternate proposal in #1532806: Allow changing weight of notify checkbox that seems like it might be a more flexible idea.

endrus’s picture

I tried the patch in #5. Alas, it didn't work for me :(

greggles’s picture

I'm not sure if there's a way to do this and #1532806: Allow changing weight of notify checkbox at the same time, but if so that would be great.

greggles’s picture

Status:Needs review» Closed (won't fix)

So, I think I'm not going to commit this.

* I don't personally like this positioning. I think it should be right before the buttons.
* The actions['#weight'] is not defined by default, as mentioned in comment #4, so this will throw php notices on a default install.
* The solution in #1532806: Allow changing weight of notify checkbox is much more flexible. I plan to use that with a weight of "1" as the default to send it low on the page.

greggles’s picture

Status:Closed (won't fix)» Postponed (maintainer needs more info)

Ok, so it seems that setting a lower weight isn't necessarily a fix. I'm re-opening this about the specific CAPTCHA issue and hope we can continue discussion of that. Ari Linn had some comments on it over at #1532806: Allow changing weight of notify checkbox.

I wonder if the Comment Notify CSS is to blame... Can someone with this issue test that?

Ari Linn’s picture

I did some testing on the css matter (just commented out all the comment_notify css) and it didn't help much. I also reported this to Captcha guys and they came up with an idea that comment_notify may change submit buttons weight incorrectly and then Captcha reads an invalid setting.

Ari Linn’s picture

StatusFileSize
new21 KB

UPDATE: I commented out the $form['actions']['#weight'] = 19; code string in comment_notify.module (function comment_notify_form_comment_form_alter) as the Captcha guys suggested and it did the trick - Captcha started displaying itself correctly (screenshot below). I don't know much of the module code structure though and most probably it's not a valid fix but at least those Captcha guys have identified the problem correctly for us.

greggles’s picture

Status:Postponed (maintainer needs more info)» Needs review
StatusFileSize
new657 bytes
PASSED: [[SimpleTest]]: [MySQL] 46 pass(es).
[ View ]

Yeah, makes sense.

Ari Linn’s picture

'kay, seems it's working. :-)

greggles’s picture

This was added for[#1434718] and I can't reproduce the original error right now, so maybe that has been fixed somewhere else?

I'll ping on that issue.

greggles’s picture

greggles’s picture

Status:Needs review» Fixed

Status:Fixed» Closed (fixed)

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

nedjo’s picture

Title:Notify checkbox pushes CAPTCHA below Save/Preview buttons in Safari and IE 8, Bartik theme» Notify checkbox pushes CAPTCHA below Save/Preview buttons

Thx for the fix. Wasn't specific to browsers or theme.

nedjo’s picture

Issue summary:View changes

UPDATE: I incorrectly stated that Firefox 10 worked, I was logged in and captcha was not required so the page rendered the Notify checkbox in the proper location. Come back from that bunny trail... :)