Bug 45288

Summary: Fix speech button's hit test logic for RTL rendering.
Product: WebKit Reporter: Satish Sampath <satish>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 39485    
Attachments:
Description Flags
Patch
none
Patch none

Satish Sampath
Reported 2010-09-07 04:21:05 PDT
Instead of checking individual values, we now do a bounds check with the button's frame rect which works for both right-to-left and left-to-right languages. Also updated layout test to check for this case. Since a separate JS file under /script-tests is no longer preferred, I also merged the JS and html files into one.
Attachments
Patch (6.29 KB, patch)
2010-09-07 04:25 PDT, Satish Sampath 		no flags
DetailsFormatted DiffDiff
Patch (6.40 KB, patch)
2010-09-07 05:53 PDT, Satish Sampath 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Satish Sampath
Comment 1 2010-09-07 04:25:09 PDT
Created attachment 66704 [details] Patch
Steve Block
Comment 2 2010-09-07 04:53:30 PDT
Comment on attachment 66704 [details] Patch > diff --git a/LayoutTests/fast/speech/input-text-speechbutton.html b/LayoutTests/fast/speech/input-text-speechbutton.html > <script src="../js/resources/js-test-post.js"></script> Is there any precedent for this - writing the test as a non-script-test, but using the script-test resources? > diff --git a/WebCore/rendering/RenderTextControlSingleLine.cpp b/WebCore/rendering/RenderTextControlSingleLine.cpp > + FloatPoint localPoint = absoluteToLocal(static_cast<MouseEvent*>(event)->absoluteLocation(), false, true); I know that this variable is scoped to not conflict with localPoint below, but using a different name would help with readability. > + if (speechBox->frameRect().contains(static_cast<int>(localPoint.x()), static_cast<int>(localPoint.y()))) { Why are the static casts required?
Satish Sampath
Comment 3 2010-09-07 05:43:05 PDT
(In reply to comment #2) > > <script src="../js/resources/js-test-post.js"></script> > Is there any precedent for this - writing the test as a non-script-test, but using the script-test resources? These includes are not script-test related but general JS test helpers, used by most tests. For e.g. an indexed db test - http://trac.webkit.org/browser/trunk/LayoutTests/storage/indexeddb/objectstore-cursor.html > > diff --git a/WebCore/rendering/RenderTextControlSingleLine.cpp b/WebCore/rendering/RenderTextControlSingleLine.cpp > > + FloatPoint localPoint = absoluteToLocal(static_cast<MouseEvent*>(event)->absoluteLocation(), false, true); > I know that this variable is scoped to not conflict with localPoint below, but using a different name would help with readability. Will do. > > > + if (speechBox->frameRect().contains(static_cast<int>(localPoint.x()), static_cast<int>(localPoint.y()))) { > Why are the static casts required? .x() and .y() return floats, and I explicitly casted them to int to avoid compile warnings/errors of precision loss.
Satish Sampath
Comment 4 2010-09-07 05:53:02 PDT
Created attachment 66709 [details] Patch
Steve Block
Comment 5 2010-09-07 06:04:32 PDT
Comment on attachment 66709 [details] Patch r=me
WebKit Commit Bot
Comment 6 2010-09-07 06:28:23 PDT
Comment on attachment 66709 [details] Patch Clearing flags on attachment: 66709 Committed r66879: <http://trac.webkit.org/changeset/66879>
WebKit Commit Bot
Comment 7 2010-09-07 06:28:27 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.