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

Rename ImageBitmapOptions dictionary to ImageBitmapOptionsInit. #4248

Closed
wants to merge 1 commit into from

Conversation

kenrussell
Copy link
Member

@kenrussell kenrussell commented Dec 20, 2018

Add ImageBitmapOptions interface which can be constructed from
ImageBitmapOptionsInit.

The ImageBitmapOptions interface allows easy detection of which
ImageBitmap features the user agent supports, especially as
ImageBitmapOptionsInit evolves.

Representatives from Apple, Google, Microsoft and Mozilla have agreed
at recent WebGL working group meetings to add this feature detection
mechanism. The pattern could be generalized as a Web IDL construct for
any web-exposed dictionaries where easy feature detection is desired.


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 7:59 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

<html>
<head><title>504 Gateway Time-out</title></head>
<body bgcolor="white">
<center><h1>504 Gateway Time-out</h1></center>
<hr><center>nginx/1.10.3</center>
</body>
</html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@kenrussell
Copy link
Member Author

@grorg @jdashg @RafaelCintron this is what was discussed at the WebGL face-to-face meeting, but without introducing a new ImageBitmapOptions interface type. While writing that up, it became clear that it was redundant. This is now a small addition providing the key feature detection mechanism that was discussed.

@jungkees
Copy link
Contributor

@kenrussell, I don't think adding a platform API to feature-detect a dictionary has been a general approach. @annevk, does firefox have a plan to change https://dxr.mozilla.org/mozilla-central/source/dom/webidl/WindowOrWorkerGlobalScope.webidl#38 to what's defined in the spec?

@kenrussell
Copy link
Member Author

@jdashg is the Mozilla employee who's been working most closely on this.

On today's WebGL conference call a different approach was suggested involving making ImageBitmapOptions an interface, providing a new constructor taking a dictionary, and changing createImageBitmap to take either that interface, or "any" - which would be the existing dictionary. This would avoid duplicating the dictionary. I'll prototype this in Chromium to see if it can work, and will update this PR.

(One way or another, the lack of ability to do feature detection on dictionaries defined in various web specs is highly problematic. If the approach currently suggested here is palatable, perhaps we should do it as an example for other APIs to follow.)

@RafaelCintron
Copy link

@jungkees , even if all browsers implement the currently speced members, we end up in the same problem if we ever add a new setting to the dictionary.

WebGL solved a similar problem with WebGLContextAttributes where the API returns a dictionary containing the settings the browser honored. Settings absent from the returned dictionary are not supported.

I look forward to seeing the result of @kenrussell 's investigation.

@domenic
Copy link
Member

domenic commented Dec 21, 2018

This is a particularly unusual method of dictionary feature detection, which I do not think we should include.

Similarly, switching from dictionaries to interfaces is also an unprecedented way of doing feature detection for options objects, which I would not encourage.

There is a canonical pattern for such feature detection, as discussed in whatwg/webidl#107. It's just ugly. I can understand the desire for something less ugly. I am not convinced it's a high priority to make a less-ugly version, and get that implemented and shipped everywhere. But if the ImageBitmap implementers do, then I'd suggest they use one of the established patterns we have, such as getContextAttributes()-equivalent on the returned ImageBitmap (returns the dictionary of applied options), or canShare()-equivalent (returns a boolean true or false as to whether the options would be accepted).

@kenrussell
Copy link
Member Author

@domenic Neither of those two options works for the ImageBitmap use case. The browser is required to ignore unsupported dictionary members, so it would have to return true from "canCreateImageBitmap" if the passed dictionary contains attributes the browser doesn't know how to handle.

The point of the proposed createImageBitmapOptions is to filter out those unhandled attributes so that the user knows exactly what the browser supports. It is not workable to require invoking createImageBitmap and checking the result; it's a costly operation, often involving decoding of large images, and applications need to decide which of various alternatives to use ahead of time.

Given the prevalence of this problem on the web platform, we could start a new convention like "filterImageBitmapOptions", "filterUnsupportedImageBitmapOptions", or similar, if that is more palatable. Otherwise, we'll reify ImageBitmapOptions into an interface in a backward-compatible way and solve the problem only for this API. It would be better to address this longstanding web platform problem more generally, though.

@domenic
Copy link
Member

domenic commented Dec 22, 2018

The browser is required to ignore unsupported dictionary members, so it would have to return true from "canCreateImageBitmap" if the passed dictionary contains attributes the browser doesn't know how to handle.

This is up to the spec to define; it's not an immutable law. The easiest way would be to take an object and check its properties, for example.

@annevk
Copy link
Member

