Bug 235303 - [GTK] Implement native form validation bubbles for GTK4
Summary: [GTK] Implement native form validation bubbles for GTK4
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: ChangSeok Oh
URL:
Keywords:
Depends on:
Blocks: 167579
  Show dependency tree / graph
 
Reported: 2022-01-17 14:48 PST by ChangSeok Oh
Modified: 2022-01-18 15:58 PST (History)
4 users (show)

See Also:

Attachments
Patch (5.62 KB, patch)
2022-01-17 19:52 PST, ChangSeok Oh 		cgarcia: review+
cgarcia: commit-queue-
Details | Formatted Diff | Diff
Patch (5.32 KB, patch)
2022-01-18 14:42 PST, ChangSeok Oh 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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].