Speech Input: wrong position was reported for scolled-down elements

[Hans Wennborg]

Speech Input: wrong position was reported for scolled-down elements

[Hans Wennborg]

Created attachment 157505 [details]
Patch

[Hans Wennborg]

Uploaded as a work-in-progress / call for help.

There are two things I need help with here:

First,

+    FrameView* containingView = renderer()->view()->frameView();
+    FrameView* mainView = document()->page()->mainFrame()->view();
+    IntRect rect = renderer()->absoluteBoundingBoxRect();
+    rect = containingView->contentsToWindow(rect);
+    rect = mainView->rootViewToContents(rect);

This does not really make sense to me. But it does solve the problem: it works when a page is scrolled down, and also when the input element is in a iframe.

It would be great if someone could provide a pointer to how these coordinate systems work.


Second, the layout test doesn't really work. I would expect that the rect that the element passes to WebKit when it is clicked would match the coordinates of the mouse click. I would have thought that the mouse position is relative to the "window" (whatever that means for DumpRenderTree).


Any pointers would be very welcome.

[Kent Tamura]

Comment on attachment 157505 [details]
Patch

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

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:624
>      if (speechInput()->startRecognition(m_listenerId, rect, language, grammar, document()->securityOrigin()))

What coordination do you need here? RootView, window, or screen?

[Hans Wennborg]

(In reply to comment #3)
> (From update of attachment 157505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157505&action=review
> 
> > Source/WebCore/html/shadow/TextControlInnerElements.cpp:624
> >      if (speechInput()->startRecognition(m_listenerId, rect, language, grammar, document()->securityOrigin()))
> 
> What coordination do you need here? RootView, window, or screen?

I think I need window.

I know, the code above doesn't make sense, so any pointers to how these coordinate systems work would be great. E.g. what exactly is the RootView coordinate system?

[Kent Tamura]

Comment on attachment 157505 [details]
Patch

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

>>> Source/WebCore/html/shadow/TextControlInnerElements.cpp:624
>>>      if (speechInput()->startRecognition(m_listenerId, rect, language, grammar, document()->securityOrigin()))
>> 
>> What coordination do you need here? RootView, window, or screen?
> 
> I think I need window.
> 
> I know, the code above doesn't make sense, so any pointers to how these coordinate systems work would be great. E.g. what exactly is the RootView coordinate system?

* Contents: offset from the top-left corner of a rendered Document.  "absolute" in WebCore means this.
* FrameView: offset from the top-left corner of a visible rectangle for a Document.
   FrameView coordinate = contents coordinate + scroll offset
* RootView:  offset from the top-left corner of the content area of your browser tab. RootView is equivalent to FrameView if a Document is a top-level document. For <frame> or <iframe>, its FrameView is different from RootView.
* Window: offset from the top-left corner of a window containing RootView. The window might be a same area with RootView, might be a rectangle containing the tab content + tab decorations, might be a whole 
browser window.

InputTagSpeechDispatcher::startRecognition() in Chromium subtracts scroll offsets from the rect.  It looks wrong.

My recommendation:
We're eliminating window coordinate system in WebCore (Bug 71945).  So the argument of startRecognition() should be in the RootView coordinate system. In this case,
  IntRect rect = document()->view()->contentsToRootView(getPixelSnappedRect());
should work.  See WebCore/html/ColorInputType.cpp and WebCore/html/shadow/CalendarPickerElement.cpp.
  In Chromium WebKit layer, convert the position in RootView coordination system to the screen coordination system by Chrome::rootViewToScreen().
Chromium code should do nothing against the rect.

[Hans Wennborg]

(In reply to comment #5)
> InputTagSpeechDispatcher::startRecognition() in Chromium subtracts scroll offsets from the rect.  It looks wrong.

Ah! This explains a lot of the brokenness :) Thanks for finding it, I'm removing this from the Chromium side.

> My recommendation:
> We're eliminating window coordinate system in WebCore (Bug 71945).  So the argument of startRecognition() should be in the RootView coordinate system. In this case,
>   IntRect rect = document()->view()->contentsToRootView(getPixelSnappedRect());
> should work.  See WebCore/html/ColorInputType.cpp and WebCore/html/shadow/CalendarPickerElement.cpp.
>   In Chromium WebKit layer, convert the position in RootView coordination system to the screen coordination system by Chrome::rootViewToScreen().
> Chromium code should do nothing against the rect.

Thank you so much for the pointers! This fixes everything. Uploading new patch.

[Hans Wennborg]

Created attachment 157700 [details]
Patch

[Satish Sampath]

Comment on attachment 157700 [details]
Patch

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

> LayoutTests/fast/speech/bubble-position-scrolled.html:37
> +    window.scrollAmount = 100;

this variable doesn't seem to be used outside this function. make it a local var?

[Hans Wennborg]

(In reply to comment #8)
> (From update of attachment 157700 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157700&action=review
> 
> > LayoutTests/fast/speech/bubble-position-scrolled.html:37
> > +    window.scrollAmount = 100;
> 
> this variable doesn't seem to be used outside this function. make it a local var?
Done.

tkent: does the patch look OK to you?

[Hans Wennborg]

Created attachment 157940 [details]
Patch

[Kent Tamura]

Comment on attachment 157940 [details]
Patch

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

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:620
> +    IntRect rect = document()->view()->contentsToRootView(getPixelSnappedRect());
>      if (speechInput()->startRecognition(m_listenerId, rect, language, grammar, document()->securityOrigin()))

Let's update the comments for startRecognition()s in WebCore/page/SpeechInputClient.h and WebKit/chromium/public/WebSpeechInputController.h so that they mentions the coordinate system for elementRect.

[Hans Wennborg]

(In reply to comment #11)
> Let's update the comments for startRecognition()s in WebCore/page/SpeechInputClient.h and WebKit/chromium/public/WebSpeechInputController.h so that they mentions the coordinate system for elementRect.
Done.

[Hans Wennborg]

Created attachment 158247 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Kent Tamura]

Comment on attachment 158247 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 158247 [details]
Patch

Clearing flags on attachment: 158247

Committed r125529: <http://trac.webkit.org/changeset/125529>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.