annevk commented Jan 3, 2019

It is not workable to require invoking createImageBitmap and checking the result

But you can invoke it with a cheap image and see which getters the browser invoked and use that knowledge going forward, no? You could even throw from the last getter the browser has to check as the order is defined, meaning it would throw at the IDL layer before you even hit any of the image components in play.

@annevk annevk added addition/proposal New features or enhancements topic: canvas labels Jan 3, 2019
@travisleithead
Copy link
Member

An alternative that could be possible is to follow the pattern in use by fetch's Request and RequestInit class/interface and dictionary, respectively. In that pattern, the members on the dictionary mirror the attributes on the class/interface, and so the class/interface object can be used to trivially feature-detect the supported dictionary members.

The difference here is that the ImageBitmap is instantiated via a create method (rather than the more commonly recommended constructor/new pattern), and the ImageBitmapOptions has evolved without mirroring its options on the ImageBitmap.

I don't think it would be that strange to add the ImageBitmapOptions members to the ImageBitmap as attributes, since most of them already have a very sensible default value (e.g., default), though I wonder about resizeQuality's value of low.

@kdashg
Copy link

kdashg commented May 1, 2019

Are there any other blockers here?

@domenic
Copy link
Member

domenic commented May 2, 2019

Besides the fundamental design disagreements discussed upthread, you mean? Nobody seems to have responded to those, so that's probably the most important blocker.

@kenrussell
Copy link
Member Author

I apologize for not coming back to this pull request and will do so soon. Wanted to prototype the alternatives in Chromium before updating it, and higher-priority issues keep coming up.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 11, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 22, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 22, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 23, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 23, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 23, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 23, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 23, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

It basically adds a new interface to hold imagebitmapoptions
while preserving the same old functionality with an imagebitmapoptionsinit.

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 24, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

It basically adds a new interface to hold imagebitmapoptions
while preserving the same old functionality with an imagebitmapoptionsinit.

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 24, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

It basically adds a new interface to hold imagebitmapoptions
while preserving the same old functionality with an imagebitmapoptionsinit.

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 24, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

It basically adds a new interface to hold imagebitmapoptions
while preserving the same old functionality with an imagebitmapoptionsinit.

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 24, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

It basically adds a new interface to hold imagebitmapoptions
while preserving the same old functionality with an imagebitmapoptionsinit.

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 24, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

It basically adds a new interface to hold imagebitmapoptions
while preserving the same old functionality with an imagebitmapoptionsinit.

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 24, 2019
This is a prototype taking into account the feedback from
whatwg/html#4248 .

It basically adds a new interface to hold imagebitmapoptions
while preserving the same old functionality with an imagebitmapoptionsinit.

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
@kenrussell kenrussell force-pushed the add-image-bitmap-options branch from 04af5f3 to d3e38fe Compare October 25, 2019 22:50
@kenrussell
Copy link
Member Author

Reviewers: @Juanmihd from the Chrome team has successfully prototyped this revised API change:
https://chromium-review.googlesource.com/1698123

It is backward-compatible, and is potentially generalizable into a Web IDL construct (perhaps an extended attribute to be applied to dictionaries) which would auto-generate both a JavaScript dictionary and interface for a Web IDL dictionary.

Could you please review this revision? Thank you!

@kenrussell kenrussell changed the title Add createImageBitmapOptions. Rename ImageBitmapOptions dictionary to ImageBitmapOptionsInit. Oct 25, 2019
@annevk
Copy link
Member

annevk commented Oct 28, 2019

Could you explain what has changed since the previous review and why the outstanding feedback, e.g., #4248 (comment), no longer applies?

@kenrussell
Copy link
Member Author

@annevk the members of the WebGL working group - representing Apple, Google, Microsoft and Mozilla - agree unanimously that a better feature detection mechanism is needed than hooking an ImageBitmapOptions dictionary, creating a small ImageBitmap, and seeing which getters of the dictionary were called.

This pull request refactors ImageBitmapOptions into a dual interface/dictionary pair, ImageBitmapOptions and ImageBitmapOptionsInit, following @travisleithead 's suggestion above regarding the Fetch API's Request and RequestInit pattern.

It would be technically possible to add the ImageBitmapOptions members to the existing ImageBitmap interface itself, but I think this would lead to developer confusion. For example, specifying resizeWidth and resizeHeight in ImageBitmapOptions changes the width and height attributes of the resulting ImageBitmap, and adding them as additional members would be redundant. ImageBitmaps can be created from other ImageBitmaps, so policy questions about whether the premultiplyAlpha attribute should reflect the most recently passed one, or some union of all of them, would arise. I think leaving ImageBitmap as a mostly-opaque handle, and reflecting the user agent's supported creation options, is the more desirable approach.

