The selector used to force focus on the close button (per ARIA guidelines) is not scoped to the modal content. The result is that when you have another element with the "close" class on the page, it also receives focus. This causes the modal offset to be incorrect since the other elements with the "close" class are scrolled into view.

CommentFileSizeAuthor
#11 interdiff.txt659 bytesjoelpittet
#11 2607626-11.patch2.39 KBjoelpittet
#9 modal_close_button-2607626-9.patch2.36 KBjoelpittet
#5 ctools-close-modal-2607626-5.patch1.7 KBdsnopek
#2 modal_close_button-2607626-2.patch789 bytesmdeltito
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mdeltito created an issue. See original summary.

mdeltito’s picture

mdeltito CreditAttribution: mdeltito at Phase2 commented
Status: Active » Needs review
FileSize
modal_close_button-2607626-2.patch789 bytes
nicobot’s picture

nicobot CreditAttribution: nicobot as a volunteer commented
Status: Needs review » Reviewed & tested by the community

It works for me.

capynet’s picture

capynet CreditAttribution: capynet as a volunteer commented

It works for me too.
tnx!

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Reviewed & tested by the community » Needs review
FileSize
ctools-close-modal-2607626-5.patch1.7 KB

Here's a version of this that also scopes to the .modal-header selector (since there can be elements with the "close" class in the modal body, not just the close button in the header) and makes sure to only unbind() on the correct elements (although, this is just a performance improvement, it should be harmless to unbind an event that was never bound).

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Issue tags: +panopoly

We're using this patch in Panopoly.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@dsnopek your patch in #2 looked better to me. Checking the .modal-header is good though. modalContent = $('#modalContent'), though using the reference can help not parse the DOM as much.

How about:

var $modalHeader = modalContent.find('.modal-header');
$('.close', $modalHeader)

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

I'm not opposed to that! :-)

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
FileSize
modal_close_button-2607626-9.patch2.36 KB

How's this?

SocialNicheGuru’s picture

SocialNicheGuru CreditAttribution: SocialNicheGuru commented
Status: Needs review » Needs work

I could not close modals in panels

modal.js?ocrnxd:680 Uncaught ReferenceError: $modalContent is not definedDrupal.CTools.Modal.unmodalContent @ modal.js?ocrnxd:680

Drupal.CTools.Modal.dismiss @ oa_basetheme.modal.js?ocrnxd:117Drupal.CTools.Modal.modal_dismiss @ modal.js?ocrnxd:333Drupal.ajax.success @ ajax.js?v=7.50:428success @ ajax.js?v=7.50:189c.success @ jquery.form.min.js?v=2.69:11o @ jquery.min.js?v=1.7.2:2fireWith @ jquery.min.js?v=1.7.2:2w @ jquery.min.js?v=1.7.2:4d @ jquery.min.js?v=1.7.2:4

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs work » Needs review
FileSize
2607626-11.patch2.39 KB
interdiff.txt659 bytes

Thanks for testing @SocialNicheGuru, I missed something it looks like. Here's the missing variable.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Priority: Minor » Normal
rivimey’s picture

rivimey
she/her
CreditAttribution: rivimey as a volunteer commented

Patch in #11 applies cleanly to 7.x-1.x and looks good to me.

More +1 responses to #11 would be helpful (@dsnopek?), but I think this one is good to go.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Feel free to change the status to "reviewed and tested by the community" if it's working for you, that would help it get in

rivimey’s picture

rivimey
she/her
CreditAttribution: rivimey as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Parent issue: » #2819121: Plan for CTools 7.x-1.12 release

Proposing RTBC - slightly reluctantly because there are no 'corroborating' comments, but as far as I can see this should be a good change.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Thanks, I'm sure if the maintainers have any concern they will knock it back to needs work, so there is at least 3 people who have reviewed and tested it.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Parent issue: #2819121: Plan for CTools 7.x-1.12 release » #2828925: Plan for CTools 7.x-1.13 release
japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry commented
Status: Reviewed & tested by the community » Fixed

The best way to test code is to deploy code. ;)

Since this is a relatively small patch, Committed.

  • japerry committed 25deecc on 7.x-1.x authored by joelpittet 
    Issue #2607626 by joelpittet, mdeltito, dsnopek: Modal: Close button...

Status: Fixed » Closed (fixed)

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