Bug 84936 – [chromium] Compute highlight for touch targets

• Component: New Bugs
• Status: RESOLVED WORKSFORME

[Tien-Ren Chen]

[chromium] Compute highlight for touch targets

[Tien-Ren Chen]

Created attachment 138954 [details]
Patch

[Tien-Ren Chen]

Created attachment 138956 [details]
A few basic test cases

[Tien-Ren Chen]

I couldn't find chromium account for the others, could you help me CC them?

This is an early WIP. I did open some pages and hover over some links to make sure it doesn't crash, but the actual generated paths are not verified. I believe there are still some bugs need to be fixed before requesting for review.

Also there are still some design concerns:

1. When do we generate the highlight, and how do we attach them to the GraphicsLayer?
The best timing is probably do it when painting the texture, but is it safe to keep a WebCore::Node* reference to the touch target? The node can be removed by javascript in the meantime.

2. How do we remove the highlight from the GraphicsLayer?

3. We probably want to refactor RenderInline::absoluteQuads.
I added another virtual function RenderObject::containerQuads that works similar to absoluteQuads except that it return the quads in a container RenderBoxModelObject's cords instead of the absolute cords. For RenderInline it is generating too much duplicate code. Better make RenderObject::absoluteQuads non-virtual and just call into containerQuads with NULL container.

4. Need to make the function more efficient.
It probably doesn't need to generate quads all descendant nodes... For example if there is a <div style="overflow:hidden"> then all its descendants can be skipped (except for fixed/absolute positioned descendant). I'm still thinking a clean way to do this.

5. absoluteQuads is probably not the perfect way to determine hit test region. There is an interesting example. See the first and second touch target in the attached example. The space between "Hello" and "link" is still occupied by the line box of the relative-positioned <div>, but it is only clickable when applied with a background color. Can't figure out how to do this right. :(

By the way there is an interesting bug in WebKit rendering. See the 4th touch target in the example. If you click on it, the blue box can escape from its parent's clipping.

[W. James MacLean]

