Bug 180513 – [Touch Bar Web API] Add support for custom Touch Bar buttons

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Aishwarya Nirmal]

Support adding custom buttons to the Touch Bar as part of the WebKit Touch Bar API.

[Aishwarya Nirmal]

Created attachment 328668 [details]
Patch

[Wenson Hsieh]

Comment on attachment 328668 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        No new tests.

I think we're at the point where we should be able to test end-to-end now...we should chat about a testing strategy for this. I could imagine either API tests that ask AppKit directly for touch bar state, or layout tests that involve plumbing touch bar state through UIScriptController in WebKitTestRunner.

> Source/WebCore/html/HTMLMenuElement.cpp:51
> +    for (unsigned i = 0; i < children->length(); i++) {

I think childrenOfType<HTMLMenuItemElement>() is the preferred way to iterate over menuitems in the subtree. We just have to make sure we don't dispatch the event while the iterator is still in scope, otherwise we'll hit a security assertion. We could do something like:

- iterate over childrenOfType
- bail if we find an appropriate menuitem
- then dispatch a click on the menuitem

> Source/WebCore/html/HTMLMenuElement.h:33
> +    WEBCORE_EXPORT void didInteractWithItem(const WTF::String& identifier);

I don't think we need the explicit WTF:: namespacing here.

> Source/WebKit/Shared/TouchBarMenuData.h:51
> +    Vector<TouchBarMenuItemData> items() const { return m_items; }

We should be able to keep this a const Vector& if we tweak the call site to be something like

auto& menuItems = touchBarMenuData.items();

Unless I'm missing something, I don't think this really needs to return a copy?

> Source/WebKit/Shared/TouchBarMenuData.h:57
> +    String identifier() const { return m_id; }

Let's rename identifier => title, to reflect the change in attribute.

> Source/WebKit/Shared/TouchBarMenuData.h:-59
> -    

Nit - stray whitespace change.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:650
> +    [[NSNotificationCenter defaultCenter] removeObserver:self];

Hm...it's not obvious to me why we need to remove ourselves as an observer. Does this object start listening for notifications elsewhere in this patch?

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:670
> +    return item;

This should probably return an -autorelease object, since the name (-itemForIdentifier:) doesn't reference "new" or "create".

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:971
> +        touchBar = m_customizedTouchBar.get();

It's a bit weird to refer to this page-controlled touch bar as the "customized" touch bar, since there's a notion of touch bar "customization" already (if you go into View > Customize Touch Bar, you can rearrange or add or remove touch bar items) and it sounds confusing to mix the terms...

I think the "pageCustomizedTouchBar" terminology you use elsewhere in this patch makes this distinction clearer. Can we rename m_customizedTouchBar to m_pageCustomizedTouchBar to be consistent?

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1107
> +    NSMutableSet<NSTouchBarItem *> *templateItems = [[NSMutableSet alloc] init];

adoptNS() here to fix the leak!

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1108
> +    NSMutableArray<NSTouchBarItemIdentifier> *defaultItems = [[NSMutableArray alloc] init];

Ditto.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1112
> +        NSCustomTouchBarItem *touchBarItem = [[NSCustomTouchBarItem alloc] initWithIdentifier:item.identifier];

adoptNS() to fix the leak!

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5411
> +        if (auto* element = frame->document()->currentMenuElement().get())

We can remove the `.get()` at the end if we just make this `auto element`.

[Darin Adler]

Comment on attachment 328668 [details]
Patch

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

> Source/WebKit/UIProcess/mac/PageClientImplMac.h:107
> +    void touchBarMenuDataDidChange() override;

To get this compiling on iOS either:

1) We need a line like this in the iOS version in PageClientImplIOS.h, or
2) We need the function in the base class to not be pure virtual, or
3) We need to put it more of the code in a HAVE(TOUCH_BAR) or PLATFORM(MAC) conditional instead of PLATFORM(COCOA).

Or something else like that.

[Aishwarya Nirmal]

Created attachment 329235 [details]
Patch

[Aishwarya Nirmal]

Created attachment 329255 [details]
Patch

[Aishwarya Nirmal]

Created attachment 329257 [details]
Patch

