Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dynamic background color #14895

Merged
merged 4 commits into from Jul 16, 2023

Conversation

kikuchan
Copy link
Contributor

Try to fix #14894

@github-actions
Copy link

📦 Preview the website for this branch here: https://deploy-preview-14895--ol-site.netlify.app/.

@kikuchan
Copy link
Contributor Author

I'm not sure what should happen if the background function returns undefined again in the test code, btw.

If you want something like this;

--- test/browser/spec/ol/renderer/canvas/vectortilelayer.test.js
+++ test/browser/spec/ol/renderer/canvas/vectortilelayer.test.js
@@ -334,6 +334,7 @@ describe('ol/renderer/canvas/VectorTileLayer', function () {
           undefined,
           'rgba(255, 0, 0, 0.5)',
           'rgba(0, 0, 255, 0.5)',
+          undefined,
         ];

         expect(resolution).to.be(map.getView().getResolution());
@@ -349,6 +350,8 @@ describe('ol/renderer/canvas/VectorTileLayer', function () {
         expect(layer.getRenderer().container.style.backgroundColor).to.be(
           'rgba(0, 0, 255, 0.5)'
         );
+        map.renderSync();
+        expect(layer.getRenderer().container.style.backgroundColor).to.be('');
         done();
       });
     });

The patch would look like this;

--- src/ol/renderer/canvas/Layer.js
+++ src/ol/renderer/canvas/Layer.js
@@ -195,12 +195,8 @@ class CanvasLayerRenderer extends LayerRenderer {
       this.container = container;
       this.context = context;
     }
-    if (
-      !this.containerReused &&
-      backgroundColor &&
-      !this.container.style.backgroundColor
-    ) {
-      this.container.style.backgroundColor = backgroundColor;
+    if (!this.containerReused && this.container && this.container.style) {
+      this.container.style.backgroundColor = backgroundColor || null;
     }
   }

@ahocevar
Copy link
Member

Thanks for your attempt to fix this, @kikuchan. The solution you chose changes the behavior in the case where multiple layers in the layer stack have a background configured. So I would suggest setting the style to null before the container is passed to the first layer renderer, and leave the rest of the code as it was before your changes.

@kikuchan kikuchan force-pushed the fix-dynamic-background-color branch from c17d60e to c7b5ad0 Compare July 13, 2023 02:22
@kikuchan
Copy link
Contributor Author

Thank you for your reply!
How about this?

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks for your continued effort! I added one minor suggestion, which you might want to consider.

src/ol/renderer/canvas/Layer.js Outdated Show resolved Hide resolved
@kikuchan
Copy link
Contributor Author

Thank you for the suggestion.
I had to change a test code because it used dummy container directly, and it failed with the change.

@kikuchan kikuchan requested a review from ahocevar July 15, 2023 00:34
Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks @kikuchan!

@ahocevar ahocevar merged commit 5473a21 into openlayers:main Jul 16, 2023
8 checks passed
@kikuchan kikuchan deleted the fix-dynamic-background-color branch July 16, 2023 13:53
@kikuchan
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic background color change doesn't work
2 participants