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

Add HTMLOrSVGElement to HTML's IDL #10110

Merged
merged 1 commit into from Mar 23, 2018

Conversation

Projects
None yet
9 participants
@annevk
Copy link
Member

annevk commented Mar 20, 2018

And align a couple other things while there.

Helps with whatwg/html#3543.

Add HTMLOrSVGElement to HTML's IDL
And align a couple other things while there.

Helps with whatwg/html#3543.

@wpt-pr-bot wpt-pr-bot requested review from jensl and yuki3 Mar 20, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 20, 2018

Build ERRORED

Started: 2018-03-20 14:57:37
Finished: 2018-03-20 15:16:44

This report has been truncated because the number of unstable tests exceeds GitHub.com's character limit for comments (65536 characters).

Failing Jobs

  • firefox:nightly
  • chrome:dev

Unstable Browsers

Browser: "Firefox Nightly"

View in: WPT PR Status | TravisCI

@sideshowbarker sideshowbarker merged commit 20d739e into master Mar 23, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@sideshowbarker sideshowbarker deleted the annevk/htmlorsvgelement branch Mar 23, 2018

annevk added a commit to whatwg/html that referenced this pull request Mar 23, 2018

Add HTMLOrSVGElement interface mixin
This exposes dataset, tabIndex, focus(), and blur() on SVG elements.

It also does away with NoncedElement.

Tests: web-platform-tests/wpt#10110 and web-platform-tests/wpt#10149 (dataset tests for SVG already exist).

Fixes #3471 and fixes w3c/svgwg#60.
@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 23, 2018

@Hexcles pointed out that it seems like this has broken a bunch of tests, like this:
https://chromium-review.googlesource.com/c/chromium/src/+/978021/4/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/interfaces.any-expected.txt

So tests that include html.idl will now have to either define 'interface SVGElement {};' or import svg.idl.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 23, 2018

This kind of thing is going increasingly be a problem because the dependencies in spec IDL can be anything at all, and @lukebjerring is working on auto-updating from specs, and still the dependencies have to be managed manually in all of the tests including these files.

Other than fixing the immediate problem, ideas for how this is not going to keep happening?

@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Mar 23, 2018

The full list of breakages found by Blink's CI (there might be more):

  • IndexedDB/interfaces.any.html
  • IndexedDB/interfaces.any.worker.html
  • cookie-store/idlharness.tentative.html
  • dom/interfaces.html
  • hr-time/idlharness.html
  • html/dom/interfaces.html
  • webaudio/idlharness.https.html

https://crrev.com/c/978021

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 23, 2018

Apologies :/

One thing that would help is having more reviewers. If we can automate all things IDL that'd help too. It seems the only thing that's really manual is defining how to allocate objects. It seems there could be a better system for that. The tests could then be generated using global knowledge of all IDL.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 23, 2018

(We could also move SVGElement includes ... to svg.idl... Though that would presumably also require that setup in the specification as otherwise the automation might mess it up again.)

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 23, 2018

The most ambitious idea that @lukebjerring, @mdittmer and others have discussed would be precisely to eliminate the "defining how to allocate objects" manual bit. It'd be something like:

  • For each interface for which an instance can be found, write one or more factory methods for it.
  • All of the idlharness.js tests are automatically created, split either by spec with automatic dependency management, or split by interface, again with automatic dependency management.
@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 23, 2018

Yeah, what do we want to do here meanwhile? Comment out the SVGElement line?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 23, 2018

Yeah that'd be a good quickfix.

@dstorey

This comment has been minimized.

Copy link
Contributor

dstorey commented Mar 23, 2018

I feel it makes sense to add SVGElement includes to svg.idl and defined in the SVG spec. That was the change I was planning to make once the SVG PR to use the new mixin format is accepted. That way it is clearer in the SVG spec what members are included in the interface (specs are not really currently consistent if the implements/includes goes under the interface or the mixin. It is mostly the former except the DOM spec I think)

@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Mar 23, 2018

Are we going to change html.idl (& svg.idl) today?

Otherwise, I'll send out a PR to batch add add_untested_idls to the affected tests.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Mar 23, 2018

@Hexcles, that would be more like a proper fix, so since the list of affected tests isn't that long, if you could try that, it'd be better I think.

@inexorabletash

This comment has been minimized.

Copy link
Contributor

inexorabletash commented Mar 23, 2018

Aside: short of the "most ambitious" approach, a helper script that knows how to pull in HTML + DOM as untested and has declarations for their dangling dependencies (LinkStyle from CSSOM, etc) would simplify maintenance on many of those tests (e.g. IndexedDB, cookie-store, webaudio, hr-time).

@lukebjerring

This comment has been minimized.

Copy link
Contributor

lukebjerring commented Mar 23, 2018

With #10100 landed, and not trying to script things, the fix for making tests robust is using the { only: ['MyInterface'] } filtering for pulling in IDL interfaces (and deps) from the IDL files. That way, as interfaces are moved, the number of breakages is limited/mitigated.

