Bug 49056 - [GTK] Spatial Navigation: snav-input.html and snav-textarea.html fail
Summary: [GTK] Spatial Navigation: snav-input.html and snav-textarea.html fail
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Chang Shu
URL:
Keywords:
Depends on:
Blocks: 46905
  Show dependency treegraph
 
Reported: 2010-11-04 21:28 PDT by Antonio Gomes
Modified: 2010-11-11 20:16 PST (History)
3 users (show)

See Also:


Attachments
fix patch (2.66 KB, patch)
2010-11-10 14:40 PST, Chang Shu
no flags Details | Formatted Diff | Diff
fix patch 2 (2.64 KB, patch)
2010-11-11 06:06 PST, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-11-04 21:28:28 PDT
Tests added recently and run fine on Qt and Mac ports.
Comment 1 Chang Shu 2010-11-10 12:49:37 PST
I will take a look.
Comment 2 Chang Shu 2010-11-10 14:40:22 PST
Created attachment 73543 [details]
fix patch
Comment 3 WebKit Review Bot 2010-11-10 14:45:42 PST
Attachment 73543 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/Skipped', u'WebKit/gtk/ChangeLog', u'WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp']" exit_code: 1
WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:219:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Chang Shu 2010-11-10 14:52:44 PST
I will fix the style problem along with other possible changes after the comments.
Comment 5 Antonio Gomes 2010-11-10 20:06:44 PST
Comment on attachment 73543 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=73543&action=review

> WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:217
> +    if (step == 1 && isSpatialNavigationEnabled(core(client->webView())->focusController()->focusedOrMainFrame())) {
> +        if (direction == 1)

Looks good to me.

Please reverse the order for the "if", so we bail out earlier in the common case.

if (isSpatialNavigationEnabled(frame) && xxx) {
    all the test here.
}
Comment 6 Chang Shu 2010-11-11 06:06:14 PST
Created attachment 73603 [details]
fix patch 2
Comment 7 Antonio Gomes 2010-11-11 06:30:40 PST
Comment on attachment 73603 [details]
fix patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=73603&action=review

Looks nice! Lets just fix the style issues before landing.

> WebKit/gtk/ChangeLog:6
> +        [GTK] Replace "MoveForward"/"MoveBackward" with "MoveRight"/"MoveLeft"
> +        to make spatial navigation work on input/textarea.

You need the bug title here. Implementation details goes right below that.

> WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:220
> +        if (direction == 1)
> +            rawCommand = "MoveRight";
> +        else if (!direction)
> +            rawCommand = "MoveLeft";

rawCommand = direction ? "MoveRight" ? "MoveLeft";

> LayoutTests/ChangeLog:5
> +        [GTK] Unskip tests that are passing now.

same here: first bug title , bugzilla entry url, then the description.
Comment 8 Chang Shu 2010-11-11 06:38:26 PST
Thanks for the review, Antonio. The style problem was fixed. It was the "direction == 0" it was complaining. Since direction can be 2 or 3, I cannot do "direction?x:y".

> Looks nice! Lets just fix the style issues before landing.
> 
> > WebKit/gtk/ChangeLog:6
> > +        [GTK] Replace "MoveForward"/"MoveBackward" with "MoveRight"/"MoveLeft"
> > +        to make spatial navigation work on input/textarea.
> 
> You need the bug title here. Implementation details goes right below that.
> 
> > WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:220
> > +        if (direction == 1)
> > +            rawCommand = "MoveRight";
> > +        else if (!direction)
> > +            rawCommand = "MoveLeft";
> 
> rawCommand = direction ? "MoveRight" ? "MoveLeft";
>
Comment 9 WebKit Commit Bot 2010-11-11 20:16:34 PST
Comment on attachment 73603 [details]
fix patch 2

Clearing flags on attachment: 73603

Committed r71881: <http://trac.webkit.org/changeset/71881>
Comment 10 WebKit Commit Bot 2010-11-11 20:16:39 PST
All reviewed patches have been landed.  Closing bug.