Bug 179785 – [Cocoa] First pass at implementing alternative presentation button element

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

[Daniel Bates]

First pass at implementing alternative presentation button element.

Part of <rdar://problem/34917108>

[Daniel Bates]

Created attachment 327094 [details]
Patch and tests

[Daniel Bates]

Created attachment 327205 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/editing/cocoa/AlternativePresentationButtonSubstitution.cpp:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 56 files


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

[Daniel Bates]

Created attachment 327212 [details]
Patch and tests

[EWS Watchlist]

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


ERROR: Source/WebCore/editing/cocoa/AlternativePresentationButtonSubstitution.cpp:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 56 files


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

[Daniel Bates]

Created attachment 327214 [details]
Patch and tests

[EWS Watchlist]

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


ERROR: Source/WebCore/editing/cocoa/AlternativePresentationButtonSubstitution.cpp:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 56 files


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

[EWS Watchlist]

Comment on attachment 327214 [details]
Patch and tests

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

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

Created attachment 327225 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[EWS Watchlist]

Comment on attachment 327214 [details]
Patch and tests

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

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

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

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

[Daniel Bates]

Created attachment 327239 [details]
Patch and tests

[EWS Watchlist]

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


ERROR: Source/WebCore/editing/cocoa/AlternativePresentationButtonSubstitution.cpp:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 56 files


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

[EWS Watchlist]

Comment on attachment 327239 [details]
Patch and tests

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

New failing tests:
fast/forms/alternative-presentation-button/replacement.html

[EWS Watchlist]

Created attachment 327276 [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

[Daniel Bates]

Created attachment 327279 [details]
Patch and tests

[EWS Watchlist]

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


ERROR: Source/WebCore/editing/cocoa/AlternativePresentationButtonSubstitution.cpp:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 57 files


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

[Ryosuke Niwa]

Comment on attachment 327279 [details]
Patch and tests

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

> Source/WebCore/editing/Editor.cpp:3834
> +    Ref<Element> firstElement = WTFMove(elements[0]);

I think it's better to call this variable elementForAlternativePresentation.
The fact it's the fist element is more of an implementation detail, not so much the semantics of what this element is.

> Source/WebCore/editing/Editor.h:600
> +    HashMap<Element*, std::unique_ptr<AlternativePresentationButtonSubstitution>> m_alternativePresentationButtonElementToSubstitutionMap;
> +    HashMap<String, Element*> m_alternativePresentationButtonIdentifierToElementMap;

What guarantees that these raw pointers to Element would always be cleared before they're deleted?

> Source/WebCore/editing/cocoa/AlternativePresentationButtonSubstitution.cpp:90
> +    // 1. Save appearance

I don't think these comments are adding any values. Please remove.

> Source/WebCore/editing/cocoa/AlternativePresentationButtonSubstitution.cpp:95
> +    // 2. Apply substitution

Ditto.

> Source/WebCore/html/HTMLInputElement.cpp:508
> +inline std::unique_ptr<InputType> HTMLInputElement::createInputType(const AtomicString& type)
> +{
> +#if ENABLE(ALTERNATIVE_PRESENTATION_BUTTON_ELEMENT)
> +    if (type == InputTypeNames::alternativePresentationButton())
> +        return std::make_unique<AlternativePresentationButtonInputType>(*this);
> +#endif
> +    return InputType::create(*this, type);
> +}

Why can't alternativePresentationButton just use InputType::create?
Adding more & more abstractions around input type is not great because input type is already a frequent source of security bugs.
It would make it much harder to reason what's happening.

> Source/WebCore/html/HTMLInputElement.cpp:721
> -        updateType();
> +        updateType(value);

What happens if the author script changes type while the presentation is being substituted?

> Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonElement.cpp:121
> +void AlternativePresentationButtonElement::defaultEventHandler(Event& event)
> +{

Can an user trigger this element purely with keyboards? i.e. What's the accessibility story?

> Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonElement.h:37
> +class AlternativePresentationButtonElement final : public HTMLDivElement {

There is no point ing making this inherit from HTMLDivElement. Just make it inherit from HTMLElement directly.

> Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonInputType.h:43
> +    // InputType

This comment serves no purpose. Please remove.

> Source/WebKit/UIProcess/API/C/WKPageUIClient.h:1033
> +    // Version 11.
> +    WKPageDidClickAlternativePresentationButtonCallback                 didClickAlternativePresentationButton;

Why are we adding a new C API?? Can't we add it to modern objective-C API?
Or are you doing that for testing purposes?

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #18)
> > Source/WebCore/editing/Editor.cpp:3834
> > +    Ref<Element> firstElement = WTFMove(elements[0]);
> 
> I think it's better to call this variable elementForAlternativePresentation.
> The fact it's the fist element is more of an implementation detail, not so
> much the semantics of what this element is.
> 

Will rename.

> > Source/WebCore/editing/Editor.h:600
> > +    HashMap<Element*, std::unique_ptr<AlternativePresentationButtonSubstitution>> m_alternativePresentationButtonElementToSubstitutionMap;
> > +    HashMap<String, Element*> m_alternativePresentationButtonIdentifierToElementMap;
> 
> What guarantees that these raw pointers to Element would always be cleared
> before they're deleted?
>

Notice that these maps only hold pointers to AlternativePresentationButtonElement elements. These pointers are guaranteed to be valid as long as they are in the maps because these pointers are added and removed from the hash maps whenever an AlternativePresentationButtonElement element is inserted into- (AlternativePresentationButtonElement::insertedIntoAncestor()) and removed from- the document (AlternativePresentationButtonElement::removedFromAncestor()), respectively.

Will change Element* to std::reference_wrapper<AlternativePresentationButtonElement> to make it clear that these hash maps hold references to AlternativePresentationButtonElement elements. That is, they do not hold references to arbitrary elements.

> > Source/WebCore/editing/cocoa/AlternativePresentationButtonSubstitution.cpp:90
> > +    // 1. Save appearance
> 
> I don't think these comments are adding any values. Please remove.
> 

Will remove.


> > Source/WebCore/editing/cocoa/AlternativePresentationButtonSubstitution.cpp:95
> > +    // 2. Apply substitution
> 
> Ditto.
> 

Will remove.

> > Source/WebCore/html/HTMLInputElement.cpp:508
> > +inline std::unique_ptr<InputType> HTMLInputElement::createInputType(const AtomicString& type)
> > +{
> > +#if ENABLE(ALTERNATIVE_PRESENTATION_BUTTON_ELEMENT)
> > +    if (type == InputTypeNames::alternativePresentationButton())
> > +        return std::make_unique<AlternativePresentationButtonInputType>(*this);
> > +#endif
> > +    return InputType::create(*this, type);
> > +}
> 
> Why can't alternativePresentationButton just use InputType::create?

We can teach InputType::create() to instantiate AlternativePresentationButtonInputType so long as we also teach it to instantiate this type only when triggered by SPI or the Internals JavaScript object (for testing purposes) (*). That is, I do not want to expose this input type to the web (at least at this time). When I wrote this patch I was unclear whether we will want to have a dedicated input type for the Alternative Presentation Button and hence chose to implement these usage restrictions by creating a helper function that knew about the AlternativePresentationButtonInputType type. Let me know if it would be preferred to take the approach listed at the end of the remarks below or if you have other ideas. 

Additional remarks:

Currently the logic in <https://trac.webkit.org/browser/trunk/Source/WebCore/html/InputType.cpp?rev=225177> only supports conditioning input types on whether a runtime feature is enabled when it builds the input type factory map: <https://trac.webkit.org/browser/trunk/Source/WebCore/html/InputType.cpp?rev=225177#L146>. And this map is built once per WebProcess process when we process the first <input> on some page loaded in the process by <https://trac.webkit.org/browser/trunk/Source/WebCore/html/InputType.cpp?rev=225177#L155> and <https://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp?rev=225177#L485> OR <https://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.cpp?rev=225177#L661>. One idea is to add state to HTMLInputElement to track whether the SPI/Internals function was invoked to use the Alternative Presentation Button and have InputType::create() query for this state before instantiating and returning a AlternativePresentationButtonInputType object.

> Adding more & more abstractions around input type is not great because input
> type is already a frequent source of security bugs.
> It would make it much harder to reason what's happening.
> 
> > Source/WebCore/html/HTMLInputElement.cpp:721
> > -        updateType();
> > +        updateType(value);
> 
> What happens if the author script changes type while the presentation is
> being substituted?
> 

Do you foresee an issue? If so, please provide an example.

This will behave just as any other input type change and the shadow subtree for the old input type (AlternativePresentationButtonInputType) will be destroyed before creating the new input type and attaching its shadow subtree.

> > Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonElement.cpp:121
> > +void AlternativePresentationButtonElement::defaultEventHandler(Event& event)
> > +{
> 
> Can an user trigger this element purely with keyboards?

Yes, using accessibility commands (VoiceOver on Mac/iOS). We should take a similar approach as HTMLButtonElement and support key events for '\r' and ' '. Unless you feel strongly that such support should be part of this patch, I will do this work in a follow up bug.

> i.e. What's the accessibility story?
> 

We want this button to be as accessible as any other button. When the button is created it adds the appropriate ARIA attributes to itself so as to register as a button type with the accessibility machinery and its accessibility description is a localized string.

> > Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonElement.h:37
> > +class AlternativePresentationButtonElement final : public HTMLDivElement {
> 
> There is no point ing making this inherit from HTMLDivElement. Just make it
> inherit from HTMLElement directly.
> 

Notice that to instantiate an HTMLElement you need a tag name. And I did not add a new tag name for this element. What tag name do you suggest I use?

> > Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonInputType.h:43
> > +    // InputType
> 
> This comment serves no purpose. Please remove.
> 

I find this comment helpful to demarcate the overrides of an interface and we have similar comments throughout the codebase, including XMLHttpRequest [1], RenderLayer [2] and ApplePayValidateMerchantEvent [3].

[1] <https://trac.webkit.org/browser/trunk/Source/WebCore/xml/XMLHttpRequest.h?rev=225177#L122>
[2] <https://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderLayer.h?rev=225177#L233>
[3] <https://trac.webkit.org/browser/trunk/Source/WebCore/Modules/applepay/ApplePayValidateMerchantEvent.h?rev=225177#L49>.

> > Source/WebKit/UIProcess/API/C/WKPageUIClient.h:1033
> > +    // Version 11.
> > +    WKPageDidClickAlternativePresentationButtonCallback                 didClickAlternativePresentationButton;
> 
> Why are we adding a new C API?? 

At the time of writing Safari on Mac uses the C API. And we want to make use of this SPI in Safari on Mac.

> Can't we add it to modern objective-C API? Or are you doing that for testing purposes?

Notice that this patch adds an equivalent modern Objective-C SPI in file WKUIDelegatePrivate.h.

[Daniel Bates]

Created attachment 327690 [details]
Patch and layout tests

Updated patch based on feedback from Ryosuke Niwa. Also added some accessibility tests. See LayoutTests change log description for more details.

[EWS Watchlist]

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


ERROR: Source/WebCore/editing/cocoa/AlternativePresentationButtonSubstitution.cpp:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 61 files


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

[Brent Fulgham]

Comment on attachment 327690 [details]
Patch and layout tests

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

r=me

> Source/WebCore/editing/Editor.cpp:3841
> +        // FIXME: This substitution is only safe to do if and only if firstElement can support a user-agent

Can you please provide a Bugzilla/Radar for this follow-up work?

> Source/WebCore/html/HTMLInputElement.h:151
> +    WEBCORE_EXPORT HTMLElement* alternativePresentationButtonElement() const;

We need to make sure this doesn't get stored anywhere. I know this is the first part of this set of patches, so we don't have to address it here, but we need a mechanism where we can avoid this pointer being stored someplace.

Could this be an Optional reference?

> Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonElement.cpp:100
> +    iconContainer->appendChild(icon);

This could probably be an WTFMove(icon). But not necessary to make this change.

> Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonElement.cpp:110
> +    textContainer->appendChild(text);

Ditto WTFMove(text)

> Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonElement.cpp:115
> +    textContainer->appendChild(subtext);

Ditto WTFMove(subtext).

[Daniel Bates]

(In reply to Brent Fulgham from comment #22)
> 
> > Source/WebCore/editing/Editor.cpp:3841
> > +        // FIXME: This substitution is only safe to do if and only if firstElement can support a user-agent
> 
> Can you please provide a Bugzilla/Radar for this follow-up work?
> 

Filed bug #180086. Will update comment to reference this bug as well as substitute elementForAlternativePresentation for firstElement or otherwise make this comment read well.

> > Source/WebCore/html/HTMLInputElement.h:151
> > +    WEBCORE_EXPORT HTMLElement* alternativePresentationButtonElement() const;
> 
> We need to make sure this doesn't get stored anywhere. I know this is the
> first part of this set of patches, so we don't have to address it here, but
> we need a mechanism where we can avoid this pointer being stored someplace.
> 
> Could this be an Optional reference?
> 

I am unclear how this would help solve the problem you described above.

> > Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonElement.cpp:100
> > +    iconContainer->appendChild(icon);
> 
> This could probably be an WTFMove(icon). But not necessary to make this
> change.
> 
> > Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonElement.cpp:110
> > +    textContainer->appendChild(text);
> 
> Ditto WTFMove(text)
> 

We cannot move this value because Node::appendChild() takes a non-const lvalue reference as an argument and hence does not bind to an rvalue reference. We could look to support passing an rvalue reference to Node::appendChild() as well as other DOM operations. I suggest that we do this in a separate bug.

> > Source/WebCore/html/shadow/cocoa/AlternativePresentationButtonElement.cpp:115
> > +    textContainer->appendChild(subtext);
> 
> Ditto WTFMove(subtext).

See my remark above.

[Daniel Bates]

(In reply to Daniel Bates from comment #19)
> > > Source/WebCore/editing/Editor.h:600
> > > +    HashMap<Element*, std::unique_ptr<AlternativePresentationButtonSubstitution>> m_alternativePresentationButtonElementToSubstitutionMap;
> > > +    HashMap<String, Element*> m_alternativePresentationButtonIdentifierToElementMap;
> > 
> > What guarantees that these raw pointers to Element would always be cleared
> > before they're deleted?
> >
> 
> [...]
> Will change Element* to
> std::reference_wrapper<AlternativePresentationButtonElement> to make it
> clear that these hash maps hold references to
> AlternativePresentationButtonElement elements. That is, they do not hold
> references to arbitrary elements.
> 

Actually we need to add traits to support storing std::reference_wrapper objects in HashMap. Filed bug #180091. For now, I will change Element* to AlternativePresentationButtonElement*.

[Daniel Bates]

Committed r225223: <https://trac.webkit.org/changeset/225223>

[Matt Lewis]

The test fast/forms/alternative-presentation-button/replacement.html added in this revision is consistently failing on Sierra and High Sierra

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r225224%20(5987)/results.html

diff:
--- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/fast/forms/alternative-presentation-button/replacement-expected.txt
+++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/fast/forms/alternative-presentation-button/replacement-actual.txt
@@ -12,9 +12,9 @@
                   RenderBlock {DIV} at (0,0) size 205x0
                 RenderBlock {DIV} at (0,0) size 205x26
                   RenderBlock (anonymous) at (0,0) size 205x13
-                    RenderInline {SPAN} at (0,0) size 185x13
-                      RenderText {#text} at (0,0) size 185x13
-                        text run at (0,0) width 185: "alternative presentation button title"
+                    RenderInline {SPAN} at (0,0) size 186x13
+                      RenderText {#text} at (0,0) size 186x13
+                        text run at (0,0) width 186: "alternative presentation button title"
                   RenderBlock {DIV} at (0,13) size 205x13
                     RenderText {#text} at (0,0) size 205x13
                       text run at (0,0) width 205: "alternative presentation button subtitle"

[Daniel Bates]

(In reply to Matt Lewis from comment #26)
> The test fast/forms/alternative-presentation-button/replacement.html added
> in this revision is consistently failing on Sierra and High Sierra
> 
> https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/
> r225224%20(5987)/results.html
> 
> diff:
> ---
> /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/fast/
> forms/alternative-presentation-button/replacement-expected.txt
> +++
> /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/fast/
> forms/alternative-presentation-button/replacement-actual.txt
> @@ -12,9 +12,9 @@
>                    RenderBlock {DIV} at (0,0) size 205x0
>                  RenderBlock {DIV} at (0,0) size 205x26
>                    RenderBlock (anonymous) at (0,0) size 205x13
> -                    RenderInline {SPAN} at (0,0) size 185x13
> -                      RenderText {#text} at (0,0) size 185x13
> -                        text run at (0,0) width 185: "alternative
> presentation button title"
> +                    RenderInline {SPAN} at (0,0) size 186x13
> +                      RenderText {#text} at (0,0) size 186x13
> +                        text run at (0,0) width 186: "alternative
> presentation button title"
>                    RenderBlock {DIV} at (0,13) size 205x13
>                      RenderText {#text} at (0,0) size 205x13
>                        text run at (0,0) width 205: "alternative
> presentation button subtitle"

Committed updated result in <https://trac.webkit.org/changeset/225237>.