Bug 30482 – [GTK] Expose Page::tabKeyCyclesThroughElements in the API

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

This is the reason that fast/events/keypress-insert-tab.html is skipped.

[Martin Robinson]

Created attachment 41363 [details]
Patch for this issue

I've attached a patch exposing this method in the GTK+ API. I marked the introduction version as 1.1.16, because I was unsure of release plans.

[Jan Alonzo]

Comment on attachment 41363 [details]
Patch for this issue

> diff --git a/WebKit/gtk/webkit/webkitwebview.cpp b/WebKit/gtk/webkit/webkitwebview.cpp
> index fb2bf35..db9fff3 100644
> --- a/WebKit/gtk/webkit/webkitwebview.cpp
> +++ b/WebKit/gtk/webkit/webkitwebview.cpp
> @@ -170,7 +170,8 @@ enum {
>      PROP_LOAD_STATUS,
>      PROP_PROGRESS,
>      PROP_ENCODING,
> -    PROP_CUSTOM_ENCODING
> +    PROP_CUSTOM_ENCODING,
> +    PROP_TAB_KEY_CYCLES_THROUGH_ELEMENTS

I think this property should be in WebSettings. Thoughts?

> +    case PROP_TAB_KEY_CYCLES_THROUGH_ELEMENTS:
> +        g_value_set_boolean(value, webkit_web_view_get_tab_key_cycles_through_elements(webView));
> +        break;

Ditto with the tab key cycle changes in the WebView. 

> diff --git a/WebKit/gtk/webkit/webkitwebview.h b/WebKit/gtk/webkit/webkitwebview.h
> index 1297695..52915f2 100644
> --- a/WebKit/gtk/webkit/webkitwebview.h
> +++ b/WebKit/gtk/webkit/webkitwebview.h
> @@ -131,241 +131,249 @@ struct _WebKitWebViewClass {
>  };
>  
>  WEBKIT_API GType
> -webkit_web_view_get_type (void);
> +webkit_web_view_get_type                            (void);
>  
>  WEBKIT_API GtkWidget *
> -webkit_web_view_new (void);
> +webkit_web_view_new                                 (void);
>  
>  WEBKIT_API G_CONST_RETURN gchar *
> -webkit_web_view_get_title                       (WebKitWebView        *web_view);
> +webkit_web_view_get_title                           (WebKitWebView        *web_view);
>  
>  WEBKIT_API G_CONST_RETURN gchar *
> -webkit_web_view_get_uri                         (WebKitWebView        *web_view);
> +webkit_web_view_get_uri                             (WebKitWebView        *web_view);
>  

Thanks for fixing the formatting but I think they should be in a separate patch.

>  void LayoutTestController::setTabKeyCyclesThroughElements(bool cycles)
>  {
> -    // FIXME: implement
> +    WebKitWebView* webView = webkit_web_frame_get_web_view(mainFrame);
> +    webkit_web_view_set_tab_key_cycles_through_elements(webView, cycles);

Ditto. The setting should be in WebSettings. 

r=- for now.

[Martin Robinson]

Created attachment 41381 [details]
Page for this issue (WebKitWebSettings version)

I've attached another patch with this setting in WebKitWebSettings. I initially wavered as to where to put this, but chose WebKitWebView. My reasoning was that this would be the first property of WebKitWebSettings that did not have a direct mapping to WebCore::Settings. I'll leave both patches here so they can duke it out.

Also, in my previous patch, I wasn't aiming to fix the formatting. The length of the new method name was longer than the others, so to keep everything lined up I had to move the rest over.

[Jan Alonzo]

Comment on attachment 41381 [details]
Page for this issue (WebKitWebSettings version)

> +    *
> +    * Whether the tab key cycles through elements on the page.
> +    *
> +    * If @flag is %TRUE, pressing the tab key will focus the next element in
> +    * the @web_view. If @flag is %FALSE, the @web_view will interpret tab
> +    * key presses as normal key presses. If the selected element is editable, the

What's the difference between pressing the tab key and the tab key being interpreted as a normal key press?

> +                 "tab-key-cycles-through-elements", &tabKeyCyclesThroughElements,

I think we prefix boolean settings with enable-.

+1 for me. Xan or Gustavo may want to look into this as well.

[Gustavo Noronha (kov)]

Comment on attachment 41381 [details]
Page for this issue (WebKitWebSettings version)

(In reply to comment #4)
> (From update of attachment 41381 [details])
> > +    * If @flag is %TRUE, pressing the tab key will focus the next element in
> > +    * the @web_view. If @flag is %FALSE, the @web_view will interpret tab
> > +    * key presses as normal key presses. If the selected element is editable, the
> 
> What's the difference between pressing the tab key and the tab key being
> interpreted as a normal key press?

As I understand it, TRUE will cause tab presses while on a textarea to move the cursor on to the next element, while FALSE will add \t to the text.

> > +                 "tab-key-cycles-through-elements", &tabKeyCyclesThroughElements,
> 
> I think we prefix boolean settings with enable-.

I think in this case, the setting is more akin to the likes of 'auto-load-images', as in, this is no feature that may be enable/disabled, but decision on behavior of an already enabled feature (interpreting tab key presses). I'm in favor of keeping the name as is in the patch, then. I'm going to go ahead and say r=me.

[WebKit Commit Bot]

Comment on attachment 41381 [details]
Page for this issue (WebKitWebSettings version)

Rejecting patch 41381 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Gustavo Noronha', '--force']" exit_code: 1
Last 500 characters of output:
f 5 hunks FAILED -- saving rejects to file WebKit/gtk/webkit/webkitwebsettings.cpp.rej
patching file WebKit/gtk/webkit/webkitwebview.cpp
Hunk #1 FAILED at 2392.
Hunk #2 FAILED at 2422.
Hunk #3 succeeded at 2451 with fuzz 2 (offset -2 lines).
Hunk #4 FAILED at 2544.
3 out of 4 hunks FAILED -- saving rejects to file WebKit/gtk/webkit/webkitwebview.cpp.rej
patching file WebKitTools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKitTools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp

[Eric Seidel (no email)]

The patch is simply out of date.  Someone could post a new patch which works with tip of tree, or someone could manually apply the patch and land this by hand.  I recommend simply posting an up-to-date patch and getting a new r+ and cq+

[Martin Robinson]

Created attachment 42723 [details]
Patch for this issue (against ToT)

I've attached a patch against tip of trunk.

[WebKit Commit Bot]

Comment on attachment 42723 [details]
Patch for this issue (against ToT)

Clearing flags on attachment: 42723

Committed r50642: <http://trac.webkit.org/changeset/50642>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.