Bug 35593 – [Qt] Add Support for WebKitEnableCaretBrowsing to Qt DRT

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Robert Hogan]

Unskip test fast/events/multiline-link-arrow-navigation.html

[Robert Hogan]

Created attachment 49822 [details]
Patch

[Benjamin Poulain]

Comment on attachment 49822 [details]
Patch

Overall, shouldn't that be only made available for the DumpRenderTree? I don't really like the idea of adding public API for a corner case.

> +        value = attributes.value(QWebSettings::CaretBrowsingEnabled,
> +                                      global->attributes.value(QWebSettings::CaretBrowsingEnabled));

The indentation looks wrong here.

[Eric Seidel (no email)]

Comment on attachment 49822 [details]
Patch

This looks good, but can't we make a Layout test for this?  We have layoutTestController.overridePreference() these days.  Seems we could make a simple test to at least validate that your check:
+    // Don't allow editor commands or text insertion for nodes that
+    // cannot edit, unless we are in caret mode.
+    if (!frame->editor()->canEdit() && !(frame->settings() && frame->settings()->caretBrowsingEnabled()))
+        return;
+
is correct.

[Robert Hogan]

(In reply to comment #2)
> (From update of attachment 49822 [details])
> Overall, shouldn't that be only made available for the DumpRenderTree? I don't
> really like the idea of adding public API for a corner case.

I know what you mean. But adding the setting to the API would at least allow QtWebKit clients to make use of it should they be so inclined. Is there a sense that QWebSettings should not get bloated with any and all settings available in WebCore?

I'm easy - so let me know which is the preferred route.

[Antonio Gomes]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 49822 [details] [details])
> > Overall, shouldn't that be only made available for the DumpRenderTree? I don't
> > really like the idea of adding public API for a corner case.
> 
> I know what you mean. But adding the setting to the API would at least allow
> QtWebKit clients to make use of it should they be so inclined. Is there a sense
> that QWebSettings should not get bloated with any and all settings available in
> WebCore?
> 
> I'm easy - so let me know which is the preferred route.

I like the idea of making it possible to enable it. Real browsers (read Safari, Firefox and Opera) have this feature and it improves the accessibility support of the browser.

@Robert, something we should be really sure about is if this feature (Caret Browsing) an Spatial Navigation (see bug 18662) would not conflitate if both are enabled at the same time. I bet there will be a problem I will double check that as well.

see bug 33714

[Robert Hogan]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> 
> @Robert, something we should be really sure about is if this feature (Caret
> Browsing) an Spatial Navigation (see bug 18662) would not conflitate if both
> are enabled at the same time. I bet there will be a problem I will double check
> that as well.
> 
> see bug 33714

Mm, that sounds like another day's work. My emphasis here is to just get tests to pass. Oddly enough, fast/events/multiline-link-arrow-navigation.html is the only one that tests caret browsing at all in LayoutTests. So there is definitely room for a lot more coverage!

[Robert Hogan]

Created attachment 50422 [details]
Update Patch for DRT support only

For now, just add support to DRT.

[Antonio Gomes]

nitpick : that change is minor, but likely unrelated :)

-    \value LocalContentCanAccessFileUrls Specifies whether locally loaded documents are allowed to access other local  urls.
+    \value LocalContentCanAccessFileUrls Specifies whether locally loaded documents are allowed to access other local urls.


and then this file should not be being touched (in commit message)

 WebKit/qt/Api/qwebsettings.cpp                     |    2 +-




othe than that , it looks good to me.

[Robert Hogan]

(In reply to comment #8)
> nitpick : that change is minor, but likely unrelated :)
> 
> -    \value LocalContentCanAccessFileUrls Specifies whether locally loaded
> documents are allowed to access other local  urls.
> +    \value LocalContentCanAccessFileUrls Specifies whether locally loaded
> documents are allowed to access other local urls.
> 
> 
> and then this file should not be being touched (in commit message)

Yes, wasn't sure if I was being naughty. I do mention it in the changelog though:

* Api/qwebsettings.cpp: 
 14         (QWebSettingsPrivate::apply): fix typo in docs

[Robert Hogan]

Manually committed as r55803: http://trac.webkit.org/changeset/55803