Skip to content

Fix Map Block Crash With >2 Points #14182

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.

Sign up for GitHub

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

Jump to bottom
Merged
jeffersonrabb merged 1 commit into from
Dec 11, 2019
Merged

Fix Map Block Crash With >2 Points #14182

jeffersonrabb merged 1 commit into from
Dec 11, 2019

Conversation

@jeffersonrabb
Copy link
Contributor

@jeffersonrabb jeffersonrabb commented Dec 5, 2019

Changes proposed in this Pull Request:

As reported in #14177 the Map block crashes at the moment when a third point is added. If a map has only one point the Mapbox zoom control is shown. When a second point is added the map's scale is calculated based on the two positions and the zoom control is removed. The crash occurs when we call removeControl() on a control that has already been removed. This is why the issue occurs only when a third point is added to a map—it's removed when point 2 is added then removed again for point 3. It's difficult to say why this is popping up now—it could be related to a change in the Mapbox library, but tough to guess. This PR resolves the issue by wrapping removeControl() in a try/catch, suppressing the error. It does not appear that there are any side effects to the error so this should be a solid solution.

Fixes #14177

Testing instructions:

  • On master create new Map.
  • Add two points and observe no issues.
  • Add a third point and observe the block crashes.
  • Switch to this branch, rebuild blocks, and repeat.
  • Observe the block does not crash no matter how many points are added.

Proposed changelog entry for your changes:

  • Resolve an error that occurs when more than two points are added to the Map block.

@jeffersonrabb jeffersonrabb requested a review from a team December 5, 2019 13:13
@matticbot
Copy link
Contributor

matticbot commented Dec 5, 2019

Caution: This PR has changes that must be merged to WordPress.com
Hello jeffersonrabb! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D36277-code before merging this PR. Thank you!

@jeffersonrabb jeffersonrabb self-assigned this Dec 5, 2019
@jeffersonrabb jeffersonrabb requested a review from jeherve December 5, 2019 13:13
@jetpackbot
Copy link
Collaborator

jetpackbot commented Dec 5, 2019

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against f4e2976

@jeherve jeherve added [Block] Map [Status] Needs Review This PR is ready for review. [Type] Bug When a feature is broken and / or not performing as intended labels Dec 5, 2019
@jeherve jeherve added this to the 8.1 milestone Dec 5, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Dec 11, 2019
jeherve
jeherve approved these changes Dec 11, 2019
View reviewed changes
Copy link
Member

@jeherve jeherve 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 in my tests. Should be good to merge!

@jeffersonrabb jeffersonrabb merged commit 2cf820b into master Dec 11, 2019
@jeffersonrabb jeffersonrabb deleted the fix/map-block-crash-when-third-point-added branch December 11, 2019 22:52
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 11, 2019
jeherve added a commit that referenced this pull request Dec 13, 2019
@jeherve
Changelog: add #14182
2b4720c
zinigor added a commit that referenced this pull request Dec 30, 2019
@jeherve @zinigor
Changelog: 8.1 additions (#14166)
fff089c 
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <[email protected]>
zinigor added a commit that referenced this pull request Dec 30, 2019
@jeherve @zinigor
Changelog: 8.1 additions (#14166)
d4a7565 
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

@jeffersonrabb jeffersonrabb

Labels

[Block] Map Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended

Projects

None yet

Milestone

8.1

Development

Successfully merging this pull request may close these issues.

Map Block: multiple location markers error

5 participants

@jeffersonrabb @matticbot @jetpackbot @jeherve