Could this please be re-reviewed? Thanks.

@domenic
Copy link
Member

domenic commented Oct 28, 2019

Can those members of the working group please present their arguments to the editors of HTML? Or at least, answer the arguments that the editors of HTML have previously put forward?

@kenrussell
Copy link
Member Author

@domenic what specific arguments would you like presented? I thought I answered the earlier questions and points in my message above.

This newly uploaded pull request eliminates the ad-hoc feature detection mechanism proposed earlier in this thread with one that is general, and which could be extended to other web APIs if desired.

@annevk
Copy link
Member

annevk commented Oct 29, 2019

If you are proposing this as a general mechanism that discussion ought to be had with the Web IDL community. Thus far there's been no agreement there and adding a class-per-dictionary doesn't seem great.

Also, Fetch doesn't establish precedent here as the Request and Response classes do not exist for feature detection.

@kenrussell
Copy link
Member Author

@annevk certainly not proposing to add a class per dictionary throughout the HTML specification - only suggesting that the approach now taken in this pull request is generalizable.

Could this revised pull request please be reviewed?

@domenic
Copy link
Member

domenic commented Oct 29, 2019

@domenic what specific arguments would you like presented? I thought I answered the earlier questions and points in my message above.

The only answer I saw to #4248 (comment) was "agree unanimously that a better feature detection mechanism is needed". No technical arguments were presented to support this position.

Additionally, the arguments that this needs a general solution are being answered in two ways: one, with a claim that the proposed solution is general; but two, with the claim that the proposed solution is not proposed to be used generally. We need to get consensus within the Web IDL community on a general solution before accepting such a design.

@domenic
Copy link
Member

domenic commented Oct 29, 2019

In response to the 3 repetitions of "Could this revised pull request please be reviewed?", I'll note that that is what we are doing. Pull request review for additions to the HTML Standard doesn't consist solely of finding typos line by line. It primarily consists of evaluating the addition presented as to whether it fits in with the architecture of the web platform and the HTML Standard itself.

@kdashg
Copy link

kdashg commented Oct 29, 2019

Personally I'm open to suggestions. We've identified a pretty bad developer pain-point, and have provided a proposal to address it. If we don't know how to move this forward, we need your help solving this developer need.

Ultimately, it's not actually the WebGL WG's problem to solve, it's HTML's problem that we're asking for a solution for. We're just trying to jump-start progress here.

@kenrussell
Copy link
Member Author

@domenic I apologize, I misunderstood. It sounded like none of the HTML editors had looked at the body of this pull request.

The current state of the art of detecting supported dictionary members described in whatwg/webidl#107 is complex and error prone. One suggestion above - throwing on the "last" getter invoked against the dictionary, in order to avoid having to provide a valid image to createImageBitmap - has the following problem: on a user agent which doesn't support all of the specified dictionary members, there's no way to know a priori which getters will be called and which won't be. The application code trying to do the feature detection can't know which will be the "last" getter called. This problem affects both the present, where some browsers are catching up with the spec, and the future, when new dictionary members are added. It's unfortunate to have to synthesize a valid image - likely by creating a Blob containing handwritten data for a small JPEG or PNG - just to figure out whether the user agent supports the resizing or image orientation options.

Does this provide enough evidence that an improvement should be made, at least for the ImageBitmap APIs?

This comment above attempts to motivate the design decision to reify ImageBitmapOptions itself, rather than reflecting its properties on the created ImageBitmap. What are your thoughts on that aspect?

Let's put aside my statements that this is a general solution - since the more general Web IDL question of reifying dictionaries in JavaScript hasn't been solved in 3+ years, I'd like to not gate this pull request on solving that problem. I do however hope that this PR might inform a way forward for dictionary feature detection in other web APIs.

@annevk
Copy link
Member

annevk commented Oct 30, 2019

Yes, there are some downsides, but it's not as complicated as you make it out to be and you have to understand that there's a lot of APIs with similar oddities and we should really solve for them holistically. Meanwhile I'd recommend developers to use something along these lines, which seems to work pretty well (I'll note that Firefox doesn't seem to support the options argument currently):

async function isImageBitmapOptionSupported(feature) {
  try {
    await self.createImageBitmap(new ImageData(1,1), { get [feature]() { throw 1 } });
  } catch (e) {
    if (e === 1) {
      return true;
    }
  }
  return false;
}
isImageBitmapOptionSupported("imageOrientation").then(isSupported => ...);

@kenrussell
Copy link
Member Author

