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

Update api.TextMetrics compatibility for Chrome and Edge #5659

Merged
merged 4 commits into from
Feb 11, 2020
Merged

Update api.TextMetrics compatibility for Chrome and Edge #5659

merged 4 commits into from
Feb 11, 2020

Conversation

1valdis
Copy link
Contributor

@1valdis 1valdis commented Feb 5, 2020

  • Summarize your changes: Chrome has four more properties from TextMetrics enabled by default since release 77 of desktop, Android, and Webview browsers.

  • Data: link to resources that verify support information: Chrome Platform Status: New TextMetrics object in canvas says that actualBoundingBoxLeft, actualBoundingBoxRight, actualBoundingBoxAscent and actualBoundingBoxDescent are enabled by default since release 77.

  • Data: if you tested something, describe how you tested with details like browser and version:
    I made a quick test on Chrome 79 desktop:

document.createElement('canvas').getContext('2d').measureText('testing metrics')

It gives an object with the following properties:

width: 64.462890625
actualBoundingBoxLeft: -0.1123046875
actualBoundingBoxRight: 64.1015625
actualBoundingBoxAscent: 7.1728515625
actualBoundingBoxDescent: 2.2119140625
  • Review the results of the linter and fix problems reported (If you need help, please ask in a comment!): done, all green.

  • Link to related issues or pull requests, if any: haven't found anything related to TextMetrics in Chrome 77.

@ghost ghost added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Feb 5, 2020
@1valdis
Copy link
Contributor Author

1valdis commented Feb 5, 2020

I wonder if instead of removing flag data, I should make them arrays of objects, as such:

[
  {
    "version_added": "77"
  },
  {
    "version_added": true,
    "version_removed": "77",
    "flags": [
      {
        "type": "preference",
        "name": "Experimental Web Platform Features"
      }
    ]
  }
]

@1valdis 1valdis removed the request for review from queengooborg February 10, 2020 07:44
@1valdis
Copy link
Contributor Author

1valdis commented Feb 11, 2020

Is anyone here? I see other PRs being created and merged and mine just getting ignored for a week 🤔

@Elchi3
Copy link
Member

Elchi3 commented Feb 11, 2020

Hey, sorry for the slow response, but as you've seen there are a lot of PRs here, so sometimes things can take a while.

I wonder if instead of removing flag data, I should make them arrays of objects, as such:

Yeah, a range could be done here, but given we don't have the start of the range for when it was behind a flag, I wonder how useful this one really is.

For Edge, you could change the false values to 79, given Edge uses Chromium 79 now :)

@1valdis
Copy link
Contributor Author

1valdis commented Feb 11, 2020

@Elchi3

I wonder how useful this one really is.

Yep I saw ranges while a released feature was behind a flag, but those were with specific versions. Although the data that this was behind a flag earlier, even though specific versions are unknown, could be useful for some historical reasons. So should I leave it as it is or create an array?

I'll edit Edge data soon.

@Elchi3
Copy link
Member

Elchi3 commented Feb 11, 2020

So should I leave it as it is or create an array?

I wish we had a clear policy about this, but we haven't come to an agreement on this matter yet, see #3318.
If you're a fan of preserving data, then you could add the arrays. However as we're lacking a policy, I would also be fine with the PR as it is now.

I'll edit Edge data soon.

Thank you!

@1valdis
Copy link
Contributor Author

1valdis commented Feb 11, 2020

@Elchi3

If you're a fan of preserving data, then you could add the arrays.

Yep I like that better. You never know who might need it, and for what bizarre reasons :). The PR should be finished in a few hours. Should I somehow notify you when it's done?

@Elchi3
Copy link
Member

Elchi3 commented Feb 11, 2020

Thanks for your work and going the extra mile here!
Feel free to notify me, I will do my best to review it timely :)

@1valdis
Copy link
Contributor Author

1valdis commented Feb 11, 2020

@Elchi3 done.

Note though that I haven't tested the change for Edge, so I'm counting on your data. I will rename the PR to reflect that it touches Edge too.

@1valdis 1valdis changed the title Update api.TextMetrics compatibility for Chrome Update api.TextMetrics compatibility for Chrome and Edge Feb 11, 2020
Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thank you! Nice work! 👍

@Elchi3 Elchi3 merged commit 4007d36 into mdn:master Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants