Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
After upgrading to 6.2.x for Drupal 10, our offcanvas element edit forms were completely broken. We tracked this down to webform_css_alter not removing core's offcanvas CSS. This was happening because, for whatever reason, \Drupal::service('router.admin_context')->isAdminRoute(); was returning FALSE.
The weird thing is, if I run drush ev "\Drupal::service('library.discovery')->clearCachedDefinitions();" and reload the page it works fine. But as soon as I do a full cache clear after that it breaks again.
This commit removed the explicit Claro check too.
Steps to reproduce
Unfortunately I'm not sure how to reproduce this on a vanilla setup, will keep digging though.
Proposed resolution
Can we use \Drupal\webform\WebformRequest::isWebformAdminRoute instead?
Should we also move the _admin_route route option from the route subscriber into the routing.yml definitions?
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | 3347104-6.1.x-20.patch | 1.79 KB | jrockowitz |
| #19 | 3347104-6.1.x-19.patch | 348 bytes | jrockowitz |
| #13 | 3347104-13.patch | 1.7 KB | jrockowitz |
| #3 | 3347104-3.patch | 1.05 KB | fenstrat |
Comments
Comment #2
acbramley commentedAlso worth noting that https://www.drupal.org/node/2888767 states:
So we may need to change this anyway if routes aren't reliable in this context.
Comment #3
fenstratHere's the
WebformRequest::isWebformAdminRoute()approach. It fixes the issue in our case. But it's not convincing as the correct fix™ becauseisWebformAdminRoute()is still accessing the admin context.Comment #4
fenstratSo in our case the admin context check in
isWebformAdminRoute()is picking up the admin route (which oddly goes against what #2 is suggesting - that was our issue in the first place,\Drupal::service('router.admin_context')->isAdminRoute() === FALSE). Though @acbramley noted he did see 2 requests when debugging through getCurrentRequest().So then because it is being seen as an admin route that means it then falls through to the
(preg_match('/^(webform\.|^entity\.([^.]+\.)?webform)/', $route_name)) ? TRUE : FALSE;check which picks correctly picks up the admin route.As that last check is done on route_name, and that seems to be more reliable in hook_css_alter, then this seems fine.
Also, reusing
\Drupal::service('webform.request')->isWebformAdminRoute()and deferring to existing code seems like a sane approach here anyway.Comment #5
acbramley commented#3 works (for us) because by the time webform_css_alter is called,
$this->isAdminRouteis already set on the WebformRequest service and returns early.Debugging into
isWebformAdminRoute, the first time it's called (fromwebform_theme_suggestions_form_elementthat calls WebformElementHelper::isWebformElement.For whatever reason, this initial code path does return TRUE for
$this->adminContext->isAdminRoute()Comment #6
jrockowitz commentedThe problem and solution make sense.
What are the steps required to replicate this issue?
Comment #8
jrockowitz commentedUsing WebformRequest::isWebformAdminRoute will remove the offcanvas
Comment #9
catchThis came up in slack https://drupal.slack.com/archives/C78MFLN9K/p1685066450956139 persumably without realising this issue was open.
Could webform look for its own CSS amongst the CSS in hook_css_alter(), and if it's there, remove the offcanvas resets?
Comment #10
lauriiiI don't think this actually works with 10.2.x because
\Drupal\webform\WebformRequest::isWebformAdminRoutealso depends on the current request. You'll probably have to implement something like what's proposed in #9. More info can be found in this CR https://www.drupal.org/node/2888767.Comment #12
jrockowitz commentedI am going to roll back the commit since I think a better approach might be detecting if webform-specific CSS is present.
Comment #13
jrockowitz commentedLooks like #9 is much better and safer approache.
Comment #14
catch#13 looks as simple as I'd hoped it would be!
Comment #15
jrockowitz commentedComment #16
luke.leberI've manually tested against 9.5.x-dev and 10.1.x-dev.
9.5.x-dev works great -- even without the patch (as expected).
10.1.x-dev works great -- even with CSS aggregation enabled. #13 seems to fix the "files can be aggregated in a separate process" problem.
+1 RTBC from me!
Comment #17
catchMoving #13 to RTBC per the manual testing in #16.
Comment #18
jrockowitz commentedI am going to back port this to 6.1.x because I am continually merge 6.1.x into 6.2.x
Comment #19
jrockowitz commentedComment #20
jrockowitz commentedComment #21
luke.leberPedantically, I gave #20 a test run against 9.5.x and everything seemed okay -- as in it worked great before, and worked equally as great after!
Comment #25
jrockowitz commented