Bug 76914 – [Qt] Implement tap feedback respecting -webkit-tap-highlight-color

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 123736 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'ManualTests/qt/tap-highlight..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebPage/TapHighlightController.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebPage/TapHighlightController.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/page/GestureTapHighlighter.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/page/GestureTapHighlighter.cpp:48:  Missing space before {  [whitespace/braces] [5]
Total errors found: 6 in 15 files


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

[Kenneth Rohde Christiansen]

Created attachment 123740 [details]
Patch

[Antonio Gomes]

Comment on attachment 123736 [details]
Patch

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

nits:

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:100
> +

unneeded new line?

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:70
> +        m_webPage->uninstallPageOverlay(m_overlay, true);

/* what true means */

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:81
> +    if (webPage)
> +        return;

just curious: !webPage?

also is it different from m_webpage?

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:100
> +bool TapHighlightController::mouseEvent(PageOverlay* pageOverlay, const WebMouseEvent& mouseEvent)

maybe omit the parameters?

[Kenneth Rohde Christiansen]

Uh this wasnt up for review yet but thanks :-)

(In reply to comment #4)
> (From update of attachment 123736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123736&action=review
> 
> nits:
> 
> > Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:100
> > +
> 
> unneeded new line?
> 
> > Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:70
> > +        m_webPage->uninstallPageOverlay(m_overlay, true);
> 
> /* what true means */
> 
> > Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:81
> > +    if (webPage)
> > +        return;
> 
> just curious: !webPage?

No it is moving to another page, this code is two other places in webkit2

> also is it different from m_webpage?
> 
> > Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:100
> > +bool TapHighlightController::mouseEvent(PageOverlay* pageOverlay, const WebMouseEvent& mouseEvent)
> 
> maybe omit the parameters?

[Kenneth Rohde Christiansen]

Created attachment 123744 [details]
Patch

[Antonio Gomes]

question: when you have a link that wraps line, being the beginning of it at the right of the page, and its wrapped content at the left of it at the line below, do you get the bounding box of the link? it would show a very big tap highlight...

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> question: when you have a link that wraps line, being the beginning of it at the right of the page, and its wrapped content at the left of it at the line below, do you get the bounding box of the link? it would show a very big tap highlight...

That is not handled yet. There are also a few other things that I would like to treat as follow-ups. You are of course very welcome to help me with that :-)

[Simon Hausmann]

Comment on attachment 123744 [details]
Patch

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

I think this is a nice solution! (nicer than what we have in Grob :)

However there's one thing that was better in Grob compared to the way it's done here: The fade animation in Grob was just the result of composing the "highlight" pixmap with a different opacity, i.e. the animation required only composition and no re-rendering of content/texture-upload. The current way the page overlay implements the fade animation requires constant re-painting of the tiles AFAICS.

Although outside of the scope of this patch directly, in an ideal world we'd apply the same technique to this: If supported by the overlay (client), the content could be rendered into a layer once and the animation could be implemented via AC.

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:96
> +void TapHighlightController::drawRect(PageOverlay* pageOverlay, GraphicsContext& graphicsContext, const IntRect& dirtyRect)
> +{
> +    if (!m_node)
> +        return;
> +
> +    GestureTapHighlighter::drawHighlight(&graphicsContext, m_node);
> +}

I think for the fade animation to work, you need to respect pageOverlay->fractionFadedIn();

[Kenneth Rohde Christiansen]

Created attachment 123907 [details]
Patch

Now with animations

[Kenneth Rohde Christiansen]

> However there's one thing that was better in Grob compared to the way it's done here: The fade animation in Grob was just the result of composing the "highlight" pixmap with a different opacity, i.e. the animation required only composition and no re-rendering of content/texture-upload. The current way the page overlay implements the fade animation requires constant re-painting of the tiles AFAICS.
> 
> Although outside of the scope of this patch directly, in an ideal world we'd apply the same technique to this: If supported by the overlay (client), the content could be rendered into a layer once and the animation could be implemented via AC.

Yes, it would be the proper way to do this. Maybe No'am can have a look or help me when we are in Hungary.

[Kenneth Rohde Christiansen]

Created attachment 123921 [details]
Patch

[Simon Hausmann]

Comment on attachment 123921 [details]
Patch

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

> Source/WebCore/page/GestureTapHighlighter.cpp:153
> +void drawHighlight(GraphicsContext* context, Node* node)

I suggest to change the API to just return you the path for the highlight. Then the caller can call it once on initialization time (the node doesn't change after all) and then decide how to continously draw the path with the fade animation, instead of calculating the same path for every rendered frame.

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.h:62
> +    WebCore::Node* m_node;

Is it intentional to store the Node as raw pointer instead of RefPtr? It seems the highlight controller will dereference node many times during the animation, and during that time the node could easily get deleted, no?

But I would suggest to avoid storing the Node* altogether and instead calculate the path you need for highlighting once on initialization time and then store that instead.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1434
> +    Node* activationNode = 0;
> +    Frame* mainframe = m_page->mainFrame();
> +
> +    if (point != IntPoint::zero()) {
> +        HitTestResult result = mainframe->eventHandler()->hitTestResultAtPoint(mainframe->view()->windowToContents(point), /*allowShadowContent*/ false, /*ignoreClipping*/ true);
> +        activationNode = result.innerNode();

You could add a // ### FIXME to point out that when using touch we need to do touch adjustment here to locate the correct node.

[Kenneth Rohde Christiansen]

Created attachment 123929 [details]
Patch

Changed to incorporate some of Simon's ideas

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'ManualTests/qt/tap-highlight..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/TapHighlightController.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


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

[Kenneth Rohde Christiansen]

Created attachment 123936 [details]
Patch

ChangeLogs method listing will be updated before any commit.

[Kenneth Rohde Christiansen]

Created attachment 123941 [details]
Patch

[WebKit Review Bot]

Comment on attachment 123941 [details]
Patch

Rejecting attachment 123941 [details] from commit-queue.

New failing tests:
media/audio-garbage-collect.html
Full output: http://queues.webkit.org/results/11242819

[Kenneth Rohde Christiansen]

Comment on attachment 123941 [details]
Patch

Must be flaky, resetting

[WebKit Review Bot]

Comment on attachment 123941 [details]
Patch

Clearing flags on attachment: 123941

Committed r105893: <http://trac.webkit.org/changeset/105893>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.