Bug 167579 - [GTK] WTR: Native HTML form validation popover is not supported
Summary: [GTK] WTR: Native HTML form validation popover is not supported
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: ChangSeok Oh
URL:
Keywords: Gtk, LayoutTestFailure
: 202232 (view as bug list)
Depends on: 234629 235303
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-30 02:06 PST by Carlos Garcia Campos
Modified: 2022-01-27 08:32 PST (History)
18 users (show)

See Also:


Attachments
Patch (22.25 KB, patch)
2022-01-19 09:04 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (22.22 KB, patch)
2022-01-20 20:13 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (16.60 KB, patch)
2022-01-21 15:39 PST, ChangSeok Oh
cgarcia: review+
cgarcia: commit-queue-
Details | Formatted Diff | Diff
Patch (16.47 KB, patch)
2022-01-26 16:58 PST, ChangSeok Oh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-01-30 02:06:40 PST
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.
Comment 1 Carlos Garcia Campos 2017-01-30 02:12:22 PST
Skipped in r211359
Comment 2 Miguel Gomez 2018-11-29 07:57:22 PST
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 ]
Comment 3 ChangSeok Oh 2021-12-29 18:14:20 PST
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.
Comment 4 ChangSeok Oh 2022-01-19 09:04:08 PST
Created attachment 449488 [details]
Patch
Comment 5 EWS Watchlist 2022-01-19 09:06:58 PST
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
Comment 6 ChangSeok Oh 2022-01-19 16:28:16 PST
Hrm... Why do mac/ios ports complain about a changeset for the GTK port? -_-a
Comment 7 Carlos Garcia Campos 2022-01-20 00:25:32 PST
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.
Comment 8 ChangSeok Oh 2022-01-20 20:12:51 PST
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.
Comment 9 ChangSeok Oh 2022-01-20 20:13:21 PST
Created attachment 449635 [details]
Patch
Comment 10 Carlos Garcia Campos 2022-01-21 02:15:37 PST
(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.
Comment 11 Carlos Garcia Campos 2022-01-21 02:19:23 PST
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.
Comment 12 ChangSeok Oh 2022-01-21 15:39:03 PST
Created attachment 449703 [details]
Patch
Comment 13 ChangSeok Oh 2022-01-21 15:39:38 PST
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.
Comment 14 ChangSeok Oh 2022-01-21 15:40:48 PST
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.
Comment 15 ChangSeok Oh 2022-01-25 13:45:00 PST
@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.
Comment 16 Carlos Garcia Campos 2022-01-26 03:20:14 PST
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()
Comment 17 ChangSeok Oh 2022-01-26 16:58:04 PST
Created attachment 450084 [details]
Patch
Comment 18 ChangSeok Oh 2022-01-26 17:01:34 PST
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.
Comment 19 EWS 2022-01-26 22:17:45 PST
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].
Comment 20 ChangSeok Oh 2022-01-27 08:32:39 PST
*** Bug 202232 has been marked as a duplicate of this bug. ***