Implementing API to set the initial focus in gtk webkit. windows, chromium and mac port of webkit have this api.A bug is also logged for qt port to implement this API.
Created attachment 105619 [details] Patch
Attachment 105619 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/gtk/webkit/webkitwebview.h:255: Extra space between gboolean and forward [whitespace/declaration] [3] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 105619 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105619&action=review > Source/WebKit/gtk/webkit/webkitwebview.cpp:4026 > +void webkit_web_view_set_initial_focus(WebKitWebView* webView, gboolean forward) This needs API documentation and a unit test. > Source/WebKit/gtk/webkit/webkitwebview.cpp:4027 > +{ Why no g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));? > Source/WebKit/gtk/webkit/webkitwebview.cpp:4030 > + Page* page = core(webView); > + > + if (page && page->focusController()) { Please use an early return here.
(In reply to comment #3) > (From update of attachment 105619 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105619&action=review > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:4026 > > +void webkit_web_view_set_initial_focus(WebKitWebView* webView, gboolean forward) > > This needs API documentation and a unit test. By documentation do you mean the comments present for other APIs, for eg. "webkit_web_view_go_to_back_forward_item()"? If possible could you please provide any reference to the existing API unit test.That would be helpful. > > Source/WebKit/gtk/webkit/webkitwebview.cpp:4027 > > +{ > > Why no g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView));? Ok.Will change accordingly. > > Source/WebKit/gtk/webkit/webkitwebview.cpp:4030 > > + Page* page = core(webView); > > + > > + if (page && page->focusController()) { > > Please use an early return here. Ok.Will change accordingly.
(In reply to comment #4) > By documentation do you mean the comments present for other APIs, for eg. "webkit_web_view_go_to_back_forward_item()"? Here I'm referring to gtkdoc comments that typically prefix all of the public API implementations. Take a look at the comments before webkit_web_view_set_editable and the documentation at http://webkitgtk.org/reference/webkitgtk-webkitwebview.html#webkit-web-view-load-uri. > > If possible could you please provide any reference to the existing API unit test.That would be helpful. The unit tests are in Source/WebKit/gtk/tests.
(In reply to comment #5) > (In reply to comment #4) > > > By documentation do you mean the comments present for other APIs, for eg. "webkit_web_view_go_to_back_forward_item()"? > > Here I'm referring to gtkdoc comments that typically prefix all of the public API implementations. Take a look at the comments before webkit_web_view_set_editable and the documentation at http://webkitgtk.org/reference/webkitgtk-webkitwebview.html#webkit-web-view-load-uri. Ok. > > If possible could you please provide any reference to the existing API unit test.That would be helpful. > > The unit tests are in Source/WebKit/gtk/tests. As I understood each test file inside "Source/WebKit/gtk/tests" is for related implementation in "Source/WebKit/gtk/webkit".For eg. Source/WebKit/gtk/tests/testwebsettings.c is for testing API present in Source/WebKit/gtk/webkit/webkitwebsettings.h. I have added a new API to webkitwebview.h, so should I add a new unit test function in testwebview.c?
(In reply to comment #6) > I have added a new API to webkitwebview.h, so should I add a new unit test function in testwebview.c? Sure.
Created attachment 106552 [details] Patch
Attachment 106552 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/gtk/webkit/webkitwebview.h:255: Extra space between gboolean and forward [whitespace/declaration] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c" Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 106552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106552&action=review > Source/WebKit/gtk/tests/testwebview.c:390 > + > + gtk_widget_show_all(window); > + g_signal_connect(window, "map-event", > + G_CALLBACK(map_event_cb), loop); > + g_main_loop_run(loop); > + > + webkit_web_view_load_uri(view, uri); > + g_main_loop_run(loop); > + > + webkit_web_view_set_initial_focus(view, true); > + g_main_loop_run(loop); > + > + g_free(uri); > + gtk_widget_destroy(window); Your test has no assertions! > Source/WebKit/gtk/webkit/webkitwebview.cpp:4031 > + * Set the initial focus element. Even with this documentation, I'm still not sure what this API does or how it works. I think this could be exanded greatly here. Does GTK+ even have the concept of focus direction? What situations is this API useful?
(In reply to comment #10) > (From update of attachment 106552 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106552&action=review > > > Source/WebKit/gtk/tests/testwebview.c:390 > > + > > + gtk_widget_show_all(window); > > + g_signal_connect(window, "map-event", > > + G_CALLBACK(map_event_cb), loop); > > + g_main_loop_run(loop); > > + > > + webkit_web_view_load_uri(view, uri); > > + g_main_loop_run(loop); > > + > > + webkit_web_view_set_initial_focus(view, true); > > + g_main_loop_run(loop); > > + > > + g_free(uri); > > + gtk_widget_destroy(window); > > Your test has no assertions! > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:4031 > > + * Set the initial focus element. > > Even with this documentation, I'm still not sure what this API does or how it works. I think this could be exanded greatly here. Does GTK+ even have the concept of focus direction? What situations is this API useful? The basic purpose of the API is to set the initial focus in the document.This is need for spatial navigation(keyboard navigation) feature.So with spatial navigation when when a page is loaded, user should be able see the initial position of focus without pressing tab/arrow key.As also mentioned in initial comments, this API is already present in other ports. The test function doesn't have any assertion as its a pretty simple function that just create a window, view and sets the focus on the page loaded.Please let me know any potential place for assertion?
(In reply to comment #11) > > Even with this documentation, I'm still not sure what this API does or how it works. I think this could be exanded greatly here. Does GTK+ even have the concept of focus direction? What situations is this API useful? > The basic purpose of the API is to set the initial focus in the document.This is need for spatial navigation(keyboard navigation) feature.So with spatial navigation when when a page is loaded, user should be able see the initial position of focus without pressing tab/arrow key.As also mentioned in initial comments, this API is already present in other ports. Do you think you could incorporate this somehow into the documentation comment. I also don't understand what the difference is between forward focus and backward focus in the context of this new API. > The test function doesn't have any assertion as its a pretty simple function that just create a window, view and sets the focus on the page loaded.Please let me know any potential place for assertion? If calling this method focuses an element, you can use the DOM bindings to verify the focused element.
Thanks for your feedback. (In reply to comment #12) > Do you think you could incorporate this somehow into the documentation comment. I also don't understand what the difference is between forward focus and backward focus in the context of this new API. Ok. I will modify the documentation. When we set the forward focus it focuses the first focusable node of the page and the backward focus focuses the last focusable element of the page. > > If calling this method focuses an element, you can use the DOM bindings to verify the focused element. Ok.I will modify the test function.
Created attachment 106851 [details] Updated patch. Updated patch as per review comments.
Attachment 106851 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/gtk/webkit/webkitwebview.h:255: Extra space between gboolean and forward [whitespace/declaration] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c" Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 106851 [details] Updated patch. Please don't review the patch.Will upload a modified patch.
Created attachment 106852 [details] Modified patch
Attachment 106852 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/gtk/webkit/webkitwebview.h:255: Extra space between gboolean and forward [whitespace/declaration] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c" Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 106852 [details] Modified patch View in context: https://bugs.webkit.org/attachment.cgi?id=106852&action=review > Source/WebKit/gtk/tests/testwebview.c:368 > + char* uri = g_strconcat(base_uri, "bigdiv.html", NULL); You didn't include this file...you should probably just load HTML as a string via webkit_web_view_load_string. > Source/WebKit/gtk/tests/testwebview.c:388 > + g_signal_connect(window, "map-event", > + G_CALLBACK(map_event_cb), loop); No need to break this line. If I'm not mistaken if you take my suggestion below, you shouldn't need to wait for the map event. > Source/WebKit/gtk/tests/testwebview.c:412 > + char script[] = "if (document.activeElement.id == \"link\") \ > + window.scrollBy(0, 100);"; Hrm...this approach seems a little fragile. Why not just use webkit_dom_html_document_get_active_element from the DOM bindings to inspect the active element? > Source/WebKit/gtk/tests/testwebview.c:414 > + /* Execute the script to verify the focus */ Missing a period here. > Source/WebKit/gtk/webkit/webkitwebview.cpp:4037 > + * Set the initial focus element for spatial(keyboard) navigation.So with spatial > + * navigation when a page is loaded, user should be able see the initial position > + * of focus without pressing tab/arrow key. > + * > + * If @forward is %TRUE, then focus is set in the forward direction i.e. focuses the first > + * focusable element.If @forward is %FALSE, then focus is set in backward direction i.e > + * focuses the last focusable element. Nice documentation. You are missing spaces after all of the periods though. I recommned replacing both "i.e." with something like this: If @forward is %TRUE, then focus is set in the forward direction, which focuses the first focusable element. > Source/WebKit/gtk/webkit/webkitwebview.cpp:4044 > + Extra newline. > Source/WebKit/gtk/webkit/webkitwebview.cpp:4049 > + Frame* frame = page->focusController()->focusedOrMainFrame(); > + frame->document()->setFocusedNode(0); No need to cache the frame here.
Created attachment 107025 [details] Patch Updated patch as per review comments.
Attachment 107025 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/gtk/webkit/webkitwebview.h:255: Extra space between gboolean and forward [whitespace/declaration] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c" Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 107025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107025&action=review This looks good to me. Since it is an API change, it needs the approval of another reviewer. It'd also be nice if one of a11y people could chime in to say if this negatively affects caret navigation. > Source/WebKit/gtk/tests/testwebview.c:399 > + element = webkit_dom_html_document_get_active_element((WebKitDOMHTMLDocument*)document); Here it would be good to use a GLib style cast if possible: WEBKIT_DOM_HTML_DOCUMENT(document). > Source/WebKit/gtk/webkit/webkitwebview.cpp:4034 > + * Set the initial focus element for spatial(keyboard) navigation. So with spatial > + * navigation when a page is loaded, user should be able see the initial position > + * of focus without pressing tab/arrow key. > + * The English is a just a little off here. Maybe something like: Set the initially focused element, so that the user can see the initial focus position withou pressing the tab or arrow key. I removed the mention of spatial navigation because I assume this does the same thing whether or not it's enabled. Is that true?
Created attachment 107142 [details] Patch Modified the patch as per review comments.
Attachment 107142 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/gtk/webkit/webkitwebview.h:255: Extra space between gboolean and forward [whitespace/declaration] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c" Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #23) > Created an attachment (id=107142) [details] > Patch > > Modified the patch as per review comments. Do you mind answering my question above: I removed the mention of spatial navigation because I assume this does the same thing whether or not it's enabled. Is that true?
(In reply to comment #25) > (In reply to comment #23) > > Created an attachment (id=107142) [details] [details] > > Patch > > > > Modified the patch as per review comments. > > Do you mind answering my question above: > > I removed the mention of spatial navigation because I assume this does the same thing whether or not it's enabled. Is that true? yes you are right.But currently it's useful mainly for keyboard(spatial) navigation to set the initial focus.
This looks good to me, but we need one more GTK+ reviewer to officially approve this API. I'm CCing some people.
I was debating internally yesterday whether this should be better built as a setting, what do you think? I'm fine with going forward with the proposed API, but this sounds so much like it would be used as a setting.
Are you imagining it being an enum like FORWARD, REVERSE, NONE?
Hi, Overall, the patch looks good to me, just some minor nitpicking: (In reply to comment #29) > Are you imagining it being an enum like FORWARD, REVERSE, NONE? I agree with Martin, I'd rather have a enum type instead of a bool for that parameter, since it's more clear and less error prone, IMHO. See this post from Havoc Pennington on the topic for a further explanation why I agree with this: http://blog.ometer.com/2011/01/20/boolean-parameters-are-wrong With regard to this piece of code: [...] + static void test_webkit_web_view_set_initial_focus() + { + WebKitDOMDocument* document; + WebKitDOMElement* element; [...] You should be declaring those parameters later in the code, as soon as you need it, instead of doing it at that early point, which is the normal thing in WK.
Thanks to all for the valuable feedback. (In reply to comment #30) > Hi, > > Overall, the patch looks good to me, just some minor nitpicking: > > (In reply to comment #29) > > Are you imagining it being an enum like FORWARD, REVERSE, NONE? > > I agree with Martin, I'd rather have a enum type instead of a bool for that parameter, since it's more clear and less error prone, IMHO. See this post from Havoc Pennington on the topic for a further explanation why I agree with this: > > http://blog.ometer.com/2011/01/20/boolean-parameters-are-wrong > Actually the API focuses either the first or last focusable element.So I think a boolean is apt for it.Please provide your thought. > > With regard to this piece of code: > > [...] > + static void test_webkit_web_view_set_initial_focus() > + { > + WebKitDOMDocument* document; > + WebKitDOMElement* element; > [...] > > You should be declaring those parameters later in the code, as soon as you need it, instead of doing it at that early point, which is the normal thing in WK. Ok.
(In reply to comment #31) > Actually the API focuses either the first or last focusable element.So I think a boolean is apt for it.Please provide your thought. That's not the point, the point is a boolean is harder to understand by just looking at the code - you have to know the parameter is called 'forward' to understand that TRUE means "go forward", while if you use an enum you'll see FOCUS_FIRST, which is very obvious, agree? =) I agree with Mario here, we should make it an enum, indeed.
(In reply to comment #32) > That's not the point, the point is a boolean is harder to understand by just looking at the code - you have to know the parameter is called 'forward' to understand that TRUE means "go forward", while if you use an enum you'll see FOCUS_FIRST, which is very obvious, agree? =) I agree with Mario here, we should make it an enum, indeed. Ok.will make the necessary changes and upload the modified patch.
Before updating the patch it would have been better to understand if Gustavo suggested this be a property on WebKitWebSettings instead.
(In reply to comment #34) > Before updating the patch it would have been better to understand if Gustavo suggested this be a property on WebKitWebSettings instead. As I understand most of the property present in webkitwebsettings corresponds to one feature and have some default value.And user can override these with command line options.The "PROP_ENABLE_SPATIAL_NAVIGATION" (spatial navigation feature) has default value false and user can override them with "--enable-spatial-navigation=true". The propose API is not a functional feature and it's useful mainly for keyboard(spatial) navigation to set the initial focus.so please provide me some pointers on how we can use it with webkitwebsettings, if we intend to use it with websettings.That will be helpful.
The way I envisioned this would be having this in WebSettings so that, if you set the initial focus setting to, let's say, FIRST, the focus would be done automatically whenever a page is loaded. On ther other hand, this looks more akin to full content zoom than to, say, enable-developers-tools, so I'm fine with keeping it a WebView property.
Comment on attachment 107142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107142&action=review > Source/WebKit/gtk/webkit/webkitwebview.cpp:4048 > +void webkit_web_view_set_initial_focus(WebKitWebView* webView, gboolean forward) > +{ > + g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView)); > + > + Page* page = core(webView); > + if (!page || !page->focusController()) > + return; > + > + page->focusController()->focusedOrMainFrame()->document()->setFocusedNode(0); > + page->focusController()->setInitialFocus(forward ? FocusDirectionForward : FocusDirectionBackward, 0); > +} Ah, this is a one-time thing...might want to update the documentation about that. What does this do exactly? Does it always focus the first node and then just set the focus direction? >> Source/WebKit/gtk/webkit/webkitwebview.h:255 >> + gboolean forward); > > Extra space between gboolean and forward [whitespace/declaration] [3] You need one more space here.
(In reply to comment #37) > (From update of attachment 107142 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107142&action=review > > > Source/WebKit/gtk/webkit/webkitwebview.cpp:4048 > > +void webkit_web_view_set_initial_focus(WebKitWebView* webView, gboolean forward) > > +{ > > + g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView)); > > + > > + Page* page = core(webView); > > + if (!page || !page->focusController()) > > + return; > > + > > + page->focusController()->focusedOrMainFrame()->document()->setFocusedNode(0); > > + page->focusController()->setInitialFocus(forward ? FocusDirectionForward : FocusDirectionBackward, 0); > > +} > > Ah, this is a one-time thing...might want to update the documentation about that. What does this do exactly? Does it always focus the first node and then just set the focus direction? Actually focusController::setInitialFocus() it uses the focus direction to determine which node to focus, eg. the first focusable node or the last. > >> Source/WebKit/gtk/webkit/webkitwebview.h:255 > >> + gboolean forward); > > > > Extra space between gboolean and forward [whitespace/declaration] [3] > > You need one more space here. As per earlier review comments I should use enum in place of boolean.So please let me know if I should keep the boolean or use enum.
(In reply to comment #38) > > > > Ah, this is a one-time thing...might want to update the documentation about that. What does this do exactly? Does it always focus the first node and then just set the focus direction? > > Actually focusController::setInitialFocus() it uses the focus direction to determine which node to focus, eg. the first focusable node or the last. What does this provide that you cannot do with DOM APIs. I guess that's what I'm confused about.
(In reply to comment #39) > (In reply to comment #38) > > > > > > Ah, this is a one-time thing...might want to update the documentation about that. What does this do exactly? Does it always focus the first node and then just set the focus direction? > > > > Actually focusController::setInitialFocus() it uses the focus direction to determine which node to focus, eg. the first focusable node or the last. > > What does this provide that you cannot do with DOM APIs. I guess that's what I'm confused about. yes.it focuses a node.
(In reply to comment #40) > > What does this provide that you cannot do with DOM APIs. I guess that's what I'm confused about. > > yes.it focuses a node. You can focus DOM elements with the GObject DOM APIs. Have you tried that? Is it different from what this patch does? If it is, how is it different?
(In reply to comment #41) > (In reply to comment #40) > > > What does this provide that you cannot do with DOM APIs. I guess that's what I'm confused about. > > > > yes.it focuses a node. > > You can focus DOM elements with the GObject DOM APIs. Have you tried that? Is it different from what this patch does? If it is, how is it different? Sorry If I have unable to represent my issues. 1. I am using it with spatial navigation feature.With current code when a page loads and when user presses tab/arrow key then only the first focusable node is focused i.e displayed with focus ring. 2. But the behavior user may need is as the page loads, he should be able to see the initial focus on node, and on subsequent tab/arrow press he continue from that initial node. 3. To achieve this I need a node which can take focus (link, text box etc) and then focus it. Please note that first focusable node on the page may not be the first node on the page. 4. A option to set the focus based on direction can help, say focus the last focusable node. 5. Sometimes user may want to toggle between modes (spatial and normal). currently I don't have any use case for this scenario, but Qt browser provides this option. I tried to find a DOM API which does this, but didn't find one. I think a way to do it with it as follows:- 1. I have to get the list of nodes present on the page, I think with "webkit_dom_node_get_child_nodes" passing the document as a node. 2. Further I have to check each node whether this is a focusable node or not. I have perform this check as I just don't want to focus the first node, but the first focusable node.I didn't find any corresponding DOM API. Please let me know if there is one. 3. When I encounter the first node, focus it using "webkit_dom_element_focus". Please provide your feedback.
Created attachment 108448 [details] Patch Patch with review comments.
Attachment 108448 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/gtk/webkit/webkitwebview.h:85: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/gtk/webkit/webkitwebview.h:261: Extra space between WebKitFocusDirection and direction [whitespace/declaration] [3] WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c" Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 108448 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
I think we are just going to skip this now until we need it.