Bug 45288 – Fix speech button's hit test logic for RTL rendering.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Satish Sampath]

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.

[Satish Sampath]

Created attachment 66704 [details]
Patch

[Steve Block]

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]

(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]

Created attachment 66709 [details]
Patch

[Steve Block]

Comment on attachment 66709 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 66709 [details]
Patch

Clearing flags on attachment: 66709

Committed r66879: <http://trac.webkit.org/changeset/66879>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.