Bug 235303

Summary: [GTK] Implement native form validation bubbles for GTK4
Product: WebKit Reporter: ChangSeok Oh <changseok>
Component: WebKitGTKAssignee: ChangSeok Oh <changseok>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, aperez, bugs-noreply, cgarcia
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 167579    
Attachments:
Description Flags
Patch
cgarcia: review+, cgarcia: commit-queue-
Patch none

Description ChangSeok Oh 2022-01-17 14:48:31 PST
This is a follow-up ticket for bug 234629. We enable native validation bubbles for gtk4.
Comment 1 ChangSeok Oh 2022-01-17 19:52:07 PST
Created attachment 449362 [details]
Patch
Comment 2 Carlos Garcia Campos 2022-01-18 00:25:35 PST
Comment on attachment 449362 [details]
Patch

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

> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:68
> +#if USE(GTK4)
> +    gtk_label_set_wrap(GTK_LABEL(label), TRUE);
> +#else
>      gtk_label_set_line_wrap(GTK_LABEL(label), TRUE);
> +#endif

Include <WebCore/GtkVersioning.h> and just use gtk_label_set_line_wrap() without ifdefs.
Comment 3 Adrian Perez 2022-01-18 00:28:29 PST
Comment on attachment 449362 [details]
Patch

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

> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:83
> +    gtk_popover_set_position(GTK_POPOVER(m_popover), GTK_POS_TOP);

Is it needed to move the “gtk_popover_set_position()“ call? Also, maybe it would be
possible to move the call after the “#endif” given that it is the same for both GTK3
and GTK4.
Comment 4 ChangSeok Oh 2022-01-18 14:42:00 PST
Created attachment 449426 [details]
Patch
Comment 5 ChangSeok Oh 2022-01-18 14:50:06 PST
Comment on attachment 449362 [details]
Patch

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

Thanks for your review.

>> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:68
>> +#endif
> 
> Include <WebCore/GtkVersioning.h> and just use gtk_label_set_line_wrap() without ifdefs.

I was not aware of this abstraction. Good to know. Thanks.

>> Source/WebCore/platform/gtk/ValidationBubbleGtk.cpp:83
>> +    gtk_popover_set_position(GTK_POPOVER(m_popover), GTK_POS_TOP);
> 
> Is it needed to move the “gtk_popover_set_position()“ call? Also, maybe it would be
> possible to move the call after the “#endif” given that it is the same for both GTK3
> and GTK4.

I was concerned about flickering possibly caused by the gtk_popover_set_position call after attaching the popover to parents. But it seems fine. I have moved this line below #endif.
Comment 6 Adrian Perez 2022-01-18 15:19:20 PST
Comment on attachment 449426 [details]
Patch

Patch LGTM now, thanks for the update!
Comment 7 EWS 2022-01-18 15:57:57 PST
Committed r288156 (246150@main): <https://commits.webkit.org/246150@main>

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