Problem/Motivation

Editor AJAX routes don't determine the proper admin theme when making a request. This results in the request loading the default theme's libraries (and alters to those libraries). This can cause issues when the default theme's assets aren't actually loaded.

Proposed resolution

Add the necessary ajax_base_page theme negotiator option to editor.routing.yml.

Remaining tasks

  • Create patch
  • Create tests? - Unclear if this is needed

User interface changes

None

API changes

None

Data model changes

None

Original Post

This happens since https://www.drupal.org/node/2637888

Steps to reproduce:
- Install Drupal with profile standard
- Set bootstrap theme as installed and default
- add an article node
- Try to use the link button for the body in the wysiwyg
The modal doesn't show up and you get an error on the js console:
Drupal.theme.bootstrapModal is not a function

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rodrigoaguilera created an issue. See original summary.

markhalliwell’s picture

Title: Don't use bootstrap modal for admin theme » Editor routes don't use the ajax_base_page theme negotiator
Project: Bootstrap » Drupal core
Version: 8.x-3.x-dev » 8.0.x-dev
Component: Code » editor.module
Priority: Normal » Major

This is a core bug, will attach a patch shortly.

markhalliwell’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.28 KB
rodrigoaguilera’s picture

Status: Needs review » Needs work

I did a manual test and I get the seven theme modal instead of the bootstrap error.

I have no clue on how this can be tested without something like behat.

markhalliwell’s picture

Status: Needs work » Needs review

It may not need tests. Not sure why you set to CNW, especially since you have confirmed the patch works, but setting it back to CNR so others can take a look.

markhalliwell’s picture

The Bootstrap IQ has already received 3 issues on this (including this one that was moved, see referenced by issues). While this isn't necessarily "critical" (e.g. no data loss), it's extremely annoying and the above patch is not an obvious fix to the symptom it causes.

Does this really need tests? What needs to happen for this issue to move forward?

markhalliwell’s picture

Issue tags: +JavaScript

Another issue.

markhalliwell’s picture

Issue summary: View changes
jonodunnett’s picture

Patch in #3 fixes the issue for me :) thank you.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

In order to test this properly as a functional test we would require javascript testing, which doesn't exist at this point in time.

Wim Leers’s picture

LGTM

catch’s picture

Status: Reviewed & tested by the community » Fixed

This could probably be tested by faking the AJAX request then confirming that theme negotiation happens, however it's not going to be a meaningful test of the editor AJAX functionality, and we should already have testing that the route option works, so not worth it in either case.

Would be a good one to add a BrowserTestBase test for once that's available though.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 92e7090 on 8.1.x
    Issue #2640086 by markcarver: Editor routes don't use the ajax_base_page...

  • catch committed 5cf7ffc on 8.0.x
    Issue #2640086 by markcarver: Editor routes don't use the ajax_base_page...

Status: Fixed » Closed (fixed)

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