Bug 35593 - [Qt] Add Support for WebKitEnableCaretBrowsing to Qt DRT
Summary: [Qt] Add Support for WebKitEnableCaretBrowsing to Qt DRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-03-02 11:23 PST by Robert Hogan
Modified: 2010-03-10 13:57 PST (History)
2 users (show)

See Also:


Attachments
Patch (8.97 KB, patch)
2010-03-02 11:28 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Update Patch for DRT support only (8.03 KB, patch)
2010-03-10 12:29 PST, Robert Hogan
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-03-02 11:23:18 PST
Unskip test fast/events/multiline-link-arrow-navigation.html
Comment 1 Robert Hogan 2010-03-02 11:28:55 PST
Created attachment 49822 [details]
Patch
Comment 2 Benjamin Poulain 2010-03-05 14:03:52 PST
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.
Comment 3 Eric Seidel (no email) 2010-03-05 15:44:41 PST
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.
Comment 4 Robert Hogan 2010-03-06 10:20:20 PST
(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.
Comment 5 Antonio Gomes 2010-03-09 10:39:26 PST
(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
Comment 6 Robert Hogan 2010-03-09 12:51:55 PST
(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!
Comment 7 Robert Hogan 2010-03-10 12:29:34 PST
Created attachment 50422 [details]
Update Patch for DRT support only

For now, just add support to DRT.
Comment 8 Antonio Gomes 2010-03-10 12:53:42 PST
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.
Comment 9 Robert Hogan 2010-03-10 13:45:20 PST
(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
Comment 10 Robert Hogan 2010-03-10 13:57:39 PST
Manually committed as r55803: http://trac.webkit.org/changeset/55803