[Aishwarya Nirmal]

Created attachment 329293 [details]
Patch

[Wenson Hsieh]

Comment on attachment 329293 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:648
> +- (void)didDestroyView

It doesn't look like anything calls -[WKPageCustomizedTouchBarItemController didDestroyView] here. It looks like we should call this in WebViewImpl::~WebViewImpl (just like we do for the text touch bar item controller) so we ensure that WKPageCustomizedTouchBarItemController's pointer to _webViewImpl is invalidated when our WebViewImpl goes away.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:653
> +- (void)touchBarMenuItemTapped:(WKPageCustomizedTouchBarButton *)button

Since _webViewImpl may be null if the WebViewImpl is destroyed, we probably need to check if (!_webViewImpl) and bail early here.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1108
> +    for (TouchBarMenuItemData& item : items) {

Nit - instead of having the const TouchBarMenuData* touchBarMenuData up there, this would probably read better as:

for (auto& item : m_page->touchBarMenuData().items()) {
    …
}

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:33
> +

Nit - I don't think we need another PLATFORM(COCOA) here.

[Aishwarya Nirmal]

Created attachment 329366 [details]
Patch

[Wenson Hsieh]

Comment on attachment 329366 [details]
Patch

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

> Source/WebKit/Shared/WebPreferences.yaml:644
> +  defaultValue: true

I don't think we'll want to enable <menuitem> by default quite yet!

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:667
> +- (nullable NSTouchBarItem *)itemForIdentifier:(NSString *)identifier

Hm...is overriding -itemForIdentifier: here needed? The idea is that AppKit internally maintains a cache of touch bar items, and the superclass (NSTouchBar's) implementation of -itemForIdentifier: either returns a cached touch bar item (if any is found) or, if necessary, invokes -makeItemForIdentifier: to create a new touch bar item (which it then adds to the item cache).

[Aishwarya Nirmal]

Created attachment 329380 [details]
Patch

[Wenson Hsieh]

Comment on attachment 329380 [details]
Patch

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

This mostly looks good! There are still a few important issues, but core functionality is there and tested — let's add a couple of FIXMEs.

> Source/WebCore/dom/Document.cpp:1017
> +    m_currentMenuElement = element;

Nothing in this patch seems to clear out the current menu element (i.e. setCurrentMenuElement(nullptr)). Can we note this in a FIXME?

We probably should clear this out when a menu element is disconnected from the document, if the disconnected menu element is the current menu element.

> Source/WebCore/html/HTMLMenuElement.cpp:52
> +        if (item.attributeWithoutSynchronization(titleAttr) == identifier) {

I don't think checking against the button title will work if there are multiple buttons with the same title...can we note this in a FIXME?

I think we should probably assign a UUID for each menu element, and propagate that over IPC to the UI process and back instead.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:667
> +- (nullable NSTouchBarItem *)itemForIdentifier:(NSString *)identifier

We won't need to override this method if we're just calling super.

[Darin Adler]

Comment on attachment 329380 [details]
Patch

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

This patch looks really good. There is more work that might be needed to get this completely solid.

Likely the best analogy for the global "current menu element" is the current title element, the one that determines the document’s title. Having clear rules about which menu element takes effect if there is more than one and what happens is menu elements are added or removed can easily become important for cross-browser interoperability, like they did for the title element.

It also seems wrong to call the menu element for the touch bar just the "current menu element". It’s an important menu element, the one with a menu that should be seen, but there could also (some day) be a menu element for the current right mouse click, or something like that. So "current menu element" sounds not *quite* right as the name for the concept we are implementing, but maybe I am wrong about that.

The pattern for the title element is that the element itself just notifies the document and it’s the document that has to adjudicate which one is the effective title element. Thus the title element itself doesn’t do things like directly notifying a client that it exists, since that’s unwanted for an extra title element that is not the one in effect.

Sadly, the title element code is a bit complicated because of special cases for SVG, so we wouldn’t want to replicate all of its code, but the general pattern is probably right.

>> Source/WebCore/dom/Document.cpp:1017
>> +    m_currentMenuElement = element;
> 
> Nothing in this patch seems to clear out the current menu element (i.e. setCurrentMenuElement(nullptr)). Can we note this in a FIXME?
> 
> We probably should clear this out when a menu element is disconnected from the document, if the disconnected menu element is the current menu element.

The most likely correct pattern for object lifetimes would be to match the other RefPtr<Element> type data members in Document and add code to null it out in Document::removedLastRef. This is important to avoid a possible massive storage leak.

Separately, it’s also likely critical for correctness reasons to null it out when disconnecting from the document as Wenson says. So for that a FIXME, might be sufficient. I think what we need most for that is a test case.

> Source/WebCore/dom/Document.h:481
> +    void setCurrentMenuElement(RefPtr<HTMLMenuElement>&&);
> +    RefPtr<HTMLMenuElement>& currentMenuElement() { return m_currentMenuElement; }

Given that setCurrentMenuElement is only called when m_isTouchBarMenu is true, this is not a "current menu element" at all. It’s a "current touch bar menu".

The argument to setCurrentMenuElement does not need to be a RefPtr&& unless callers are going to pass in a newly created object or need to hand over ownership for some reason. I suggest HTMLMenuElement& or HTMLMenuElement* for the argument type.

It’s peculiar to have both a set function and also a function that returns a non-const reference. Those two idioms are both ways to allow someone to set and we don’t need both. I suggest changing currentMenuElement() to be a const member function that returns HTMLMenuElement*, or if I am out of date and it’s important to return RefPtr for our future design for security, then it should be a const member function that returns RefPtr<HTMLMenuElement> rather than a non-const member function that returns RefPtr<HTMLMenuElement>&.

These two new functions should *not* be paragraphed in with adoptNode, which is a very different type of function with a more generic purpose. They should be their own paragraph or be grouped with functions that similarly track a document-wide special element.

> Source/WebCore/html/HTMLMenuElement.cpp:46
> +void HTMLMenuElement::didInteractWithItem(const String& identifier)

I think this function should be called clickItem or something like that. Doesn’t seem to require the "did" naming.

But given Wenson’s comment about UUIDs this may be changing significantly anyway.

> Source/WebCore/html/HTMLMenuElement.cpp:61
> +    Ref<MouseEvent> mouseEvent = MouseEvent::create(eventNames().clickEvent, document().defaultView(), platformEvent, 0, 0);
> +    tappedItem->dispatchEvent(mouseEvent);

I suggest combining these two into a single line.

> Source/WebCore/html/HTMLMenuElement.cpp:68
> +        document().setCurrentMenuElement(this);

This is a peculiar rule. It means that whenever we insert a new HTMLMenuElement that is a touch bar menu, then it will displace the already existing one. It’s a "last one inserted wins" rule. We should be sure that’s the rule we want. Arbitrary rules like this often lead to cross-browser incompatibility so it’s typically best to come up with an explicit rule.

> Source/WebCore/html/HTMLMenuElement.cpp:70
>          if (auto* page = document().page())
>              page->chrome().client().didInsertMenuElement(*this);

Is it a good idea to call the chrome client every time we insert a touch bar menu element? What do we expect clients to do about multiple touch bar menu elements? I think maybe we actually want to call the client when the current menu element changes; could be a Document thing instead of an HTMLMenuElement thing. And also, seems not quite right to do this only for touch bar menus, yet have the client function be named generally "did insert menu element". Name and semantics should match.

> Source/WebCore/html/HTMLMenuElement.h:33
> +    WEBCORE_EXPORT void didInteractWithItem(const WTF::String& identifier);

No need for the "WTF::" prefix here.

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1109
> +        RetainPtr<NSCustomTouchBarItem> touchBarItem = adoptNS([[NSCustomTouchBarItem alloc] initWithIdentifier:item.title]);

Can use "auto" here.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5413
> +    if (auto* frame = mainFrame()) {
> +        if (auto element = frame->document()->currentMenuElement())
> +            element->didInteractWithItem(touchBarMenuItemData.title);
> +    }

Seems to me this should be calling a function on the Document, instead of explicitly getting at the current menu element. This function we would add on Document would replace the Document::currentMenuElement function entirely.

[Anne van Kesteren]

Closing as this is not being worked on and would probably warrant a novel approach if started today.