I'd love to say that only (and except) are documented @ http://web-platform-tests.org/writing-tests/idlharness.html

... but that would be a lie.

annevk added a commit that referenced this pull request Mar 23, 2018

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 23, 2018

@dstorey that seems reasonable, but then we should remove it from the HTML Standard and make it a note there. For now I created #10160 to just comment this out, since it seems svg/interfaces.html hasn't really been converted to use mixins thus far and also duplicates some stuff (GlobalEventHandlers) so I'm not sure what the right strategy there would be.

chromium-wpt-export-bot added a commit that referenced this pull request Mar 23, 2018

Fix some IDL tests after html.idl includes SVGElement
html.idl changed in upstream
#10110
and imported in
https://crrev.com/c/978021

Bug: 825191
Change-Id: Ie1a04e730aabd50c615f1dab079f92eeaa22565a

chromium-wpt-export-bot added a commit that referenced this pull request Mar 23, 2018

Fix some IDL tests after html.idl includes SVGElement
html.idl changed in upstream
#10110
and imported in
https://crrev.com/c/978021

Bug: 825191
Change-Id: Ie1a04e730aabd50c615f1dab079f92eeaa22565a

aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 23, 2018

Fix some IDL tests after html.idl includes SVGElement
html.idl changed in upstream
web-platform-tests/wpt#10110
and imported in
https://crrev.com/c/978021

Bug: 825191
Change-Id: Ie1a04e730aabd50c615f1dab079f92eeaa22565a
Reviewed-on: https://chromium-review.googlesource.com/978508
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545588}

chromium-wpt-export-bot added a commit that referenced this pull request Mar 23, 2018

Fix some IDL tests after html.idl includes SVGElement
html.idl changed in upstream
#10110
and imported in
https://crrev.com/c/978021

Bug: 825191
Change-Id: Ie1a04e730aabd50c615f1dab079f92eeaa22565a
Reviewed-on: https://chromium-review.googlesource.com/978508
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545588}

chromium-wpt-export-bot added a commit that referenced this pull request Mar 23, 2018

Fix some IDL tests after html.idl includes SVGElement
html.idl changed in upstream
#10110
and imported in
https://crrev.com/c/978021

Bug: 825191
Change-Id: Ie1a04e730aabd50c615f1dab079f92eeaa22565a
Reviewed-on: https://chromium-review.googlesource.com/978508
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545588}

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 15, 2018

Bug 1448493 [wpt PR 10163] - Fix some IDL tests after html.idl includ…
…es SVGElement, a=testonly

Automatic update from web-platform-testsFix some IDL tests after html.idl includes SVGElement

html.idl changed in upstream
web-platform-tests/wpt#10110
and imported in
https://crrev.com/c/978021

Bug: 825191
Change-Id: Ie1a04e730aabd50c615f1dab079f92eeaa22565a
Reviewed-on: https://chromium-review.googlesource.com/978508
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545588}

wpt-commits: d04a8fc02b85bd32799691759c8c05ead07cd939
wpt-pr: 10163
wpt-commits: d04a8fc02b85bd32799691759c8c05ead07cd939
wpt-pr: 10163

mykmelez pushed a commit to mozilla/gecko that referenced this pull request Apr 16, 2018

Bug 1448493 [wpt PR 10163] - Fix some IDL tests after html.idl includ…
…es SVGElement, a=testonly

Automatic update from web-platform-testsFix some IDL tests after html.idl includes SVGElement

html.idl changed in upstream
web-platform-tests/wpt#10110
and imported in
https://crrev.com/c/978021

Bug: 825191
Change-Id: Ie1a04e730aabd50c615f1dab079f92eeaa22565a
Reviewed-on: https://chromium-review.googlesource.com/978508
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545588}

wpt-commits: d04a8fc02b85bd32799691759c8c05ead07cd939
wpt-pr: 10163
wpt-commits: d04a8fc02b85bd32799691759c8c05ead07cd939
wpt-pr: 10163

yutakahirano added a commit to yutakahirano/html that referenced this pull request Apr 26, 2018

Add HTMLOrSVGElement interface mixin
This exposes dataset, tabIndex, focus(), and blur() on SVG elements.

It also does away with NoncedElement.

Tests: web-platform-tests/wpt#10110 and web-platform-tests/wpt#10149 (dataset tests for SVG already exist).

Fixes whatwg#3471 and fixes w3c/svgwg#60.

alice added a commit to alice/html that referenced this pull request Jan 8, 2019

Add HTMLOrSVGElement interface mixin
This exposes dataset, tabIndex, focus(), and blur() on SVG elements.

It also does away with NoncedElement.

Tests: web-platform-tests/wpt#10110 and web-platform-tests/wpt#10149 (dataset tests for SVG already exist).

Fixes whatwg#3471 and fixes w3c/svgwg#60.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.