Skip to content

Remove tests for, and test the removal of, context menus#6167

Merged
sideshowbarker merged 4 commits intomasterfrom
contextmenu-historical
Jun 8, 2017
Merged

Remove tests for, and test the removal of, context menus#6167
sideshowbarker merged 4 commits intomasterfrom
contextmenu-historical

Conversation

@domenic
Copy link
Copy Markdown
Member

@domenic domenic commented Jun 6, 2017

Follows whatwg/html#2742.

Bugs to file:

  • Firefox to remove everything
  • Edge to remove menu.type
  • Chrome to remove onshow

@sideshowbarker, there are some conformance checker tests for menuitem, but I think they are testing invalid cases which stay invalid; would appreciate your help checking those.

@ghost
Copy link
Copy Markdown

ghost commented Jun 6, 2017

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision fabf1b3
Using browser at version BuildID 20170608100220; SourceStamp 7efda263a842e60cd0cc00b3c4a7058c65590702
Starting 10 test iterations

@ghost
Copy link
Copy Markdown

ghost commented Jun 6, 2017

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision fabf1b3
Using browser at version 10.0
Starting 10 test iterations

@ghost
Copy link
Copy Markdown

ghost commented Jun 6, 2017

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision fabf1b3
Using browser at version 60.0.3112.20 dev
Starting 10 test iterations

@ghost
Copy link
Copy Markdown

ghost commented Jun 6, 2017

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision fabf1b3
Using browser at version 14.14393
Starting 10 test iterations

assert_false("onshow" in location,
`${location.constructor.name} must not have a property "onshow"`);
}
assert_false("onshow" in Element.prototype, `Element.prototype must not contain an "onshow" property`);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this not in the array instead?

Maybe also test HTMLDocument.prototype.

assert_false("contextMenu" in HTMLElement.prototype,
"HTMLElement's prototype must not have a property \"contextmenu\"");
assert_false("contextMenu" in document.createElement("div"),
"A div must not have a property \"contextmenu\"");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Uppercase M in contextmenu

}, "menu.label must not be present");

test(() => {
console.log(document.querySelectorAll("menuitem:enabled"));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove


test(() => {
assert_equals(getComputedStyle(menu).display, "block");
}, "The user-agent stylesheet must type=\"context\" menus as block display");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This sentence is missing some word

}, "el.contextMenu must not be present");

test(() => {
assert_false("type" in menu);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also test that setting type does not set a content attribute (and similarly for the others).

@zcorpan
Copy link
Copy Markdown
Member

zcorpan commented Jun 8, 2017

I fixed the conformance checker tests and filed validator/validator#526

@domenic domenic force-pushed the contextmenu-historical branch from cffaafe to f63325d Compare June 8, 2017 18:17
@sideshowbarker
Copy link
Copy Markdown
Member

I fixed the conformance checker tests and filed validator/validator#526

thanks

@sideshowbarker sideshowbarker merged commit 242de2b into master Jun 8, 2017
@sideshowbarker sideshowbarker deleted the contextmenu-historical branch June 8, 2017 23:17
@jgraham
Copy link
Copy Markdown
Contributor

jgraham commented Jun 9, 2017

There doesn't seem to be any documentation about why this was merged despite failing the test jobs on all browsers?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants