luke.leber → created an issue.
Update I.S. with more info.
luke.leber → created an issue.
luke.leber → created an issue.
smustgrave → credited luke.leber → .
Setting back to Needs Work. Here's a slack chat to document where things are.
balintbrews
23 minutes ago
@lleber
I might be doing something wrong, but I can’t reproduce the issue on 0.x anymore.lleber
:male-scientist: 23 minutes ago
I'll double check, but I was just able to around 8:30 :sweat_smile:lleber
:male-scientist: 16 minutes ago
Ah, so it may be a dependency based thing :open_mouth:.
I added an example-flexbox-break component and here's the CSS inheritance...(XB styles override example-flexbox-break component styles here)
9:38
Then I added a header (custom) component and here's the inheritance!(header component styles override XB styles here)
9:39
The [data-xb-slot-id] selector seems to have nondeterministic loading order, meaning the example in the I.S. seems to work, but another component does not!
9:40
I'll dig a little deeper to see if I can sleuth a better example.balintbrews
13 minutes ago
I can see that you have CSS aggregation turned on. Maybe that comes into play as well?lleber
:male-scientist: 13 minutes ago
I can try disabling it, but the only reason the example component in the I.S. seems to work is because its display: flex is being overridden.balintbrews
13 minutes ago
Yes, I saw that happening on my end as well.lleber
:male-scientist: 12 minutes ago
I do think that if the SDC had a dependency injected on the preview CSS, it'd straighten itself out, but that seems odd.balintbrews
12 minutes ago
Yeah, we don’t want that.
9:44
@lleber
I’m leaning towards just committing the width: 100% you suggested, I can’t see how that would hurt us. And it may help you now. :slightly_smiling_face: If we see more of these styling issues, we can do a deeper investigation. What do you think?lleber
:male-scientist: 8 minutes ago
I think it'd definitely help in the short-term, but it may be a harbinger of deeper issues. I think it has to do with the SMACSS layer the libraries are loaded at.
9:46
The component displaying problems is registered at the component layer. Trying to figure out which layer SDCs are registered at.
9:46
I wonder if the XB styles should load at the lowest layer so that everything can override them? Or will that cause issues (see: layout builder off-canvas :smile:)? (edited)
Newbalintbrews
2 minutes ago
Our styles in preview.css are unlayered, so they will take precedence over other layers.lleber
:male-scientist: 1 minute ago
Hm..then I wonder how our custom header is winning there. I'm out of time this morning to debug, but I'm unblocked to find a root cause now.
I think a deeper investigation is needed to see what exactly is going on.
I.S. update + NR
This is certainly still an issue, except now the .xb--sortable-slot-empty-placeholder
class doesn't specify a minimum width, and will collapse down to 0px wide if the parent element has display: flex
.
I think that the safest bet here is to give the .xb--sortable-slot-empty-placeholder
a width of 100%. The rationality here being that there should only ever be one empty placeholder per slot, right? As soon as a component is added, the placeholder goes away. I can't think of any reason why we wouldn't want the placeholder's drop-zone to be smaller than it could be!
luke.leber → created an issue.
https://www.drupal.org/project/experience_builder/issues/3491021 ✨ Leverage HTML comment annotations, remove wrapping div elements Active has landed and this issue needs re-triaged. Self-assigning and will get around to this tomorrow between 8:30 and 5:00 ET.
Whenever I see a call to wait on a lock I get a chill up my spine.
From a cursory glance, this change could result on a ton of processes spinning/waiting to acquire a lock under a heavy load, couldn't it?
I think this one should be subject to some load testing on a larger site.
A first step is probably to get all of the changes needed in place that Gitlab needs to get all of the jobs green. That's a standardized set of tooling that everyone can share. I think https://www.drupal.org/project/juicebox/issues/3351683 📌 [PP-1] Resolve all coding standard violations Postponed is the place for that work I think. Any commit made to the issue fork over there will automatically run those tests.
Koustav Mondal self-assigned that issue to them self, but hasn't made any commits in a week, so IMO it's fair game for anyone to commit to.
Things like phpstorm configuration will probably vary from person to person, but the gitlab tests are a shared configuration between everyone. They should probably be resolved first before making any other changes so that everyone's on the same page with the standard checks.
After the gitlab checks are all passing, so long as further changes don't make the checks fail, they can probably be applied in a follow-up.
Here's something for us to run temporarily in the new year...
diff --git a/core/assets/scaffold/files/default.settings.php b/core/assets/scaffold/files/default.settings.php
index cd364bb00d..751a5de5ff 100644
--- a/core/assets/scaffold/files/default.settings.php
+++ b/core/assets/scaffold/files/default.settings.php
@@ -811,6 +811,14 @@
*/
$settings['migrate_node_migrate_type_classic'] = FALSE;
+/**
+ * Aggregated asset error logging.
+ *
+ * This is used to inform site owners when CSS or JS assets fail to be included
+ * in an aggregated asset.
+ */
+$settings['log_aggregated_asset_errors'] = FALSE;
+
/**
* The default settings for migration sources.
*
diff --git a/core/lib/Drupal/Core/Asset/CssOptimizer.php b/core/lib/Drupal/Core/Asset/CssOptimizer.php
index 8f2d910250..61d00ea129 100644
--- a/core/lib/Drupal/Core/Asset/CssOptimizer.php
+++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
@@ -5,6 +5,7 @@
use Drupal\Component\Utility\Unicode;
use Drupal\Core\StreamWrapper\StreamWrapperManager;
use Drupal\Core\File\FileUrlGeneratorInterface;
+use Drupal\Core\Site\Settings;
/**
* Optimizes a CSS asset.
@@ -145,7 +146,11 @@ public function loadFile($file, $optimize = NULL, $reset_base_path = TRUE) {
// stylesheets in their .info.yml file that don't exist in the theme's path,
// but are merely there to disable certain module CSS files.
$content = '';
- if ($contents = @file_get_contents($file)) {
+
+ // Temporary hack for debugging impossible to diagnose aggregation issues...
+ $contents = Settings::get('log_aggregated_asset_errors') ? file_get_contents($file) : @file_get_contents($file);
+
+ if ($contents !== FALSE) {
// If a BOM is found, convert the file to UTF-8, then use substr() to
// remove the BOM from the result.
if ($encoding = (Unicode::encodingFromBOM($contents))) {
Unrelated - there should probably be a strict comparison against the file_get_contents
result here -- otherwise some false positives can slip through depending on the file contents 😱!
Ideally when we know that a corrupted asset is going to be generated, we'd want to issue an HTTP/500 to prevent the asset from becoming cached by downstream infrastructure, but that definitely can't happen without breaking huge swathes of sites.
Given the uncertainty about what's supported or not in #2, I think maybe we could add this behind an opt-in behavior to allow for a graceful path to deprecation?
I'm sure that a lot of sites out there have missing assets that's considered very, very normal.
I think this one might be dependent on how https://www.drupal.org/project/experience_builder/issues/3491021 ✨ Leverage HTML comment annotations, remove wrapping div elements Active pans out.
Hi all, given the relatively short remainder of life for Drupal 7, I'm going to close this out, but still assign issue credit where appropriate (hence the 'fixed' status) as it looks like genuine effort was put into this.
Cheers all.
luke.leber → changed the visibility of the branch 3351683-pp-1-resolve-all to hidden.
Hey, I'm closing this one as outdated, but if this is still an issue, please re-open it.
Thanks
I'm un-postponing this issue.
Given we're running on Gitlab now, let's get all of the jobs green. I'll open a fresh MR to start.
Hi Jane, that looks right for working with the Entity API in Drupal 8+. Thanks for sharing!
Hi, can you please provide the view configuration that you were seeing this on?
If we're not able to reproduce the issue with a carefully controlled set of steps to reproduce and write a new test for it, it's a lot harder to justify making any changes. I'm triaging this as PMNMI until we can hear more about the particulars of the site experiencing issues.
Thanks!
Good day, 4.0.0-alpha2 just landed. If you run into any issues don't hesitate to spin up an issue.
Thanks
Stand by for 4.0.0-alpha2 as soon as the main pipeline goes green.
I've manually tested the common use cases for this module against 11.1 and 10.4 including...
- Media reference field configured to display juicebox formatters
- Image field configured to display juicebox formatters
- View configured to display content as juicebox galleries
Given we have green tests and a solid manual test, I'm going to merge this in and cut 4.0.0-alpha2 for folks waiting. If anything did manage to slip through, I'll be available to triage things throughout the week after the day job.
luke.leber → changed the visibility of the branch 4.0.x to hidden.
luke.leber → changed the visibility of the branch 3433346-automated-drupal-11 to hidden.
Green on 10.4.0, 11.0.8, and 11.1.0.
Testing on PREVIOUS_MAJOR, PREVIOUS_MINOR, and CURRENT seems very logical to me. Setting to Needs Review for manual testing.
This module does, indeed need work, as there are missing schema definitions.
Leaving NW so that someone can look into the test failures, or I can pick them back up when I'm able.
Outdated by https://www.drupal.org/project/juicebox/issues/3494657 📌 Get tests passing on Gitlab CI Active (oops, I forgot I already did this work last year :O ).
Now that I've merged this in,pulling in the 4.0.x branch on a dev site via composer require drupal/colorbox:dev-4.0.x
will pull in the latest development build for the 4.0.x branch.
Subsequent patches can be applied on top of that. I'll see you over in the Drupal 11 issue.
The update bot has messed up the support ranges here.
The deprecation helper service was introduced in Drupal 10.1.3, but it has the module support range specified as >=10. That means that 10.0.0 through 10.1.2 users would have their sites killed by this.
The support range has to be at minimum ^10.1.3 || ^11
here. I strongly recommend using ^10.3 || ^11
since that's what we're able to test on though...
Setting back to needs work. If no one else picks this up before I'm able to, I'll make the commit myself, but in the meantime feel free.
Given these are test-only changes, I'll merge this in tomorrow evening if it hasn't been reviewed by then so we can move onto the Drupal 11 compatibility issue with confidence and get that one finished up by the weekend. 👍
FINALLY! A green run that actually runs the tests.
Setting to NR.
Hey, there are still some issues preventing this. Primarily not being able to run tests on Gitlab CI. Admittedly, I've slacked as a maintainer here, as I no longer manage any sites that actively use Juicebox, but I'll set aside time this through the weekend to hopefully grind through things and release something.
luke.leber → created an issue.
Isn't the required attribute the equivalent signal for non-visual users? It's announced by all major A/T tools.
Adding another signal is probably redundant IMO.
I gave !475 a test run, recompiled the UI, flushed some caches, and refreshed. Lo and behold, working required indicators.
This looks great to me, marking RTBC, as the accessibility concern is alleviated by this, even if a follow-up design task needs to happen.
luke.leber → created an issue.
This might be fine if we think SDC creators already know to output $attributes for other reasons, but #9 indicates that this isn't currently being consistently done in the wild.
We made the decision not to use $attributes
in our twig, as it'd tightly couple things to Drupal. We may be the outlier, but I'm living proof of the concern :-)
Added copy/paste example to I.S. w/o any external dependencies :-).
It's actually pretty simple to resolve if the component knows if it's being observed in Experience Builder.
A custom twig extension...
class XBExtension extends AbstractExtension {
public function getFunctions() {
return parent::getFunctions() + [
new TwigFunction('is_xb_editing', [
$this,
'isXbEditing',
]),
];
}
public function isXbEditing() {
return \Drupal::routeMatch()->getRouteName() === 'experience_builder.api.preview';
}
}
...and using it in the SDC template:
{% set expanded = is_xb_editing() %} {# <-- "in_preview" variable-like-thing #}
{% include '@my-upstream-components/accordion.twig' %}
That said, I wouldn't recommend writing this off as a niche problem. I can't think of any interactive / progressive disclosure elements from *any* design systems that wouldn't need a mechanism like this. I think it's more along the lines of not having any example components available in XB to have stepped on this yet. It'd be really unfortunate for each design system to have to roll their own "in_preview"-like feature.
That's pretty neat, @jessebaker. It definitely *helps*, but the editing UX still seems off as the accordion seems to re-flip back to closed every time it's re-rendered.
Strangely enough, the newly added component seems to be selected by default even after re-paint and it's hidden.
luke.leber → created an issue.
Testing out the idea #32 is probably blocked by https://www.drupal.org/project/experience_builder/issues/3491021 ✨ Leverage HTML comment annotations, remove wrapping div elements Active .
I did play around with the latest work in #3491021, but it was too unstable confirm without any doubt that the approach is viable. From the limited experimentation I've performed, I think there's a good chance it'll be workable as #3491021 wraps up though.
Here's a gif of the behavior (described in previous comment).
Even when the accordion is collapsed, are you able to move a component into a hidden slot via the Layers panel (left sidebar)?
Yep. That works.
That should already work with #3486785: Initial support for allowing interaction with the preview. Not sure if state is retained when exiting preview though, or if the whole canvas gets re-rendered and therefore the accordion reverts to collapsed.
Yep, the preview mode works perfectly -- things are able to be interacted with just fine in there. It's just the non-preview mode where it's impossible to use the mouse and/or keyboard to expand/collapse interactive things.
While this can be worked around for accordions (through adding an "expand by default" prop and temporarily adjust that while adding content), I don't think it'll be quite as easy for other interactive UI patterns, like multiple tabs. For completeness, this is how we've set up our Tabs implementation in Layout Builder (dragging a block over a tab activator will actually activate the tab in the LB editor):
This all looks good to me! I re-ran
# Set a cache entry to persist for 5 minutes with a tag of "test_tag".
vendor/bin/drush eval "\Drupal::cache()->set('test', 'value', time() + 300, ['test_tag']);" && echo 'Set a cache entry'
# Confirm the cache entry is valid.
vendor/bin/drush eval "echo 'The cache entry is...' . (\Drupal::cache()->get('test') ? 'valid' : 'invalid');"
# Invalidate the tag via an ECA action.
vendor/bin/drush eca:trigger:custom_event test && echo 'ECA invalidated the tag'
# Confirm the cache entry is no longer valid.
vendor/bin/drush eval "echo 'The cache entry is...' . (\Drupal::cache()->get('test') ? 'valid' : 'invalid');"
with...
- A single hard-coded tag
- A single tag that was resolved by a token
- No tags (invalidate everything)
I think this one's good to go! Great stuff!
Updated I.S. the xb link in toolbar works on HEAD now.
— Any chance you could add the missing test coverage? 🙏
All signs point to probably not 😭. NW and slowly backs away.
I suppose that an alternative approach might be to allow certain rendered elements receive input events, but that seems like it'd be quite the technical lift to accomplish.
Attached screenshot with more context.
However! when creating a brand new action (rather than starting with an old one, then patching), I get [warning] Undefined array key "backend" CacheActionBase.php:80
printed out.
I'm setting this back to NW. It looks like Drupal\eca_cache\Plugin\Action\CacheActionBase::getCacheBackend
may need an update to cope with derivative classes that do not operate on a specific backend.
As a result, I'm pretty sure that Drupal\eca_cache\Plugin\Action::execute
needs to be refactored to do a full cache flush and not exit early via
if (!($cache = $this->getCacheBackend())) {
return;
}
That said, I'm not 100% sure what the right API calls should be in if tags are empty.
if (empty($tags)) {
$cache->invalidateAll(); <-- definitely not this, but I'm not sure what to replace it with!
}
I was able to successfully test the invalidation action.
uuid: 46349853-db2b-4cfc-822c-9ac6680f2ff1
langcode: en
status: true
dependencies:
module:
- eca_base
- eca_cache
id: process_jfh9njk
modeller: bpmn_io
label: noname
version: ''
weight: 0
events:
Event_1bq8hvb:
plugin: 'eca_base:eca_custom'
label: Event_1bq8hvb
configuration:
event_id: test
successors:
-
id: Activity_087tmd6
condition: ''
conditions: { }
gateways: { }
actions:
Activity_087tmd6:
plugin: eca_raw_cache_invalidate
label: Activity_087tmd6
configuration:
tags: test_tag
backend: entity
key: 'values:node:1'
successors: { }
# Set a cache entry to persist for 5 minutes with a tag of "test_tag".
vendor/bin/drush eval "\Drupal::cache()->set('test', 'value', time() + 300, ['test_tag']);" && echo 'Set a cache entry'
# Confirm the cache entry is valid.
vendor/bin/drush eval "echo 'The cache entry is...' . (\Drupal::cache()->get('test') ? 'valid' : 'invalid');"
# Invalidate the tag via an ECA action.
vendor/bin/drush eca:trigger:custom_event test && echo 'ECA invalidated the tag'
# Confirm the cache entry is no longer valid.
vendor/bin/drush eval "echo 'The cache entry is...' . (\Drupal::cache()->get('test') ? 'valid' : 'invalid');"
Output should be:
Set a cache entry
The cache entry is...valid
ECA invalidated the tag
The cache entry is...invalid
My only concern here is one of performance. If token substitution happens, but yields an empty string, does that mean that this kicks in?
When empty, then the whole cache is being invalidated.
For larger sites, full cache invalidations can lead to some pretty significant performance degradation, and in some cases denial of service if the ECA workflow runs often enough.
I'll put it on my list to do tomorrow.
luke.leber → created an issue.
luke.leber → created an issue.
luke.leber → created an issue.
luke.leber → created an issue.
luke.leber → created an issue.
I saw this today too, but chalked it up to me removing, adding, and refactoring props and slots on the fly without any regard for B/C.
For the most part XB seemed to be pretty forgiving, but I did run into this a couple times.
Manually visiting /xb/node/1
following step #7 is a winner though (fyi)!
luke.leber → created an issue.
Opened a MR based on #2, except the composer.json
and *.info.yml
support ranges match in !10. Both changed to ^10.2 || ^11
.
luke.leber → made their first commit to this issue’s fork.
I'm closing this issue out! Beta 15 and later seems to gracefully resolve this through https://www.drupal.org/project/entity_usage/issues/3366527 ✨ Differentiate pending/old revisions and default translations Active .
🥳
Drupal will generate an error on the Status report page (admin/reports/status) when a site is using "READ-COMMITTED" whilst having a table that doesn't contain primary key. The reason for this error is that "READ-COMMITTED" does not work as intended when missing primary keys
Say there is a table on the database that is not used by Drupal. Would that also fall into making things "not work as intended", or is the requirement check a bit overzealous in what tables it's checking?
Thanks!
luke.leber → created an issue.
Reviewing MR !25, none of this appears to be needed.
This iframe is marked display:none
which effectively removes the DOM element from the accessibility tree.
The issue appears to be the result of naive accessibility scans and not the result of actual manual testing.
I recommend closing this issue, citing the display property on the iframe. If anything, I'd recommend removing the visibility CSS property as it's a no-op when paired with display none.
Marking as works as designed. If anyone has a11y citations suggesting otherwise, feel free to re-open, but all signs point to a buggy accessibility scanner as the cause here.
I know of at least 3 issues that were opened with confusion on this point. It may be a good idea to add some prominent documentation explaining this to the module page. $.02
+1 on deprecating the library.
The maintainer was explicitly presented with a dialog explaining what changing their username entails. They chose to do this despite the warnings about downstream chaos it would create.
To me, that means they probably shouldn't be an upstream dependency if such a change was made on a whim in this age of supply chain attacks.
I think that Jurgen is correct in #4. When invoking the Cache Tags Invalidator Service, that's a high level API that does not need to know / care about which backend is used.
All that needs to be passed to the service is an array of strings like node:##
and the API should be able to do the rest.
Alright! I've worked through some testing with MR !36 and there's one tiny thing that needs to be added.
Here's the originally reported issue:
After applying the MR and re-loading the page, the YouTube thumbnail is still broken! This is because the render cache still contains a bad entry.
Following a manual cache flush, everything is looking good!
It looks like we need an empty post-update hook added to flush out any bad cache entries, and then this is good to go!
Thanks Felipe, the merge request looks good.
I'll give it a manual test on a real site later on this weekend and if all goes well, we should have a new release by Monday.
Cheers
Thanks for the report. I just hit up https://oembed.com/providers.json and /live is indeed a valid oembed endpoint scheme.
We'll have to add some test coverage and a fix to a merge request to get this one fixed, if anyone picks this up before I'm able to.
Cheers!
Chiming in as an early-ish adopter of content moderation. When we upgraded to Drupal 8, workspaces wasn't really a thing. We now have years of muscle memory built up around content moderation, having explained in depth to our users why it works the way it works as opposed to something like, say, the Oasis Workflow plugin for wordpress (which has been lauded above the Drupal tooling btw...).
In my experience, teaching the technical mechanics of a solution has been secondary to the "why doesn't this work like other workflow solutions that I am used to" barrier.
For us, it would be a very hard sell a switch to something completely different again with different justifications involved. If a motion to strong-arm use of a specific workflow tool is inevitable, I would urge looking at the most popular (by number of installs globally) workflow tools and to derive the end-user facing terminology from those tools versus Drupal implementation detail nomenclature.
Not sure if this is helpful at this stage of the game or not, but I wanted to get this point of view out there.
What is the benefit of changing the name of the spaceless filter in a non-bc way?
Maybe I'm dense, but not causing excess contrib and custom disruption should be a chief consideration in the decision making process, right?
gábor hojtsy → credited luke.leber → .
luke.leber → made their first commit to this issue’s fork.
Re- #58
See https://fiddle.nette.org/twig/#3c6dbf089c
There is certainly changes in the generated output such that whitespace control operators are not a direct replacement for spaceless.
catch makes a good point in #56. The if statement could probably just be a version check on the twig dependency.
My main point here in #55 is that we probably shouldn't reimplement spaceless with a new filter name, as that would just cause needless untold fatal errors in custom and contributes themes on upgrade where it could otherwise be avoided if the same filter name is used.