1263145 - Don't render multiline input text entries as GtkEntry

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Martin Stránský [:stransky] (ni? me)]

GtkEntry is not supposed to hold multiline text. For instance it can have round corners so scrollbars does not fit there. Also may have padding/border defined so there's space between scrollbars and entry widget border.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: text.patch

Replaces GtkEntry with GtkTextView. It needs to get proper background by GTK_STYLE_CLASS_VIEW as Gtk does.

The GTK_STYLE_CLASS_FRAME is needed for the frame, GtkTextView does not draw border without it. We do similar thing to gScrolledWindowWidget for instance.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8739463 [details] [diff] [review]
text.patch

(In reply to Martin Stránský from comment #1)
> Replaces GtkEntry with GtkTextView. It needs to get proper background by
> GTK_STYLE_CLASS_VIEW as Gtk does.

That's good, thanks.

> The GTK_STYLE_CLASS_FRAME is needed for the frame, GtkTextView does not draw
> border without it. We do similar thing to gScrolledWindowWidget for instance.

GTK draws the frame on the GtkScrolledWindow parent of the GtkTextView, and
NS_THEME_TEXTFIELD_MULTILINE corresponds to both of those widgets.
Therefore, draw the GtkScrolledWindow and then the GtkTextView inset
appropriately for border+padding.

I don't see any border/padding code in gtktextview.c, so I think they need
only be considered for the GtkScrolledWindow.  That means MOZ_GTK_TREEVIEW and
MOZ_GTK_TEXT_VIEW can share the same code in moz_gtk_get_widget_border().

>+static GtkWidget* gTextViewWidget;

Note that this already exists since
http://hg.mozilla.org/mozilla-central/rev/f7eef7358c99

>+    /* This gets us a lovely greyish disabledish look */
>+    gtk_widget_set_sensitive(gTextViewWidget, !state->disabled);

I expect this is no longer required with GTK_STATE_FLAG_INSENSITIVE set on the
style context below.

>+STUB(gtk_text_view_get_type)

What depends on this symbol?

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.2

Thanks, there's the updated one.

> GTK draws the frame on the GtkScrolledWindow parent of the GtkTextView, and
> NS_THEME_TEXTFIELD_MULTILINE corresponds to both of those widgets.
> Therefore, draw the GtkScrolledWindow and then the GtkTextView inset
> appropriately for border+padding.
> 
> I don't see any border/padding code in gtktextview.c, so I think they need
> only be considered for the GtkScrolledWindow.  That means MOZ_GTK_TREEVIEW
> and
> MOZ_GTK_TEXT_VIEW can share the same code in moz_gtk_get_widget_border().

I used that during developing this patch but I decided to go with GtkTextView to avoid potential breakage when gtk internal implementation changes. The GtkScrolledWindow is not official parent of GtkTextView, it's only an implementation detail. But if you're rather go with GtkScrolledWindow I guess there's no big issue here.

> >+STUB(gtk_text_view_get_type)
> 
> What depends on this symbol?

Ahh, looks like a leftover from gtk3.20 tests when I used that to get GtkTexView type for CSS node creation. We don't need it here so removed.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8739914 [details] [diff] [review]
patch v.2

> > GTK draws the frame on the GtkScrolledWindow parent of the GtkTextView, and
> > NS_THEME_TEXTFIELD_MULTILINE corresponds to both of those widgets.
> > Therefore, draw the GtkScrolledWindow and then the GtkTextView inset
> > appropriately for border+padding.

(In reply to Martin Stránský from comment #4)
> I used that during developing this patch but I decided to go with
> GtkTextView to avoid potential breakage when gtk internal implementation
> changes. The GtkScrolledWindow is not official parent of GtkTextView, it's
> only an implementation detail. But if you're rather go with
> GtkScrolledWindow I guess there's no big issue here.

I want the GtkScrolledWindow for drawing the frame because that is the widget
that draws the frame around the scrollable area and corresponds with how Gecko
uses NS_THEME_TEXTFIELD_MULTILINE.  GtkScrolledWindow is not an internal
implementation detail, but the widget used by applications when text may be
scrollable.  GtkTextView doesn't draw a frame and so applications need to use
a parent widget with a frame, or border windows on the text view, if they want
a frame.

Using GTK_STYLE_CLASS_FRAME on the text view introduces the risk of not
matching a style rule from a theme that matches against the widget type or node
name instead of the style class.  It will also draw the text view background
too large for any themes that may choose to use rounded borders.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.3

Okay, there's the updated one. Thanks.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8740945 [details] [diff] [review]
patch v.3

>+    style_frame = gtk_widget_get_style_context(gTextViewWidget);
>+    gtk_style_context_save(style_frame);
>+    gtk_style_context_add_class(style_frame, GTK_STYLE_CLASS_FRAME);

Use gScrolledWindowWidget for style_frame.

>+        /* This will get us the lit borders that focused textboxes enjoy on
>+         * some themes. */
>+        gtk_style_context_set_state(style, GTK_STATE_FLAG_FOCUSED);
>+        gtk_style_context_set_state(style_frame, GTK_STATE_FLAG_FOCUSED);

The comment doesn't really apply here.  GTK does not use lit borders on scrolled window borders when the text views are focused.  GTK sets GTK_STATE_FLAG_FOCUSED on only one widget.

I don't mind if you you want to try to show focus on the scrolled window frame also, but I don't expect it will be effective.

>+    gtk_render_background(style, cr, rect->x, rect->y, rect->width, rect->height);
>+    gtk_render_frame(style_frame, cr, rect->x, rect->y, rect->width, rect->height);

This is the correct rect for gtk_render_frame corresponding to the scrolled
window.

For gtk_render_background corresponding to the text view, use a smaller
rectangle, inset by style_frame border and padding, and render_background with |style| after rendering with |style_frame|.  The text view is a child of the scrolled window, and so gets drawn after (on top of) the scrolled window.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.4

Thanks, there's the updated one.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8741701 [details] [diff] [review]
patch v.4

>+    GtkStyleContext* style;
>+    GtkStyleContext* style_frame;

Please declare at the initialization, in line with
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices

>+    /* only handle disabled and normal states, otherwise the whole background
>+     * area will be painted differently with other states */
>+    GtkStateFlags state_flags = state->disabled ? GTK_STATE_FLAG_INSENSITIVE :
>+                                                  GTK_STATE_FLAG_NORMAL;

This comment doesn't make so much sense here because state_flags is used only for border and padding.

>+    GtkBorder border, padding;
>+    gtk_style_context_get_border(style, state_flags, &border);
>+    gtk_style_context_get_padding(style, state_flags, &padding);

|style_frame| (from the scrolled window) for the inset by border and padding.

>+    gtk_render_background(style_frame, cr,

|style| (from the text view) for the background.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.5

Sorry, I hope this one is correct. I also used the default state for border+padding.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8745900 [details] [diff] [review]
patch v.5

(In reply to Martin Stránský from comment #10)
> I also used the default state for border+padding.

Nice, thanks.

Comment 12

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8745900 [details] [diff] [review]
patch v.5

Thanks! Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dbf335584be

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd98bfea4fd

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5cd98bfea4fd