Bug 116676 - [BlackBerry] Respect tabindex when using form controls.
Summary: [BlackBerry] Respect tabindex when using form controls.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike Fenton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-23 08:01 PDT by Mike Fenton
Modified: 2013-05-24 05:47 PDT (History)
1 user (show)

See Also:


Attachments
Patch (9.65 KB, patch)
2013-05-23 08:03 PDT, Mike Fenton
xan.lopez: review-
Details | Formatted Diff | Diff
Updated Patch (10.11 KB, patch)
2013-05-23 10:24 PDT, Mike Fenton
xan.lopez: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Fixed patch. (10.07 KB, patch)
2013-05-24 05:17 PDT, Mike Fenton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Fenton 2013-05-23 08:01:03 PDT
SSIA.
Comment 1 Mike Fenton 2013-05-23 08:03:25 PDT
Created attachment 202715 [details]
Patch
Comment 2 Xan Lopez 2013-05-23 09:09:58 PDT
Comment on attachment 202715 [details]
Patch

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

It took me a while to figure out what this was doing. I think here it would be really useful to have a general comment at the beginning saying that we basically:

Do a full pass for every node. The main case is the focus node, there we use the source order to select the prev/next candidates. For the other nodes, if tabIndex is positive, we use their tabIndex to pick the best prev/next. Otherwise remember the highest tabindex, since that will be the prev node whe tabIndex of the focused node is 0.

This is pretty hairy, I'm sure you can write a good explanation. The logic seems sound to me after examination, but r- to add a good comment.

> Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp:956
> +    int nextTabIndex = std::numeric_limits<short>::max() + 1;

This seems pretty magical at first sight, since numeric limit for short + 1 in an int is just A Big Number(tm). You mentioned on IRC that this is likely because the limit for tab index is a short, so maybe worth having a comment.
Comment 3 Mike Fenton 2013-05-23 10:24:58 PDT
Created attachment 202726 [details]
Updated Patch
Comment 4 Xan Lopez 2013-05-24 02:27:03 PDT
Comment on attachment 202726 [details]
Updated Patch

Looks good to me, but EWS says the patch does not apply cleanly, so I think we won't be able to land it. r+ anyway.
Comment 5 WebKit Commit Bot 2013-05-24 02:30:09 PDT
Comment on attachment 202726 [details]
Updated Patch

Rejecting attachment 202726 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 202726, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
z']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Parsed 2 diffs from patch file(s).
patching file Source/WebKit/blackberry/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/blackberry/WebKitSupport/InputHandler.cpp
patch: **** malformed patch at line 124: +        m_previousFocusableTextElement = highestTabIndexElement;


Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Xan Lopez']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/686021
Comment 6 Mike Fenton 2013-05-24 05:17:23 PDT
Created attachment 202805 [details]
Fixed patch.
Comment 7 WebKit Commit Bot 2013-05-24 05:46:58 PDT
Comment on attachment 202805 [details]
Fixed patch.

Clearing flags on attachment: 202805

Committed r150637: <http://trac.webkit.org/changeset/150637>
Comment 8 WebKit Commit Bot 2013-05-24 05:47:00 PDT
All reviewed patches have been landed.  Closing bug.