Bug 167579 – [GTK] WTR: Native HTML form validation popover is not supported

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

This is causing several tests to timeout in the bots:

fast/forms/validation-bubble-disappears-when-input-detached.html
fast/forms/validation-bubble-disappears-when-input-moved.html
fast/forms/validation-bubble-disappears-when-input-no-longer-visible.html
fast/forms/validation-custom-message.html
fast/forms/validation-message-detached-iframe.html
fast/forms/validation-message-detached-iframe2.html
fast/forms/validation-messages.html

I don't know if we really want to implement them or not, I guess it looks better using a GtkPopover instead of the Shadow DOM based UI, and it's easy to implement. For now I'll just skip these tests.

[Carlos Garcia Campos]

Skipped in r211359

[Miguel Gomez]

Four new tests have started timing out since r238038

fast/forms/validation-message-on-checkbox.html [ Timeout ]
fast/forms/validation-message-on-listbox.html [ Timeout ]
fast/forms/validation-message-on-menulist.html [ Timeout ]
fast/forms/validation-message-on-radio.html [ Timeout ]

[ChangSeok Oh]

Even after closing bug 234629, tests related to the validation bubble would keep failing due to missing JS APIs for the test runner (e.g., UIController.contentsOfUserInterfaceItem). We may want to add those interfaces here.

[ChangSeok Oh]

Created attachment 449488 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[ChangSeok Oh]

Hrm... Why do mac/ios ports complain about a changeset for the GTK port? -_-a

[Carlos Garcia Campos]

Comment on attachment 449488 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        This change enables form-validation-related tests. To this end, two APIs called
> +        overridePreference and contentsOfUserInterfaceItem are added to UIScriptController.
> +        The two APIs need some private interface of JavaScriptCore so we suppress
> +        compiler complains that happen when accessing JSC headers directly in the test runner.

I think we can just use the C API from WTR instead of the glib one.

> Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:70
> +void WKViewSetMinimumFontSize(WKViewRef viewRef, double fontSize)
> +{
> +    webkitWebViewBaseSetMinimumFontSize(toImpl(viewRef), fontSize);
> +}

WKPreferences is exposed in the C API, I could just add WKPreferencesSetMinimumFontSize() instead

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1649
> +    if (!g_strcmp0(userInterfaceItem, "validationBubble")) {

I would add an early return instead.

[ChangSeok Oh]

Comment on attachment 449488 [details]
Patch

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

Thanks for your review. I am not sure if I effectively addressed your comments. Please have a look at the new patch and let me know if it needs more improvement. We can take another cycle.

>> Source/JavaScriptCore/ChangeLog:11
>> +        compiler complains that happen when accessing JSC headers directly in the test runner.
> 
> I think we can just use the C API from WTR instead of the glib one.

Sorry, I am not sure if I understand correctly. Are you talking about the dictionary implementation in WebKitWebViewBase.cpp (i.e., webkitWebViewBaseContentsOfUserInterfaceItem)?

>> Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:70
>> +}
> 
> WKPreferences is exposed in the C API, I could just add WKPreferencesSetMinimumFontSize() instead

O.K. I get the preference by using these APIs, WKPageGroupGetPreferences(WKPageGetPageGroup(WKViewGetPage(viewRef))). Do you think this is the most efficient way to get the WKPreferences here? If you know a better way, please let me know.

>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1649
>> +    if (!g_strcmp0(userInterfaceItem, "validationBubble")) {
> 
> I would add an early return instead.

Done.

[ChangSeok Oh]

Created attachment 449635 [details]
Patch

[Carlos Garcia Campos]

(In reply to ChangSeok Oh from comment #8)
> Comment on attachment 449488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449488&action=review
> 
> Thanks for your review. I am not sure if I effectively addressed your
> comments. Please have a look at the new patch and let me know if it needs
> more improvement. We can take another cycle.
> 
> >> Source/JavaScriptCore/ChangeLog:11
> >> +        compiler complains that happen when accessing JSC headers directly in the test runner.
> > 
> > I think we can just use the C API from WTR instead of the glib one.
> 
> Sorry, I am not sure if I understand correctly. Are you talking about the
> dictionary implementation in WebKitWebViewBase.cpp (i.e.,
> webkitWebViewBaseContentsOfUserInterfaceItem)?

No, I mean we can use the JSC C API, that is already used by WTR, instead of the JSC glib API and then we don't need the changes in jsc headers.

> >> Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:70
> >> +}
> > 
> > WKPreferences is exposed in the C API, I could just add WKPreferencesSetMinimumFontSize() instead
> 
> O.K. I get the preference by using these APIs,
> WKPageGroupGetPreferences(WKPageGetPageGroup(WKViewGetPage(viewRef))). Do
> you think this is the most efficient way to get the WKPreferences here? If
> you know a better way, please let me know.

That's fine, you can also use TestController::platformPreferences().

> >> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1649
> >> +    if (!g_strcmp0(userInterfaceItem, "validationBubble")) {
> > 
> > I would add an early return instead.
> 
> Done.

[Carlos Garcia Campos]

Comment on attachment 449635 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:72
> +void WKViewSetMinimumFontSize(WKViewRef viewRef, double fontSize)
> +{
> +    auto preferences = WKPageGroupGetPreferences(WKPageGetPageGroup(WKViewGetPage(viewRef)));
> +    WKPreferencesSetMinimumFontSize(preferences, static_cast<uint32_t>(fontSize));
> +}

We don't need to expose WKViewSetMinimumFontSize, WTR can use WKPreferencesSetMinimumFontSize directly.

[ChangSeok Oh]

Created attachment 449703 [details]
Patch

[ChangSeok Oh]

Comment on attachment 449635 [details]
Patch

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

>> Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:72
>> +}
> 
> We don't need to expose WKViewSetMinimumFontSize, WTR can use WKPreferencesSetMinimumFontSize directly.

O.K.

[ChangSeok Oh]

Comment on attachment 449488 [details]
Patch

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

>>>> Source/JavaScriptCore/ChangeLog:11
>>>> +        compiler complains that happen when accessing JSC headers directly in the test runner.
>>> 
>>> I think we can just use the C API from WTR instead of the glib one.
>> 
>> Sorry, I am not sure if I understand correctly. Are you talking about the dictionary implementation in WebKitWebViewBase.cpp (i.e., webkitWebViewBaseContentsOfUserInterfaceItem)?
> 
> No, I mean we can use the JSC C API, that is already used by WTR, instead of the JSC glib API and then we don't need the changes in jsc headers.

I understood.

>>>> Source/WebKit/UIProcess/API/C/gtk/WKView.cpp:70
>>>> +}
>>> 
>>> WKPreferences is exposed in the C API, I could just add WKPreferencesSetMinimumFontSize() instead
>> 
>> O.K. I get the preference by using these APIs, WKPageGroupGetPreferences(WKPageGetPageGroup(WKViewGetPage(viewRef))). Do you think this is the most efficient way to get the WKPreferences here? If you know a better way, please let me know.
> 
> That's fine, you can also use TestController::platformPreferences().

platformPreferences is private so I had to make it public.

[ChangSeok Oh]

@KaL, would you have a look at the new patch once again? If possible, I'd like to close this bug this week and move on to another.

[Carlos Garcia Campos]

Comment on attachment 449703 [details]
Patch

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

> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:177
> +static Ref<JSON::Object> toJSONObject(GVariant* variant)

Since we always want the string representation, this could return a String instead of the JSON object.

> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:201
> +static JSStringRef toJSStringRef(GVariant* contentDictionary)
> +{
> +    auto jsonObject = toJSONObject(contentDictionary);
> +    return JSStringCreateWithUTF8CString(jsonObject->toJSONString().utf8().data());
> +}

And then we don't need this, because we can use WTR::createString()

[ChangSeok Oh]

Created attachment 450084 [details]
Patch

[ChangSeok Oh]

Comment on attachment 449703 [details]
Patch

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

>> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:177
>> +static Ref<JSON::Object> toJSONObject(GVariant* variant)
> 
> Since we always want the string representation, this could return a String instead of the JSON object.

This function is recursive so it needs to return JSON::Object. Please see line 188.

>> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:201
>> +}
> 
> And then we don't need this, because we can use WTR::createString()

Maybe WTR::createJSString() you mean? This function is removed anyway.

[EWS]

Committed r288666 (246472@main): <https://commits.webkit.org/246472@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450084 [details].

[ChangSeok Oh]

*** Bug 202232 has been marked as a duplicate of this bug. ***