Bug 45288 - Fix speech button's hit test logic for RTL rendering.
Summary: Fix speech button's hit test logic for RTL rendering.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 39485
  Show dependency treegraph
 
Reported: 2010-09-07 04:21 PDT by Satish Sampath
Modified: 2010-09-07 06:28 PDT (History)
2 users (show)

See Also:


Attachments
Patch (6.29 KB, patch)
2010-09-07 04:25 PDT, Satish Sampath
no flags Details | Formatted Diff | Diff
Patch (6.40 KB, patch)
2010-09-07 05:53 PDT, Satish Sampath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Satish Sampath 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.
Comment 1 Satish Sampath 2010-09-07 04:25:09 PDT
Created attachment 66704 [details]
Patch
Comment 2 Steve Block 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?
Comment 3 Satish Sampath 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.
Comment 4 Satish Sampath 2010-09-07 05:53:02 PDT
Created attachment 66709 [details]
Patch
Comment 5 Steve Block 2010-09-07 06:04:32 PDT
Comment on attachment 66709 [details]
Patch

r=me
Comment 6 WebKit Commit Bot 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>
Comment 7 WebKit Commit Bot 2010-09-07 06:28:27 PDT
All reviewed patches have been landed.  Closing bug.