Bug 28921 – Tablet Input Panel Icon does not show up by text fields

• Component: HTML Editing
• Status: ASSIGNED

[Brian Weinstein]

When you focus a text field in WebKit using either your finger or a tablet pen (on a touchscreen/tablet PC), it is expected to see the Tablet Input icon, which will allow you to enter text or data without having to manually un-dock the input panel. This currently does not show up in WebKit.

[Brian Weinstein]

Created attachment 38943 [details]
Fix

[Eric Seidel (no email)]

I guess there is no way to test this on a normal windows machine?  I wonder how chromium will implement this.  They'll probably have to pipe this across the bridge somehow.  Which means they'll probably turn this into a FrameClient method.

[Adam Roben (:aroben)]

Comment on attachment 38943 [details]
Fix

> +#if PLATFORM(WIN)
> +void Frame::updateWindowsSystemCaret()

This function should go in FrameWin.cpp.

> +{
> +    ::DestroyCaret();
> +
> +    Page* page = document()->page();
> +    if (!page || !page->chrome()->platformWindow())
> +        return;
> +
> +    AccessibilityObject* focusedObject = document()->axObjectCache()->focusedUIElementForPage(page);
> +    if (!focusedObject || !focusedObject->isTextControl())
> +        return;

It seems strange to use the accessibility code for this. Can we figure this out some other way?

> +    PlatformWidget widget = page->chrome()->platformWindow();
> +    if (!widget)
> +        return;
> +
> +    HBITMAP caretBitmap = ::CreateBitmap(1, 1, 1, 1, NULL);

Can we use a 0x0 bitmap instead of a 1x1 bitmap? It seems like a 1x1 bitmap will make 1 pixel on the screen blink. MSDN seems to indicate that a 0x0 bitmap should work.

> +    ::CreateCaret(widget, caretBitmap, 1, 1);
> +    ::ShowCaret(widget);
> +
> +    IntRect rect = selection()->absoluteCaretBounds();
> +
> +    ::SetCaretPos(rect.topLeft().x() - view()->scrollX(),
> +        rect.topLeft().y() - view()->scrollY());

There's no need for the .topLeft() calls here. Can we use something like contentsToWindow here?

MSDN says multiple times that "A window should create a caret only when it has the keyboard focus or is active. The window should destroy the caret before losing the keyboard focus or becoming inactive." Does your code guarantee this?

[Adam Roben (:aroben)]

<rdar://problem/5272138>

[Brian Weinstein]

(In reply to comment #3)
> (From update of attachment 38943 [details])
> > +#if PLATFORM(WIN)
> > +void Frame::updateWindowsSystemCaret()
> 
> This function should go in FrameWin.cpp.
> 
> > +{
> > +    ::DestroyCaret();
> > +
> > +    Page* page = document()->page();
> > +    if (!page || !page->chrome()->platformWindow())
> > +        return;
> > +
> > +    AccessibilityObject* focusedObject = document()->axObjectCache()->focusedUIElementForPage(page);
> > +    if (!focusedObject || !focusedObject->isTextControl())
> > +        return;
> 
> It seems strange to use the accessibility code for this. Can we figure this out
> some other way? 

Yeah, I can just use the documents focused node, I switched over to using that.


> > +    PlatformWidget widget = page->chrome()->platformWindow();
> > +    if (!widget)
> > +        return;
> > +
> > +    HBITMAP caretBitmap = ::CreateBitmap(1, 1, 1, 1, NULL);
> 
> Can we use a 0x0 bitmap instead of a 1x1 bitmap? It seems like a 1x1 bitmap
> will make 1 pixel on the screen blink. MSDN seems to indicate that a 0x0 bitmap
> should work.

Done.

> 
> > +    ::CreateCaret(widget, caretBitmap, 1, 1);
> > +    ::ShowCaret(widget);
> > +
> > +    IntRect rect = selection()->absoluteCaretBounds();
> > +
> > +    ::SetCaretPos(rect.topLeft().x() - view()->scrollX(),
> > +        rect.topLeft().y() - view()->scrollY());
> 
> There's no need for the .topLeft() calls here. Can we use something like
> contentsToWindow here?
> 

Done.

> MSDN says multiple times that "A window should create a caret only when it has
> the keyboard focus or is active. The window should destroy the caret before
> losing the keyboard focus or becoming inactive." Does your code guarantee this?

This is the part I was struggling with. I was trying to find the correct place to put this function call that means it would get called:

1) Whenever focus jumps from one WebView to another
2) Whenever focus jumps from one node to another in the same WebView.

And this all has to be done after the page has been laid out, or else the icon will end up in the wrong place.

Now that the code is in Frame, that gives me a lot of flexibility where to call it from, but I'm not sure the best place/places to put the call.

[Brian Weinstein]

Created attachment 39095 [details]
Updated Fix

[Adam Roben (:aroben)]

Comment on attachment 39095 [details]
Updated Fix

> +    Node* focusedNode = document()->focusedNode();
> +    if (!focusedNode || !focusedNode->renderer() 
> +        || !(focusedNode->renderer()->isTextField() || focusedNode->renderer()->isTextArea()))
> +        return;

What about something like <div contenteditable>?

> +    HBITMAP caretBitmap = ::CreateBitmap(0, 0, 1, 1, NULL);
> +    ::CreateCaret(widget, caretBitmap, 0, 0);
> +    ::ShowCaret(widget);
> +
> +    IntPoint contentPoint = selection()->absoluteCaretBounds().topLeft();
> +    
> +    IntPoint windowPoint = view()->contentsToWindow(contentPoint);
> +    ::SetCaretPos(windowPoint.x(), windowPoint.y());
> +
> +    ::DeleteObject(caretBitmap);

Should we just cache the empty bitmap? It seems kind of silly to create/destroy it every time the caret moves.

Does updateWindowsSystemCaret get called in response to WM_SETFOCUS/WM_KILLFOCUS? I'm still worried we aren't calling it at the right times.

[Brian Weinstein]

Created attachment 46874 [details]
[PATCH] Fix with Cached Bitmap + Support for ContentEditable

[WebKit Review Bot]

Attachment 46874 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/win/WebView.cpp:89:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/page/win/FrameWin.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/page/win/FrameWin.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/page/win/FrameWin.cpp:128:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/page/win/FrameWin.cpp:129:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/page/win/FrameWin.cpp:129:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/page/Frame.h:241:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 7


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Weinstein]

Created attachment 46875 [details]
[PATCH] Fix + Style bot happy

[WebKit Review Bot]

Attachment 46875 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/page/win/FrameWin.cpp:129:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brian Weinstein]

Created attachment 46878 [details]
[PATCH] Fix + Last Style Nit

[Adam Barth]

Comment on attachment 46878 [details]
[PATCH] Fix + Last Style Nit

This has been sitting here since January.  I'm marking r- because there is no test.  Please at least add a manual test.

aroben seems to have been doing the earlier review.  Please feel free to override my r- if you think a test isn't needed here.  Mostly, I don't think it's healthy to have patches sitting in pending-review for months.