@annevk it's disappointing to me that the HTML spec editors are pushing back on an improvement to one of the APIs, requested by the de facto API owners, because we're not solving the problem for the entire HTML specification.

The feature detection that web authors must do in this case is unpleasant. They'll need to add asynchronous feature detection for all ImageBitmap options before initializing their libraries / apps / etc. Our working group has received multiple requests from web authors to improve the ability to feature detect in this area of the API. Would it help if they voiced their opinions on this pull request?

My concrete suggestion for generalizing our solution would be to add some new extended attribute to Web IDL - perhaps [ReifyDictionary] - which, for any dictionary to which it is applied, would generate both that dictionary, and an associated interface which can be constructed from that dictionary, in the ECMAScript binding. Essentially, the same transformation that has been done to ImageBitmapOptions in this pull request. I realize similar approaches have been considered before, but perhaps not this specific incarnation.

@annevk
Copy link
Member

annevk commented Nov 1, 2019

Given the longevity of web standards, I think having a pretty high bar for new primitives is good, especially when there are workarounds available.

I'd recommend contributing to whatwg/webidl#107. A number of implementers do seem interested in doing something and more slightly problematic cases might tip it in the direction of something getting done.

@kdashg
Copy link

kdashg commented Dec 4, 2019

I'm relatively satisfied that this is polyfillable now:
https://jsfiddle.net/ptkyewhx/

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 18, 2020
This is a prototype taking into account the feedback from
whatwg/html#4248 .

It basically adds a new interface to hold imagebitmapoptions
while preserving the same old functionality with an imagebitmapoptionsinit.

Bug: 983173
Change-Id: I622e10c892a552accd9c4b80c9f453ac9e10ced8
Base automatically changed from master to main January 15, 2021 07:57
@Kaiido
Copy link
Member

Kaiido commented Feb 1, 2021

I recently implemented a createImageBitmap monkey-patch and I did face this detection problem.
I don't believe there is any need for a special API here, the usual property-bag-trap should be enough.

IMHO, what could be done at the specs level (or maybe at Web IDL's?) is

  1. to enforce that all supported properties of any such property-bag are accessed synchronously
  2. that only supported properties are being accessed (might be harder to spec though...)

In current Chrome's implementation of the ImageBitmapOptions, all the properties are accessed synchronously, so the example in #4248 (comment) can even be rewritten as a synchronous method, which is actually very important since we need the source's buffer to be copied synchronously (think for instance about a webgl context without preserveDrawingBuffer, or even just a 2d context where something else is drawn right after that call to createImageBitmap()).

Regarding the second bullet, I think in particular about CSS's ScrollOptions case (which faces the same detection issue), where unfortunately we can't use the property-bag-trap for the behavior member because even though they don't allow the smooth behavior, some browsers (particularly Safari, but maybe others with some user-defined rules) will still try to access these members.

@annevk
Copy link
Member

annevk commented Feb 1, 2021

@Kaiido 1 follows from IDL. 2 relies on implementers not adding members without backing implementation and they have done that thus far.

@Kaiido
Copy link
Member

Kaiido commented Feb 1, 2021

1 follows from IDL.

Oh right I see it now in https://heycam.github.io/webidl/#es-dictionary So my point still stands that the property-bag-trap should be enough.

and they have done that thus far.

Unfortunately not all no, as I said, at least Safari will try to access the behavior member in Element.scrollTo({behavior: trap}), even though they actually don't support it https://jsfiddle.net/6cqdwfan/.
Maybe it should then read "For each dictionary member member declared on dictionary and supported by the implementation," but I guess this should rather go in an other ticket on Web IDL repo.

@annevk
Copy link
Member

annevk commented Feb 1, 2021

IDL is probably a better place, but the problem is deeper than that as they shouldn't have the IDL member in the code if it's not backed by an implementation. (I.e., the loop in IDL shouldn't be able to reach unsupported members.)

@kdashg
Copy link

kdashg commented Feb 2, 2021

I think the trap should be sufficient to feature-detect whether the key is supported, but if only a subset of all possible values are supported, we're still out of luck.

@kdashg
Copy link

kdashg commented Feb 2, 2021

Specifically, I would consider a browser that does a key lookup without supporting that key to be defective, and recommend filing a bug report with that browser.

@kenrussell
Copy link
Member Author

Given that there was no support for making this change, I'm closing this pull request.

@kenrussell kenrussell closed this Feb 2, 2021
@kenrussell kenrussell deleted the add-image-bitmap-options branch February 2, 2021 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: canvas
Development

Successfully merging this pull request may close these issues.

8 participants