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

Recent TextMetrics baseline changes may have jumped the gun #3995

Closed
domenic opened this Issue Sep 4, 2018 · 9 comments

Comments

6 participants
@domenic
Member

domenic commented Sep 4, 2018

#3931 removed the attributes hangingBaseline, alphabeticBaseline, and ideographicBaseline from the TextMetrics interface, instead adding a new method getBaselines() which returns a { hanging, alphabetic, ideographic } dictionary.

At the time we merged this, I did not realize that Safari had already shipped the previous properties to their stable channel. (I thought this was part of the still-evolving new-ish canvas APIs which nobody had yet shipped.) As such, I don't think landing that pull request without Safari's participation was the right thing to do.

On blink-dev, @fserb says

Safari already has some of the metrics implemented but since the spec was recently updated some work still needs to be done (advances and getBaselines) to have full interoperability with Chrome.

and

The baselines changes does add an incompatibility. But this was done to address deeper problems with the spec definition around baselines (i.e., nobody can agree what a hanging or an ideographic baseline is supposed to mean). This meant that the original values were meaningless heuristics (hanging is ALWAYS 80% of emHeightAscent, not spec'ed anywhere, but following some Apache guideline). The new spec basically states that, if you are not sure what the baseline is - i.e., not available on the font or on your text layout system -, don't return it.

Which are reasonable arguments for this change. But I don't think we fully considered all the options. To my mind our options are:

  1. Keep the old and new APIs in the spec, with the old ones having more heuristic/less useful definitions.
  2. Keep the old API in the spec, but change the definition so that you should return null for cases where you're not sure what the baseline is. Have Safari change their implementation to return null in such cases.
  3. Keep the new API in the spec, removing the old one, and have Safari phase out their old API over time.

We chose option (3), but I want to make sure that implementers are on board with that choice---in particular Safari, since they already ship the old API.

Of note, if we choose (2), the current blink-dev intent to ship should be put on hold. So if someone prefers that option we should say so ASAP.

/cc @smfr and @othermaciej for Safari perspectives in particular, and @whatwg/canvas in general.

@litherum

This comment has been minimized.

Show comment
Hide comment
@litherum

litherum Sep 5, 2018

We don't set the metrics from the font:

metrics->setHangingBaseline(fontMetrics.ascent() - offset.y());
metrics->setAlphabeticBaseline(-offset.y());
metrics->setIdeographicBaseline(-fontMetrics.descent() - offset.y());

So I'm not sure who is actually relying on this data.

It's a good idea to make these values optional.

litherum commented Sep 5, 2018

We don't set the metrics from the font:

metrics->setHangingBaseline(fontMetrics.ascent() - offset.y());
metrics->setAlphabeticBaseline(-offset.y());
metrics->setIdeographicBaseline(-fontMetrics.descent() - offset.y());

So I'm not sure who is actually relying on this data.

It's a good idea to make these values optional.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 6, 2018

Member

Thanks @litherum. I'm unclear whether you/WebKit would prefer (3) or (2), from that information. Any thoughts?

Member

domenic commented Sep 6, 2018

Thanks @litherum. I'm unclear whether you/WebKit would prefer (3) or (2), from that information. Any thoughts?

@othermaciej

This comment has been minimized.

Show comment
Hide comment
@othermaciej

othermaciej Sep 7, 2018

Does anyone have data on whether existing content makes use of either API?

I think we have an implementation of the various baselines for completeness rather than because of any specific demand, so we are likely mostly indifferent to the shape of the API, unless there is important content depending on it. I am not aware of any.

In general, it seems to me that a possibly-null property is a more elegant way to represent optionality than a get method returning a dictionary. Dictionaries seem better used in the case where the set of optional values is large or open-ended, or as parameters to methods to avoid the need for null placeholders. In a case like this, it makes access less convenient and less efficient. That said, I am not sure what the precedent is for this sort of thing in other web APIs, and it seems reasonable to follow the normal precedent.

othermaciej commented Sep 7, 2018

Does anyone have data on whether existing content makes use of either API?

I think we have an implementation of the various baselines for completeness rather than because of any specific demand, so we are likely mostly indifferent to the shape of the API, unless there is important content depending on it. I am not aware of any.

In general, it seems to me that a possibly-null property is a more elegant way to represent optionality than a get method returning a dictionary. Dictionaries seem better used in the case where the set of optional values is large or open-ended, or as parameters to methods to avoid the need for null placeholders. In a case like this, it makes access less convenient and less efficient. That said, I am not sure what the precedent is for this sort of thing in other web APIs, and it seems reasonable to follow the normal precedent.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Sep 10, 2018

Member

The reason for the dictionary was to align with a CSS Houdini API I thought. If it's only to be able to express optionality, using null does seem better.

Member

annevk commented Sep 10, 2018

The reason for the dictionary was to align with a CSS Houdini API I thought. If it's only to be able to express optionality, using null does seem better.

@fserb

This comment has been minimized.

Show comment
Hide comment
@fserb

fserb Sep 10, 2018

Contributor

The CSS Houdini API is still under heavy discussion, and I didn't see strong arguments there against this. The argument as I remember was that most of those values are unused/unavailable.

I don't mind using null instead of the dictionary. I agree with @othermaciej's and @annevk's argument. It addresses the compatibility issue with the already launched version, and it's easier to access.

But just for the record, let's clarify that we are doing this understanding that some of those baselines are rarely/never available. But since they are limited in number, having those null values hanging there is a good tradeoff.

Contributor

fserb commented Sep 10, 2018

The CSS Houdini API is still under heavy discussion, and I didn't see strong arguments there against this. The argument as I remember was that most of those values are unused/unavailable.

I don't mind using null instead of the dictionary. I agree with @othermaciej's and @annevk's argument. It addresses the compatibility issue with the already launched version, and it's easier to access.

But just for the record, let's clarify that we are doing this understanding that some of those baselines are rarely/never available. But since they are limited in number, having those null values hanging there is a good tradeoff.

@Yay295

This comment has been minimized.

Show comment
Hide comment
@Yay295

Yay295 Sep 11, 2018

Contributor

In the interest of bringing up possible future changes, would it be possible to expand the scope of this to more than just Canvases; for example SVG, or even plain HTML text? It also feels like there could be some integration with the Font Loading API. If these changes sound reasonable I thought it would be good to start thinking about them now before it gets even more set in stone.

Contributor

Yay295 commented Sep 11, 2018

In the interest of bringing up possible future changes, would it be possible to expand the scope of this to more than just Canvases; for example SVG, or even plain HTML text? It also feels like there could be some integration with the Font Loading API. If these changes sound reasonable I thought it would be good to start thinking about them now before it gets even more set in stone.

@litherum

This comment has been minimized.

Show comment
Hide comment
@litherum

litherum Sep 14, 2018

would it be possible to expand the scope of this to more than just Canvases

This is an interesting thought, given I've seen developers often creating a Canvas2DContext just to measure some text, and then throw away the canvas

litherum commented Sep 14, 2018

would it be possible to expand the scope of this to more than just Canvases

This is an interesting thought, given I've seen developers often creating a Canvas2DContext just to measure some text, and then throw away the canvas

domenic added a commit that referenced this issue Sep 14, 2018

Revert "Add advances to TextMetrics and change baselines API"
This reverts commit 7711a1f. As
discussed in #3995, these changes were made prematurely without
appropriate implementer signoff. Since then, a plethora of issues around
the changes here have been opened up (e.g. #3994, #4023, #4030, #4033,
#4034). We revert these changes until a more complete and agreed-upon
specification can replace them.

Closes #3995.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 14, 2018

Member

Hey all,

I've opened #4037 to revert #3931 for now. I'm glad that we've got such a detailed and spirited discussion going around how to create a better spec for these APIs, and I'm sorry that we editors didn't do our part in appropriately fostering that discussion before merging the PR.

It looks like there are a lot of threads already listing issues we'd need to resolve before reintroducing the API changes there: #3994, #4023, #4030, #4033, #4034. When the revert PR (#4037) is merged, this issue will close, so let's be sure to migrate any additional concerns and proposals from this thread into their own dedicated issues. From my understanding those would be:

  • Using nullable attributes vs. using a dictionary (although this may be dependent on #4033)
  • Expanding the scope beyond canvas (although this may be the purpose of the Houdini font metrics API already)

Thanks all, and apologies again for the premature merging.

Member

domenic commented Sep 14, 2018

Hey all,

I've opened #4037 to revert #3931 for now. I'm glad that we've got such a detailed and spirited discussion going around how to create a better spec for these APIs, and I'm sorry that we editors didn't do our part in appropriately fostering that discussion before merging the PR.

It looks like there are a lot of threads already listing issues we'd need to resolve before reintroducing the API changes there: #3994, #4023, #4030, #4033, #4034. When the revert PR (#4037) is merged, this issue will close, so let's be sure to migrate any additional concerns and proposals from this thread into their own dedicated issues. From my understanding those would be:

  • Using nullable attributes vs. using a dictionary (although this may be dependent on #4033)
  • Expanding the scope beyond canvas (although this may be the purpose of the Houdini font metrics API already)

Thanks all, and apologies again for the premature merging.

@litherum

This comment has been minimized.

Show comment
Hide comment
@litherum

litherum Sep 17, 2018

Also: don’t forget #4026

litherum commented Sep 17, 2018

Also: don’t forget #4026

domenic added a commit that referenced this issue Sep 17, 2018

Revert "Add advances to TextMetrics and change baselines API"
This reverts commit 7711a1f. As
discussed in #3995, these changes were made prematurely without
appropriate implementer sign-off. Since then, a plethora of issues
around the changes here have been opened up (e.g. #3994, #4023, #4026,
#4030, #4033, #4034). We revert these changes until a more complete and
agreed-upon specification can replace them.

Closes #3995.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment