Bug 179020 – [Touch Bar Web API] Add support for menuitem tag

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Aishwarya Nirmal]

The Touch Bar API will make use of the HTML tags for menu and menuitem. Currently WebKit does not support menuitem, so this changes adds support for that tag.

[Aishwarya Nirmal]

Created attachment 325362 [details]
Patch

[Build Bot]

Attachment 325362 [details] did not pass style-queue:


ERROR: Source/WebCore/JSHTMLMenuitemElement.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 325362 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325362&action=review

> Source/WebCore/ChangeLog:7
> +

I think it should be possible to write a simple test for this. I'll look for one to mimic.

> Source/WebCore/JSHTMLMenuitemElement.h:1
> +/*

This doesn't seem right. The generated file shouldn't be in the WebKit source tree, it should get generated into the build directory (and not show up in your patch). You may need help getting Xcode to do the right thing, it's not exactly straightforward.

[Tim Horton]

(In reply to Tim Horton from comment #3)
> Comment on attachment 325362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325362&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> I think it should be possible to write a simple test for this. I'll look for
> one to mimic.

Maybe something in LayoutTests/fast/html? There are a bunch that end in -element that seem like sensible starting points.

[Aishwarya Nirmal]

Created attachment 325364 [details]
Patch

[Aishwarya Nirmal]

(In reply to Tim Horton from comment #4)
> (In reply to Tim Horton from comment #3)
> > Comment on attachment 325362 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=325362&action=review
> > 
> > > Source/WebCore/ChangeLog:7
> > > +
> > 
> > I think it should be possible to write a simple test for this. I'll look for
> > one to mimic.
> 
> Maybe something in LayoutTests/fast/html? There are a bunch that end in
> -element that seem like sensible starting points.

Sounds good! I think I fixed the issue with the JSHTML file, so I'll look into creating some tests.

[Build Bot]

Comment on attachment 325364 [details]
Patch

Attachment 325364 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5042101

New failing tests:
imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html

[Build Bot]

Created attachment 325368 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325364 [details]
Patch

Attachment 325364 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5042110

New failing tests:
imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html

[Build Bot]

Created attachment 325370 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325364 [details]
Patch

Attachment 325364 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5042140

New failing tests:
imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html

[Build Bot]

Created attachment 325371 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325364 [details]
Patch

Attachment 325364 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5042152

New failing tests:
imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html

[Build Bot]

Created attachment 325373 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Ryosuke Niwa]

Comment on attachment 325364 [details]
Patch

r- since the patch is failing to build.

[Tim Horton]

(In reply to Ryosuke Niwa from comment #15)
> Comment on attachment 325364 [details]
> Patch
> 
> r- since the patch is failing to build.

It's not, the test is failing.

Aishwarya, you'll want to find out why imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical asserts that HTMLMenuItemElement *doesn't* exist :P

[Ryosuke Niwa]

(In reply to Tim Horton from comment #16)
> (In reply to Ryosuke Niwa from comment #15)
> > Comment on attachment 325364 [details]
> > Patch
> > 
> > r- since the patch is failing to build.
> 
> It's not, the test is failing.
> 
> Aishwarya, you'll want to find out why
> imported/w3c/web-platform-tests/html/semantics/interactive-elements/
> contextmenu-historical asserts that HTMLMenuItemElement *doesn't* exist :P

GTK+, Windows, and WPE seem to be failing to build? Have you confirmed that they build locally somehow?

[Aishwarya Nirmal]

It builds locally for me.

Looking at that file, imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical, it seems that there are some tests ensuring that HTMLMenuItemElement does not exist. We probably don't want to check for this anymore, so I can work on editing the test.

[Tim Horton]

(In reply to Aishwarya Nirmal from comment #18)
> It builds locally for me.

rniwa is right, something is up with the CMake build. I think it doesn't use DerivedSources.make, so maybe look for other files that mention an adjacent derived source to find out what mechanism it does use.

[Aishwarya Nirmal]

Created attachment 325391 [details]
Patch

[Build Bot]

Comment on attachment 325391 [details]
Patch

Attachment 325391 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5044601

New failing tests:
imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html

[Build Bot]

Created attachment 325398 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325391 [details]
Patch

Attachment 325391 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5044716

New failing tests:
imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html

[Build Bot]

Created attachment 325401 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325391 [details]
Patch

Attachment 325391 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5044751

New failing tests:
imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html

[Build Bot]

Created attachment 325404 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325391 [details]
Patch

Attachment 325391 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5044815

New failing tests:
imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html

[Build Bot]

Created attachment 325406 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Ryosuke Niwa]

The test is failing because it's expecting HTMLMenuItemElement to be not defined in the global scope. You have run "run-webkit-tests LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/contextmenu-historical.html --reset" to reset the expected results.

This is because the feature had been removed from the HTML specification in https://github.com/whatwg/html/pull/2742.

[Ryosuke Niwa]

Comment on attachment 325391 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325391&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

You need to describe what kind of API you're implementing and what code changes you're making for it.
r- due to the lack of change log descriptions and the test failures.

> Source/WebCore/ChangeLog:8
> +        * CMakeLists.txt:

Given the feature has been removed from the HTML specification, we should put this under a runtime flag.

[Aishwarya Nirmal]

Created attachment 325482 [details]
Patch

[Build Bot]

Comment on attachment 325482 [details]
Patch

Attachment 325482 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5054553

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/sandbox_032.htm

[Build Bot]

Created attachment 325501 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Aishwarya Nirmal]

The test that is failing seems to be unrelated to my changes. When I run run-webkit-tests LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/sandbox_032.htm --debug locally, the test passes.

[Ryosuke Niwa]

Comment on attachment 325482 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325482&action=review

> Source/WebCore/ChangeLog:12
> +        is set to false by default.  

Nits: There are two trailing spaces here.

> Source/WebCore/ChangeLog:13
> +

Please add some references to old HTML specifications like https://www.w3.org/TR/2013/WD-html51-20130528/interactive-elements.html.

> Source/WebKit/ChangeLog:8
> +        Adds in the MenuItemEnabled flag and default value. 

Nit: A trailing space at the end.

> Source/WebCore/html/HTMLMenuItemElement.idl:27
> +] interface HTMLMenuItemElement : HTMLElement {

So the idea is to implement more attributes of https://www.w3.org/TR/2013/WD-html51-20130528/interactive-elements.html in the future patches?

[Aishwarya Nirmal]

Yes! I will add those details to the change log.

[Aishwarya Nirmal]

Created attachment 325633 [details]
Patch

[Aishwarya Nirmal]

Created attachment 325635 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 325635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325635&action=review

> LayoutTests/fast/html/menuitem-element.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Please use <!DOCTYPE html>

> LayoutTests/fast/html/menuitem-element.html:12
> +

You either need to skip this test in TestExpectations or enable the feature using settings, etc...

[Aishwarya Nirmal]

Created attachment 325768 [details]
Patch

[Build Bot]

Comment on attachment 325768 [details]
Patch

Attachment 325768 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5080207

New failing tests:
fast/html/menuitem-element.html

[Build Bot]

Created attachment 325776 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325768 [details]
Patch

Attachment 325768 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5080229

New failing tests:
fast/html/menuitem-element.html

[Build Bot]

Created attachment 325778 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Aishwarya Nirmal]

Created attachment 325779 [details]
Patch

[Build Bot]

Comment on attachment 325779 [details]
Patch

Attachment 325779 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5081414

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting.https.html

[Build Bot]

Created attachment 325795 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Darin Adler]

Comment on attachment 325779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325779&action=review

> Source/WebCore/ChangeLog:16
> +        A specification for the menuitem element can be found at
> +        https://www.w3.org/TR/2013/WD-html51-20130528/interactive-elements.html#the-menuitem-element.
> +        More attributes of this element will be implemented in future patches.

Should we be adding this? This is obsolete. More recent HTML specifications do not include this. See <https://html.spec.whatwg.org/#non-conforming-features>.

[Aishwarya Nirmal]

Menuitem is noncorforming because Mozilla is the only implementor, but it does not seem to be entirely obsolete. The w3 folks seem to be willing to reconsider the removal of the menuitem if multiple browsers were to support it https://github.com/whatwg/html/issues/2929. 

We are aiming to use menuitem to represent the touch bar menu items for the touch bar web API. The API is experimental, so we are planning to use any information gathered to better inform a web standard for menuitem.

[Darin Adler]

(In reply to Aishwarya Nirmal from comment #49)
> Menuitem is noncorforming because Mozilla is the only implementor, but it
> does not seem to be entirely obsolete. The w3 folks seem to be willing to
> reconsider the removal of the menuitem if multiple browsers were to support
> it https://github.com/whatwg/html/issues/2929.

OK, makes sense.

[Ryosuke Niwa]

Comment on attachment 325779 [details]
Patch

r=me since this patch can't possibly cause the service worker test failure.

Sorry for the delay. In the future, please also set cq? flag so that the committer can set cq+ flag to land the patch via commit queue.

[WebKit Commit Bot]

Comment on attachment 325779 [details]
Patch

Clearing flags on attachment: 325779

Committed r224457: <https://trac.webkit.org/changeset/224457>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567759>