(In reply to comment #3)
> I couldn't find chromium account for the others, could you help me CC them?
> 
> This is an early WIP. I did open some pages and hover over some links to make sure it doesn't crash, but the actual generated paths are not verified. I believe there are still some bugs need to be fixed before requesting for review.
> 
> Also there are still some design concerns:
> 
> 1. When do we generate the highlight, and how do we attach them to the GraphicsLayer?
> The best timing is probably do it when painting the texture, but is it safe to keep a WebCore::Node* reference to the touch target? The node can be removed by javascript in the meantime.

This is being worked out in https://bugs.webkit.org/show_bug.cgi?id=84487

> 2. How do we remove the highlight from the GraphicsLayer?

Ditto.

> 3. We probably want to refactor RenderInline::absoluteQuads.
> I added another virtual function RenderObject::containerQuads that works similar to absoluteQuads except that it return the quads in a container RenderBoxModelObject's cords instead of the absolute cords. For RenderInline it is generating too much duplicate code. Better make RenderObject::absoluteQuads non-virtual and just call into containerQuads with NULL container.
> 
> 4. Need to make the function more efficient.
> It probably doesn't need to generate quads all descendant nodes... For example if there is a <div style="overflow:hidden"> then all its descendants can be skipped (except for fixed/absolute positioned descendant). I'm still thinking a clean way to do this.
> 
> 5. absoluteQuads is probably not the perfect way to determine hit test region. There is an interesting example. See the first and second touch target in the attached example. The space between "Hello" and "link" is still occupied by the line box of the relative-positioned <div>, but it is only clickable when applied with a background color. Can't figure out how to do this right. :(
> 
> By the way there is an interesting bug in WebKit rendering. See the 4th touch target in the example. If you click on it, the blue box can escape from its parent's clipping.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 9 files


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

[Build Bot]

Comment on attachment 138954 [details]
Patch

Attachment 138954 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12547008

[WebKit Review Bot]

Comment on attachment 138954 [details]
Patch

Attachment 138954 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12548064

[W. James MacLean]

Comment on attachment 138954 [details]
Patch

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

Overall the code seems to provide good targets, and does a good job generating the outline paths. Let's keep at this!

> Source/WebKit/chromium/src/WebViewImpl.cpp:3561
> +        SkPath* path = highlight->get(graphicsLayer);

I would prefer if we can use Path here, as it's more general and is used by the proposed patch for drawing the highlight layers.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3586
> +            path->moveTo(quad.p1().x(), quad.p1().y());

Can use Path::addRect(const FloatRect&) here, it's simpler. Can we add an option to call Path::addRoundedRect(), perhaps as an argument to this function?

> Source/WebKit/chromium/src/WebViewImpl.h:590
> +    typedef HashMap<WebCore::GraphicsLayer*, OwnPtr<SkPath> > HighlightMap;

Any reason we can't store RefPtr<GraphicsLayer> instead? This way the timer that controls the highlight animation (and which is the ultimate consumer of this data), doesn't have to worry about pointers going stale before the data is used.

[Tien-Ren Chen]

(In reply to comment #8)
> (From update of attachment 138954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138954&action=review
> 
> Overall the code seems to provide good targets, and does a good job generating the outline paths. Let's keep at this!

I saw your patch that uses GestureTapHighlighter, that is actually a very interesting piece of code. I think we'd rather try to extend it instead of invent our own stuff.
Be more specific these are the 2 functionality we want to add:

1. To support transformed elements, using the Quads family functions. IMO all the Rects family should be deprecated sooner or later...
2. Support compositor side scrolling/animation. That is, the highlights should be assigned to individual GraphicsLayers with layer-relative cords. Example:
<a href="">
<div style="width:100px;height:100px"/>
<div style="width:100px;height:100px;-webkit-transform:translateZ(0);-webkit-animation-name:someanimation"/>
</a>

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3561
> > +        SkPath* path = highlight->get(graphicsLayer);
> 
> I would prefer if we can use Path here, as it's more general and is used by the proposed patch for drawing the highlight layers.

Sure I will upload a new patch uses WebCore::Path.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:3586
> > +            path->moveTo(quad.p1().x(), quad.p1().y());
> 
> Can use Path::addRect(const FloatRect&) here, it's simpler. Can we add an option to call Path::addRoundedRect(), perhaps as an argument to this function?

It won't work for transformed touch target. For example: <div style="width:100px;height:100px;-webkit-transform:rotated(30deg)"></div>
Only 3D-transformed render objects will create its own GraphicsLayer.

> > Source/WebKit/chromium/src/WebViewImpl.h:590
> > +    typedef HashMap<WebCore::GraphicsLayer*, OwnPtr<SkPath> > HighlightMap;
> 
> Any reason we can't store RefPtr<GraphicsLayer> instead? This way the timer that controls the highlight animation (and which is the ultimate consumer of this data), doesn't have to worry about pointers going stale before the data is used.

I wanted to, but GraphicsLayer is not ref-counted, and modifying all existing code to use RefPtr will be a pain. :X

[W. James MacLean]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 138954 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138954&action=review
> > 
> > Overall the code seems to provide good targets, and does a good job generating the outline paths. Let's keep at this!
> 
> I saw your patch that uses GestureTapHighlighter, that is actually a very interesting piece of code. I think we'd rather try to extend it instead of invent our own stuff.

That patch has changed a lot. First, using the existing code gave very poor targets in my opinion. Your code worked quite nicely for me though. I may try again with the touch highlight region from my patch, and your node selector.

> Be more specific these are the 2 functionality we want to add:
> 
> 1. To support transformed elements, using the Quads family functions. IMO all the Rects family should be deprecated sooner or later...
> 2. Support compositor side scrolling/animation. That is, the highlights should be assigned to individual GraphicsLayers with layer-relative cords. Example:
> <a href="">
> <div style="width:100px;height:100px"/>
> <div style="width:100px;height:100px;-webkit-transform:translateZ(0);-webkit-animation-name:someanimation"/>
> </a>
> 
> > > Source/WebKit/chromium/src/WebViewImpl.cpp:3561
> > > +        SkPath* path = highlight->get(graphicsLayer);
> > 
> > I would prefer if we can use Path here, as it's more general and is used by the proposed patch for drawing the highlight layers.
> 
> Sure I will upload a new patch uses WebCore::Path.

Great, thanks!

> > > Source/WebKit/chromium/src/WebViewImpl.cpp:3586
> > > +            path->moveTo(quad.p1().x(), quad.p1().y());
> > 
> > Can use Path::addRect(const FloatRect&) here, it's simpler. Can we add an option to call Path::addRoundedRect(), perhaps as an argument to this function?
> 
> It won't work for transformed touch target. For example: <div style="width:100px;height:100px;-webkit-transform:rotated(30deg)"></div>
> Only 3D-transformed render objects will create its own GraphicsLayer.
> 
> > > Source/WebKit/chromium/src/WebViewImpl.h:590
> > > +    typedef HashMap<WebCore::GraphicsLayer*, OwnPtr<SkPath> > HighlightMap;
> > 
> > Any reason we can't store RefPtr<GraphicsLayer> instead? This way the timer that controls the highlight animation (and which is the ultimate consumer of this data), doesn't have to worry about pointers going stale before the data is used.
> 
> I wanted to, but GraphicsLayer is not ref-counted, and modifying all existing code to use RefPtr will be a pain. :X

OK, so we need to figure out how to work around this then. Is there anything that points to the graphics layer that is ref-counted, that we could store instead?

[Tien-Ren Chen]

Created attachment 139605 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 12 files


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

[Build Bot]

Comment on attachment 139605 [details]
Patch

Attachment 139605 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12593519

[Tien-Ren Chen]

I won't be in the office until Friday. I will try to keep in touch by email but I won't have too much time to work on code.

I changed the patch a little bit to make the highlight generation code a separate class. It is very likely I will have to improve the implementation later, but the interface will stay the same.

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 138954 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=138954&action=review
> > > 
> > > Overall the code seems to provide good targets, and does a good job generating the outline paths. Let's keep at this!
> > 
> > I saw your patch that uses GestureTapHighlighter, that is actually a very interesting piece of code. I think we'd rather try to extend it instead of invent our own stuff.
> 
> That patch has changed a lot. First, using the existing code gave very poor targets in my opinion. Your code worked quite nicely for me though. I may try again with the touch highlight region from my patch, and your node selector.

The node selector from my patch is very minimal. We have some disambiguation logic in downstream code that I don't 100% understand yet. I'll try to upstream those incrementally in later patches.

As for the highlight region, I think addFocusRingRects is actually very similar to our implementation, except that it uses absolute rects (thus can behave very wrong when transformation is used).

> > > > Source/WebKit/chromium/src/WebViewImpl.h:590
> > > > +    typedef HashMap<WebCore::GraphicsLayer*, OwnPtr<SkPath> > HighlightMap;
> > > 
> > > Any reason we can't store RefPtr<GraphicsLayer> instead? This way the timer that controls the highlight animation (and which is the ultimate consumer of this data), doesn't have to worry about pointers going stale before the data is used.
> > 
> > I wanted to, but GraphicsLayer is not ref-counted, and modifying all existing code to use RefPtr will be a pain. :X
> 
> OK, so we need to figure out how to work around this then. Is there anything that points to the graphics layer that is ref-counted, that we could store instead?

I don't think anything in the render tree is ref-counted.

LayerChromium is ref-counted but it doesn't back-reference to GraphicsLayer. One possibility is to do everything on LayerChromium. That is what I did with the compositor scrollbar layers.

[WebKit Review Bot]

Comment on attachment 139605 [details]
Patch

Attachment 139605 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12591519

[Tien-Ren Chen]

Created attachment 140662 [details]
Patch

[Build Bot]

Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12650253

[WebKit Review Bot]

Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12651249

[Early Warning System Bot]

Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12652264

[Early Warning System Bot]

Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12656212

[Build Bot]

Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12645278

[Gyuyoung Kim]

Comment on attachment 140662 [details]
Patch

Attachment 140662 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12654215

[Tom Zakrajsek]

Comment on attachment 140662 [details]
Patch

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

> Source/WebCore/rendering/RenderInline.cpp:1495
> +    generateLineBoxRects(RepaintContainerHighlightQuadsGeneratorContext(this, quads, repaintContainer));

generateLineBoxRects isn't defined anywhere.  Breaks all builds.

[Tien-Ren Chen]

(In reply to comment #23)
> (From update of attachment 140662 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140662&action=review
> 
> > Source/WebCore/rendering/RenderInline.cpp:1495
> > +    generateLineBoxRects(RepaintContainerHighlightQuadsGeneratorContext(this, quads, repaintContainer));
> 
> generateLineBoxRects isn't defined anywhere.  Breaks all builds.

Yes, waiting for 85725 to land

[Tien-Ren Chen]

Created attachment 141314 [details]
Patch

[Tien-Ren Chen]

Nothing changed. Just rebase to the latest version.

[Build Bot]

Comment on attachment 141314 [details]
Patch

Attachment 141314 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12677073

[Tien-Ren Chen]

Created attachment 141485 [details]
Patch

[Tien-Ren Chen]

Created attachment 143183 [details]
Patch

[Tien-Ren Chen]

This new version is based on the recent version of https://bugs.webkit.org/show_bug.cgi?id=84487
(https://bugs.webkit.org/attachment.cgi?id=142729)

Provides much more accurate highlight for many test cases.

[Tien-Ren Chen]

Created attachment 143184 [details]
some additional test cases

[Adam Barth]

@trchen: I'm hoping Eric or jchaffraix will review your patch.

[Julien Chaffraix]

Comment on attachment 143183 [details]
Patch

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

I will only comment on the general bits in the rendering which are clunky. Added the compositor people to look at what are the implication for the compositor's and what you need to do.

> Source/WebCore/ChangeLog:14
> +        One important difference from RenderObject::absoluteQuads is that
> +        the returned quads will be in the coordinates of a repaint container
> +        specified by the caller. This will allow threaded-scrolling/animation
> +        without having to regenerate the highlights.

What happens if your repaintContainer change? (layer removed or added for example or promotion / demotion from being hardware accelerated)

> Source/WebCore/ChangeLog:17
> +        No new tests. Tests will be added with later patches that actually
> +        add highlight to layers.

You are exposing the new function to Chromium. Will it be tested there?

What do you mean by layers: RenderLayers or GraphicsLayers?

> Source/WebCore/rendering/RenderBlock.h:450
> +    virtual void repaintContainerHighlightQuads(Vector<FloatQuad>&, RenderBoxModelObject* repaintContainer) const;

Not a huge fan of this function name: I thought it was a repaint* function (which obviously it's not). How about collectHighlightQuadsForContainer(...) or something similar?

> Source/WebCore/rendering/RenderBox.cpp:596
> +    if (!size().isEmpty())

This should be an early return. I also find this check inconsistent with the rest of the code (you would not check the other classes for empty quads?)

> Source/WebCore/rendering/RenderBox.cpp:597
> +        quads.append(localToContainerQuad(FloatRect(0, 0, width(), height()), repaintContainer, false, 0));

You don't need the 2 last parameters here. They make the code less readable and more confusing as you wonder why you need them here. Same comment later at the other call sites.

> Source/WebCore/rendering/RenderBox.h:155
> +    virtual void repaintContainerHighlightQuads(Vector<FloatQuad>&, RenderBoxModelObject* repaintContainer) const;

We annotate those functions with OVERRIDE (not repeated).

> Source/WebCore/rendering/RenderInline.cpp:1465
> +

This is inconsistent: you don't call your parent class' method here.

> Source/WebCore/rendering/RenderInline.cpp:1467
> +    if (continuation())
> +        continuation()->repaintContainerHighlightQuads(quads, repaintContainer);

continuation is RenderBoxModelObject concept which makes me dubious of the duplication between RenderInline and RenderBlock: there should be a RenderBoxModelObject function that handles the continuation for you.

> Source/WebKit/chromium/src/WebHighlight.cpp:54
> +    for (Node *node = topNode; node; node = node->traverseNextNode(topNode)) {

Style violation

> Source/WebKit/chromium/src/WebHighlight.cpp:57
> +        if (!renderer)
> +            continue;

My question here: have you considered SVG? Do you want to support it or not?

If you don't care about SVG, you don't care about anything that is not a RenderBoxModelObject anyway. That would simplify the code above.

> Source/WebKit/chromium/src/WebHighlight.cpp:59
> +        RenderLayer* repaintLayer = renderer->enclosingLayer()->enclosingCompositingLayerForRepaint();

NO! Using RenderLayer in the WebKit/ directory is a show-stopper. It means you need access to something you shouldn't be knowing about.

RenderBoxModelObject* repaintContainer = renderer->containerForRepaint();

Will give you the renderer you need. If you need more of the hidden bits, it looks like you should rethink your architecture.

> Source/WebKit/chromium/src/WebHighlight.cpp:65
> +        GraphicsLayer* graphicsLayer = repaintLayer->backing()->graphicsLayer();

What proofs do you have that this RenderLayer is hardware accelerated? If it is not, this is a NULL crasher.

> Source/WebKit/chromium/src/WebHighlight.cpp:68
> +        Path* path = m_quadMap.get(graphicsLayer);
> +        if (!path)
> +            m_quadMap.set(graphicsLayer, adoptPtr(path = new Path()));

This is super confusing and inefficient as you are doing 2 hash lookups if you have no entry:

Path* path = m_quadMap.add(graphicsLayer, adoptPtr(new Path())).iterator->second;

> Source/WebKit/chromium/src/WebHighlight.cpp:73
> +        for (size_t i = 0; i < newquads.size(); i++) {

I don't understand the purpose of this. If you need a Path can't you get one from WebCore directly instead of this hocus pocus code?

[Tien-Ren Chen]

(In reply to comment #33)
> (From update of attachment 143183 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143183&action=review
> 
> I will only comment on the general bits in the rendering which are clunky. Added the compositor people to look at what are the implication for the compositor's and what you need to do.

Thanks for the valuable comments!

> > Source/WebCore/ChangeLog:14
> > +        One important difference from RenderObject::absoluteQuads is that
> > +        the returned quads will be in the coordinates of a repaint container
> > +        specified by the caller. This will allow threaded-scrolling/animation
> > +        without having to regenerate the highlights.
> 
> What happens if your repaintContainer change? (layer removed or added for example or promotion / demotion from being hardware accelerated)

Yes that would be a problem. The current implementation generates the highlight at the time a tap gesture happened, then it stays the same no matter the nodes are moved / transformed / zapped / whatever. Ideally we should store a reference to the node, then regenerate the highlight on every compositor commit. I doubt if it'd be an overkill though.

> > Source/WebCore/ChangeLog:17
> > +        No new tests. Tests will be added with later patches that actually
> > +        add highlight to layers.
> 
> You are exposing the new function to Chromium. Will it be tested there?
> 
> What do you mean by layers: RenderLayers or GraphicsLayers?

Sorry the comment no longer applies. I'll add tests in revised patch.

> > Source/WebCore/rendering/RenderBlock.h:450
> > +    virtual void repaintContainerHighlightQuads(Vector<FloatQuad>&, RenderBoxModelObject* repaintContainer) const;
> 
> Not a huge fan of this function name: I thought it was a repaint* function (which obviously it's not). How about collectHighlightQuadsForContainer(...) or something similar?

Totally agree... Another thing aelias@ is suggesting that what the function actually does is to mimic the hit-test area, and later we might use it to generate the non-fast touch events region for threaded input event handling. So maybe collectHittestQuadsForContainer would be a better name?

> > Source/WebCore/rendering/RenderBox.cpp:596
> > +    if (!size().isEmpty())
> 
> This should be an early return. I also find this check inconsistent with the rest of the code (you would not check the other classes for empty quads?)

Empty quad probably doesn't matter, but I'll add check for all of them for the sake of consistency.

> > Source/WebCore/rendering/RenderBox.cpp:597
> > +        quads.append(localToContainerQuad(FloatRect(0, 0, width(), height()), repaintContainer, false, 0));
> 
> You don't need the 2 last parameters here. They make the code less readable and more confusing as you wonder why you need them here. Same comment later at the other call sites.
> 
> > Source/WebCore/rendering/RenderBox.h:155
> > +    virtual void repaintContainerHighlightQuads(Vector<FloatQuad>&, RenderBoxModelObject* repaintContainer) const;
> 
> We annotate those functions with OVERRIDE (not repeated).

Done

> > Source/WebCore/rendering/RenderInline.cpp:1465
> > +
> 
> This is inconsistent: you don't call your parent class' method here.

I don't understand this comment. Could you explain in detail?

> > Source/WebCore/rendering/RenderInline.cpp:1467
> > +    if (continuation())
> > +        continuation()->repaintContainerHighlightQuads(quads, repaintContainer);
> 
> continuation is RenderBoxModelObject concept which makes me dubious of the duplication between RenderInline and RenderBlock: there should be a RenderBoxModelObject function that handles the continuation for you.

I don't think there is one, as absoluteRects and absoluteQuads use the same practice. continuation() is declared as protected instead of private, I guess that means subclasses can be aware of it?

> > Source/WebKit/chromium/src/WebHighlight.cpp:54
> > +    for (Node *node = topNode; node; node = node->traverseNextNode(topNode)) {
> 
> Style violation

Done

> > Source/WebKit/chromium/src/WebHighlight.cpp:57
> > +        if (!renderer)
> > +            continue;
> 
> My question here: have you considered SVG? Do you want to support it or not?
> 
> If you don't care about SVG, you don't care about anything that is not a RenderBoxModelObject anyway. That would simplify the code above.

Currently I don't care about SVG as I almost never see them used on the internet... But I'll say we'd like to support at a later time when we have more time budget.

> > Source/WebKit/chromium/src/WebHighlight.cpp:59
> > +        RenderLayer* repaintLayer = renderer->enclosingLayer()->enclosingCompositingLayerForRepaint();
> 
> NO! Using RenderLayer in the WebKit/ directory is a show-stopper. It means you need access to something you shouldn't be knowing about.
> 
> RenderBoxModelObject* repaintContainer = renderer->containerForRepaint();
> 
> Will give you the renderer you need. If you need more of the hidden bits, it looks like you should rethink your architecture.

Ah ha, this is exactly what I need. Thanks!

> > Source/WebKit/chromium/src/WebHighlight.cpp:65
> > +        GraphicsLayer* graphicsLayer = repaintLayer->backing()->graphicsLayer();
> 
> What proofs do you have that this RenderLayer is hardware accelerated? If it is not, this is a NULL crasher.

Because it's returned from enclosingCompositingLayerForRepaint(), either it is NULL or it has a backing?

> > Source/WebKit/chromium/src/WebHighlight.cpp:68
> > +        Path* path = m_quadMap.get(graphicsLayer);
> > +        if (!path)
> > +            m_quadMap.set(graphicsLayer, adoptPtr(path = new Path()));
> 
> This is super confusing and inefficient as you are doing 2 hash lookups if you have no entry:
> 
> Path* path = m_quadMap.add(graphicsLayer, adoptPtr(new Path())).iterator->second;

This one allocates a new Path even when there is existing entry. :(

How about this?
HashMap<...>::iterator i = m_quadMap.add(graphicsLayer, adoptPtr(new Path())).iterator;
if (!i->second)
    i->second = new Path();
Path* path = i->second;

> > Source/WebKit/chromium/src/WebHighlight.cpp:73
> > +        for (size_t i = 0; i < newquads.size(); i++) {
> 
> I don't understand the purpose of this. If you need a Path can't you get one from WebCore directly instead of this hocus pocus code?

What I'm thinking is that the WebCore function should only return the exact hit test area, while the WebKit function adds decorations (like round smooth corners), and try to do some basic clipping.

[Alexandre Elias]

(In reply to comment #33)
> > Source/WebKit/chromium/src/WebHighlight.cpp:73
> > +        for (size_t i = 0; i < newquads.size(); i++) {
> 
> I don't understand the purpose of this. If you need a Path can't you get one from WebCore directly instead of this hocus pocus code?

I had a chat offline with Tien-Ren about this.  First, I think we want the WebCore part to return rects because it's not guaranteed that the highlight rect generation will always support them (personally I think it's overkill and we may want to strip out Path support at some point).

But I think you have a point.  As Tien-Ren mentioned above, I think we should rename "repaintContainerHighlightQuads()" to "hitRectContainerQuads()" as ultimately what we want to highlight is the hit rect area of the element.  Moreover, this function will be usable in the future for the touch event quick-reject feature that we've been discussing.  Then, we can move the bulk of the hocus pocus -- the clipping for overflow: hidden elements -- into WebCore, since that logic matches the "hit rect" concept.

[Tien-Ren Chen]

Created attachment 143728 [details]
Patch

[Tien-Ren Chen]

The new patch fixed some of the issue addressed by jchaffraix@.

Still need to add new test and update the existing LinkHighlightTest.

By the way how do I tell the script to regenerate the modification list in ChangeLog?

[Adam Barth]

> By the way how do I tell the script to regenerate the modification list in ChangeLog?

There isn't a very good way to do that.  One thing you can do is to run ./Tools/Scripts/prepare-ChangeLog manually, which will add a new ChangeLog entry to the top of the file.  You'll then need to merge the two entries manually.

[Tien-Ren Chen]

Created attachment 144645 [details]
Patch

[Adam Barth]

@jchaffraix: Are you happy with this patch now?

[Julien Chaffraix]

Comment on attachment 144645 [details]
Patch

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

Not good yet. The patch is too massive to be properly reviewed, please find some ways to split it as it would help to get it landed: I couldn't look at everything due to the size.

> Source/WebCore/ChangeLog:9
> +        to generate the clickable region for link highlighting.

If it's only for link, it should be more explicit in the naming.

> Source/WebCore/rendering/RenderObject.cpp:2867
> +void RenderObject::hitQuadsForContainerWithClipping(Vector<FloatQuad>& quads, RenderBoxModelObject* repaintContainer) const

The naming could really use a *verb* and be more precise. I don't understand what the 'with clipping' means, if you mean clipped then please use 'clipped' instead (which would match the rest of the code, e.g. clippedOverflowRectForRepaint (which may do something similar to what you are doing btw, I don't have the strength of looking into that)).

> Source/WebCore/rendering/RenderObject.cpp:2869
> +    size_t oldSize = quads.size();

This is useless as quads is always empty when the function is called.

> Source/WebCore/rendering/RenderObject.cpp:2880
> +        for (RenderBlock* blk = containingBlock(); blk && blk->isDescendantOf(repaintContainer); blk = blk->containingBlock()) {

I hope you understand this is a quadratic operation thanks to the isDescendantOf call and because it's in another loop it's horribly slow. 2 ways to make it less sucky:
* look-ahead and you cache the top-most containing block to avoid the isDescendantOf() call.
* cache the whole containing block chain (likely faster as containingBlock() is a non-trivial function)

> Source/WebCore/rendering/RenderObject.cpp:2881
> +            if (!blk->hasOverflowClip())

please don't abbreviate, especially since it should be containingBlock for preciseness.

> Source/WebCore/rendering/RenderObject.cpp:2883
> +            FloatQuad boundQuad = blk->localToContainerQuad(FloatRect(blk->visualOverflowRect()), repaintContainer);

visualOverflowRect is more or less the border-box for elements with a clip: it would include only a couple of operations that bleed outside the box (like box-shadow) but would miss some like outline.

I am not sure why you don't just use the border-box.

> Source/WebCore/rendering/RenderObject.cpp:2884
> +            if (!boundQuad.isRectilinear()) // don't know how to clip this. :(

You can always do the intersection with the quad's bounding box but it will be inaccurate - as in the resulting FloatRect will cover a bigger surface - if it's not rectilinear.

If you need accuracy, you would need to manipulate Path and not rects.

> Source/WebKit/chromium/src/WebHighlight.cpp:63
> +        GraphicsLayer* graphicsLayer = 0;
> +        if (repaintContainer && repaintContainer->layer() && repaintContainer->layer()->backing())
> +            graphicsLayer = repaintContainer->layer()->backing()->graphicsLayer();

It makes me cry that you need to know so much about how rendering works just to collect some simple hit quads.

> Source/WebKit/chromium/src/WebHighlight.h:48
> +class WebHighlight {

This refactoring is somehow tangential to the change (I guess it's to be able to test more easily). Please move it to a blocking bug as it would shrink your already massive patch.

> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:176
> +#if 0

Why are you disabling this test? I see no mention of it anywhere.

> Source/WebKit/chromium/tests/data/webhighlight_test.html:4
> +<a href="" id="link1" style="position:absolute;left:50px;top:50px;background-color:#ccffcc">
> +    <div style="display:inline-block;width:500px;height:20px;background-color:#ccffcc"></div>

Please factor the style into some common classes and some id selectors. It's difficult to read the test as-is.

[Antonio Gomes]

Comment on attachment 144645 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        This patch adds RenderObject::hitQuadsForContainerWithClipping call,
>> +        to generate the clickable region for link highlighting.
> 
> If it's only for link, it should be more explicit in the naming.

is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)

[Tien-Ren Chen]

Comment on attachment 144645 [details]
Patch

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

>>> Source/WebCore/ChangeLog:9
>>> +        to generate the clickable region for link highlighting.
>> 
>> If it's only for link, it should be more explicit in the naming.
> 
> is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)

GestureTapHighlighter has some major flaws that I don't think are easily fixable.
1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.

>> Source/WebCore/rendering/RenderObject.cpp:2869
>> +    size_t oldSize = quads.size();
> 
> This is useless as quads is always empty when the function is called.

True for our current code, but there may be a chance we will batch multiple calls to the same vector. The overhead is negligible anyway.

>> Source/WebCore/rendering/RenderObject.cpp:2880
>> +        for (RenderBlock* blk = containingBlock(); blk && blk->isDescendantOf(repaintContainer); blk = blk->containingBlock()) {
> 
> I hope you understand this is a quadratic operation thanks to the isDescendantOf call and because it's in another loop it's horribly slow. 2 ways to make it less sucky:
> * look-ahead and you cache the top-most containing block to avoid the isDescendantOf() call.
> * cache the whole containing block chain (likely faster as containingBlock() is a non-trivial function)

Hmm I'm just not sure if blk->containingBlock would ever jump over a repainContainer. If not I guess we can simply use blk && blk != repaintContainer.

>> Source/WebCore/rendering/RenderObject.cpp:2883
>> +            FloatQuad boundQuad = blk->localToContainerQuad(FloatRect(blk->visualOverflowRect()), repaintContainer);
> 
> visualOverflowRect is more or less the border-box for elements with a clip: it would include only a couple of operations that bleed outside the box (like box-shadow) but would miss some like outline.
> 
> I am not sure why you don't just use the border-box.

Sounds good

>> Source/WebCore/rendering/RenderObject.cpp:2884
>> +            if (!boundQuad.isRectilinear()) // don't know how to clip this. :(
> 
> You can always do the intersection with the quad's bounding box but it will be inaccurate - as in the resulting FloatRect will cover a bigger surface - if it's not rectilinear.
> 
> If you need accuracy, you would need to manipulate Path and not rects.

I think you're right. Clip loosely is better than not clipping at all in this case.
We can allow some false positive. Manipulating polygons is simply too expensive.

>> Source/WebKit/chromium/src/WebHighlight.cpp:63
>> +            graphicsLayer = repaintContainer->layer()->backing()->graphicsLayer();
> 
> It makes me cry that you need to know so much about how rendering works just to collect some simple hit quads.

I agree it is too much leak of internals. But in this case the caller must know something about graphicsLayer in order to support threaded-scrolling/CSS animation.

>> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:176
>> +#if 0
> 
> Why are you disabling this test? I see no mention of it anywhere.

Ah, actually the whole LinkHighlightTest needs to be reworked for the new code. I'm still figuring out how to keep these tests.

[Antonio Gomes]

> > is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)
> 
> GestureTapHighlighter has some major flaws that I don't think are easily fixable.
> 1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
> 2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.

Zalan would be interested in improving this, I think.

[Tien-Ren Chen]

(In reply to comment #41)
> (From update of attachment 144645 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144645&action=review
> > Source/WebCore/ChangeLog:9
> > +        to generate the clickable region for link highlighting.
> 
> If it's only for link, it should be more explicit in the naming.

Forgot to say, this will be re-used at a later time for threaded-scrolling/touch event handling. The idea is, we want to generate the hit quads for the elements with Javascript touch handler, store them in the compositor side, so the compositor will know whether we can do a quick reject to touch events instead of forward everything to the main thread. It will be very helpful for UI responsiveness.

[Tien-Ren Chen]

(In reply to comment #44)
> > > is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)
> > 
> > GestureTapHighlighter has some major flaws that I don't think are easily fixable.
> > 1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
> > 2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.
> 
> Zalan would be interested in improving this, I think.

Why not just use our version? ;)

I tried the GestureTapHighlighter based version with various test cases (see "some additional test cases" in the attachments), most of them result in a unusable highlight.

Besides transformation, it also generates wrong box for inline elements for some cases.

[zalan]

(In reply to comment #46)
> (In reply to comment #44)
> > > > is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)
> > > 
> > > GestureTapHighlighter has some major flaws that I don't think are easily fixable.
> > > 1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
> > > 2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.
> > 
> > Zalan would be interested in improving this, I think.
> 
> Why not just use our version? ;)
> 
> I tried the GestureTapHighlighter based version with various test cases (see "some additional test cases" in the attachments), most of them result in a unusable highlight.
> 
> Besides transformation, it also generates wrong box for inline elements for some cases.
Agree, GestureTapHighlighter has multiple known issues, which were planned to be improved gradually. Since it doesn't make much sense to maintain 2 different implementations of the same feature, I'll check if we can have qt to use this.

[Tien-Ren Chen]

(In reply to comment #47)
> (In reply to comment #46)
> > (In reply to comment #44)
> > > > > is not it what Source/WebCore/page/GestureTapHighlighter.cpp is responsible for, among others? (see also https://bugs.webkit.org/show_bug.cgi?id=84989)
> > > > 
> > > > GestureTapHighlighter has some major flaws that I don't think are easily fixable.
> > > > 1. The support for transformation is terribly wrong. It gets the absoluteRects then apply layer transformation again?
> > > > 2. It doesn't separate highlights for different graphicsLayer. Which is essential to support threaded-scrolling/CSS animation.
> > > 
> > > Zalan would be interested in improving this, I think.
> > 
> > Why not just use our version? ;)
> > 
> > I tried the GestureTapHighlighter based version with various test cases (see "some additional test cases" in the attachments), most of them result in a unusable highlight.
> > 
> > Besides transformation, it also generates wrong box for inline elements for some cases.
> Agree, GestureTapHighlighter has multiple known issues, which were planned to be improved gradually. Since it doesn't make much sense to maintain 2 different implementations of the same feature, I'll check if we can have qt to use this.

Sorry for the delay in reply. I'm currently on a 2-weeks vacation. Will try to reply within 48 hours.

I think our RenderObject::hitQuadsForContainer works very similar to RenderObject::addFocusRingRects if you pass null as the repaintContainer (so it returns in absolute coordinate), except with two difference:

1. hitQuadsForContainer returns quads instead of rects. If the rendering code is not capable of dealing with quads, I think using the enclosingBoundingBox would be a feasible solution.

2. hitQuadsForContainer returns the raw hit-rects without decorations such as rounded-corner. Our "beautifying" code in is done in the WebHighlight class.

[Peter Beverloo]

Hi Tien-Ren, did you had a chance to look at this yet?

[Allan Sandfeld Jensen]

(In reply to comment #48)
> I think our RenderObject::hitQuadsForContainer works very similar to RenderObject::addFocusRingRects if you pass null as the repaintContainer (so it returns in absolute coordinate), except with two difference:
> 
> 1. hitQuadsForContainer returns quads instead of rects. If the rendering code is not capable of dealing with quads, I think using the enclosingBoundingBox would be a feasible solution.
> 
> 2. hitQuadsForContainer returns the raw hit-rects without decorations such as rounded-corner. Our "beautifying" code in is done in the WebHighlight class.

Would it make sense to change addFocusRing, to return quads? It is intended for this purpose but I guess has just not been upgraded to handle transformations. Alternatively, leave it mostly as is (on RenderObject level), but let RenderLayer apply the transformations and still end up returning quads.

[Allan Sandfeld Jensen]

Additionally, I would suggest removing the [Chromium] tag from the bug. If this lands, it could also solve problems for other platforms, for instance bug #94345, which is an assertion hit due to encountering transforms in highlight path code that can not handle transforms.

[Kenneth Rohde Christiansen]

(In reply to comment #50)
> (In reply to comment #48)
> > I think our RenderObject::hitQuadsForContainer works very similar to RenderObject::addFocusRingRects if you pass null as the repaintContainer (so it returns in absolute coordinate), except with two difference:
> > 
> > 1. hitQuadsForContainer returns quads instead of rects. If the rendering code is not capable of dealing with quads, I think using the enclosingBoundingBox would be a feasible solution.
> > 
> > 2. hitQuadsForContainer returns the raw hit-rects without decorations such as rounded-corner. Our "beautifying" code in is done in the WebHighlight class.
> 
> Would it make sense to change addFocusRing, to return quads? It is intended for this purpose but I guess has just not been upgraded to handle transformations. Alternatively, leave it mostly as is (on RenderObject level), but let RenderLayer apply the transformations and still end up returning quads.

What about making addFocusRing use hitQuadsForContainer and then just do the beautifying code on top? Drawing highlights or focus rings should be quite similar

[Allan Sandfeld Jensen]

(In reply to comment #52)
> (In reply to comment #50)
> > (In reply to comment #48)
> > > I think our RenderObject::hitQuadsForContainer works very similar to RenderObject::addFocusRingRects if you pass null as the repaintContainer (so it returns in absolute coordinate), except with two difference:
> > > 
> > > 1. hitQuadsForContainer returns quads instead of rects. If the rendering code is not capable of dealing with quads, I think using the enclosingBoundingBox would be a feasible solution.
> > > 
> > > 2. hitQuadsForContainer returns the raw hit-rects without decorations such as rounded-corner. Our "beautifying" code in is done in the WebHighlight class.
> > 
> > Would it make sense to change addFocusRing, to return quads? It is intended for this purpose but I guess has just not been upgraded to handle transformations. Alternatively, leave it mostly as is (on RenderObject level), but let RenderLayer apply the transformations and still end up returning quads.
> 
> What about making addFocusRing use hitQuadsForContainer and then just do the beautifying code on top? Drawing highlights or focus rings should be quite similar

It turns out to be unnecessary. AddFocusRing is used by normal paint logic, which means it is already clipped and transformed automatically by RenderLayer, so it doesn't need quads.

But that does give me an idea: What we are doing with hitQuadsForContainer (and GestureTapHighlighter), is duplicating the clipping code from painting, because we are doing the painting outside of the normal painting logic. If the painting was instead performed by RenderObject like it is for focusRings, this would not be necessary. So would it be possible to call a function similar to paintFocusRing to paint the tap-highlight, and give it the right graphicscontext making it paint it on another layer?

[Kenneth Rohde Christiansen]

> It turns out to be unnecessary. AddFocusRing is used by normal paint logic, which means it is already clipped and transformed automatically by RenderLayer, so it doesn't need quads.
> 
> But that does give me an idea: What we are doing with hitQuadsForContainer (and GestureTapHighlighter), is duplicating the clipping code from painting, because we are doing the painting outside of the normal painting logic. If the painting was instead performed by RenderObject like it is for focusRings, this would not be necessary. So would it be possible to call a function similar to paintFocusRing to paint the tap-highlight, and give it the right graphicscontext making it paint it on another layer?

I think this should be possible, especially if we want the same clipping. I thinks that the highlight itself might be slightly bigger than the element being highlighted (and clipped). I guess we also want to make sure that the actual highlighting is always painted on a layer different from root.

[Adam Barth]

I think this work got done in another patch.