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

Calculate correct canvas size if more than one parent container has a css transform property #11493

Merged
merged 6 commits into from
Mar 10, 2022

Conversation

avpeery
Copy link
Contributor

@avpeery avpeery commented Feb 9, 2022

Fixes #11492. Original solution did not consider if more parent containers had transform properties. This fix continues looking for transform values, and avoids exiting early.

Launch Checklist

  • briefly describe the changes in this PR
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fixes canvas size if more than one parent container has a transform css property.</changelog>

@avpeery avpeery changed the title Avpeery/fix canvas sizing if parent container Calculate correct canvas size if more than one parent container has a css transform property Feb 9, 2022
@avpeery avpeery requested a review from ryanhamley February 9, 2022 21:24
Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

I spent some time trying this out and I think that there's two potential issues.

  1. We only apply the outermost transform: scale to the container width. This works in this case where there's 2 transformed elements but only one has a scaled transform. If we have more than one parent element with transform: scale, we won't get the correct output.

Something like this doesn't seem to work. Can/should we apply the transform at each step to end up with the final container width?

        #new-container {
          width: 1200px;
          height: 800px;
          transform: scale(2);
          transform-origin: top left;
        }

        #scaled-container {
          width: 1920px;
          height: 1080px;
          transform: scale(0.35);
          transform-origin: top left;
          background-color: grey;
        }

        #map-container {
          width: 1720px;
          height: 880px;  
          transform: translate(100px, 100px);
        }

        #map {
          height: 100%;
          width: 100%; 
        }

    <div id="outer-container">
      <div id="scaled-container">
        <div id="map-container">
          <div id="map"></div>
        </div>
      </div>
    </div>
  1. I think I see why you had the && !transformValues. It's not exactly the right check, but without it, this loop will always iterate all the way out to the <html> tag. This could easily be a performance drag, especially if there's quite a bit of nesting in the HTML structure. I'm not sure how to get around this without limiting the potential number of transforms that can be applied.

@avpeery
Copy link
Contributor Author

avpeery commented Feb 10, 2022

@ryanhamley Thanks for digging into this. It seems the !transformValues check wasn't very useful - if no parents containers had a transform css property it would end up looping through till the outermost container anyways. One potential approach that was previously discussed could be getting clientWidth or offsetWidth (whichever is more appropriate) and get the difference with the getBoundingClientRect to get an approximate scale? ETA: this approach ended up not being great.

@avpeery avpeery requested a review from ryanhamley March 9, 2022 23:03
@avpeery
Copy link
Contributor Author

avpeery commented Mar 9, 2022

@ryanhamley This solution works in most transform use-cases with the exception of rotateY and rotateX. Since rotateY and rotateX did not previously work since using getBoundingClientRect was introduced, I think it is ok to not support these use-cases, that are likely very unlikely. Let me know what you think

Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

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

This works well to fix the reported issue. Handling rotation seems to involve more than just applying the transformation since it breaks zooming around center and possibly other interactions, so it's fine to not handle it, especially since the use case is unclear.

@avpeery avpeery merged commit 0b81f60 into main Mar 10, 2022
@avpeery avpeery deleted the avpeery/fix-canvas-sizing-if-parent-container branch March 10, 2022 17:53
ahocevar pushed a commit to ahocevar/mapbox-gl-js that referenced this pull request Mar 23, 2022
… css transform property (mapbox#11493)

* fix canvas sizing to not stop checking transform at parent container
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map canvas sizing regressed when multiple parent elements have CSS transforms applied to them
2 participants