Bug 109319 – [chromium] Link Highlight should work with box-shadow, static/absolute/fixed positioning in composited layers.

• Component: New Bugs
• Status: RESOLVED WONTFIX

[W. James MacLean]

[chromium] Link Highlight should work with box-shadow, static/absolute/fixed positioning in composited layers.

[W. James MacLean]

Created attachment 187355 [details]
Patch

[Adam Barth]

Comment on attachment 187355 [details]
Patch

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

> Source/WebKit/chromium/src/LinkHighlight.h:91
> +    bool m_usingNonCompositedContentHost;

Should we initialize this member in the constructor?

[Adam Barth]

This patch is a bit out of my depth to review.

[W. James MacLean]

Comment on attachment 187355 [details]
Patch

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

>> Source/WebKit/chromium/src/LinkHighlight.h:91
>> +    bool m_usingNonCompositedContentHost;
> 
> Should we initialize this member in the constructor?

Probably not a bad idea, though calls to computeHighlightLayerPathAndPosition(), where it is used, should never happen without a prior call to computeEnclosingCompositingLayer(), where it is set.

[Julien Chaffraix]

Comment on attachment 187355 [details]
Patch

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

> Source/WebKit/chromium/src/LinkHighlight.cpp:215
> +        // If absolute or fixed, need to subtract out our fixed positioning.
> +        if (position == AbsolutePosition || position == FixedPosition) {
> +            const RenderStyle* style = m_node->renderer()->style();
> +            FloatPoint delta(style->logicalLeft().getFloatValue(), style->logicalTop().getFloatValue());
> +            positionAdjust.moveBy(delta);
> +        }

Is it really totally right? You want to get the position relative to your composited ancestor, not your containing block (which is what you are doing above). RenderLayer (which positioned elements have) uses convertToLayerCoords to do this conversion.

> LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-simple-div-boxshadow-fixed-nested2.html:9
> +<div id="targetDiv" onclick="doNothing();" style="position: fixed; left: 50px; top: 50px; width: 50px; height: 50px; box-shadow: 0px 4px 23px 5px rgba(0, 0, 0, 0.2); border-style: solid; border-width: 2px; -webkit-tap-highlight-color: rgba(0, 255, 0, 0.5); cursor: Pointer">

I don't see the need for onclick="doNothing()", can't you just set the focus on the element programmatically if that's what you are trying to do?

> LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-simple-div-boxshadow-fixed-nested2.html:15
> +This test is successful if the box labelled "Target" has a transparent green rectangle centred on it.

typo: centred (in all tests but I won't repeat it).

[W. James MacLean]

Comment on attachment 187355 [details]
Patch

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

>> Source/WebKit/chromium/src/LinkHighlight.cpp:215
>> +        }
> 
> Is it really totally right? You want to get the position relative to your composited ancestor, not your containing block (which is what you are doing above). RenderLayer (which positioned elements have) uses convertToLayerCoords to do this conversion.

I'll see if converToLayerCoords() will manage it. If so, that would certainly be preferable.

>> LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-simple-div-boxshadow-fixed-nested2.html:9
>> +<div id="targetDiv" onclick="doNothing();" style="position: fixed; left: 50px; top: 50px; width: 50px; height: 50px; box-shadow: 0px 4px 23px 5px rgba(0, 0, 0, 0.2); border-style: solid; border-width: 2px; -webkit-tap-highlight-color: rgba(0, 255, 0, 0.5); cursor: Pointer">
> 
> I don't see the need for onclick="doNothing()", can't you just set the focus on the element programmatically if that's what you are trying to do?

Probably don't need this ... it's likely there for interactive use of the layout test. I'll remove it.

>> LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-simple-div-boxshadow-fixed-nested2.html:15
>> +This test is successful if the box labelled "Target" has a transparent green rectangle centred on it.
> 
> typo: centred (in all tests but I won't repeat it).

Not so much a typo as my using the Canadian/UK spelling. If WebKit requires the US spelling I can convert (though I didn't see it in the WebKit style guide :-) ).

[W. James MacLean]

(In reply to comment #6)
> (From update of attachment 187355 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187355&action=review
> 
> >> Source/WebKit/chromium/src/LinkHighlight.cpp:215
> >> +        }
> > 
> > Is it really totally right? You want to get the position relative to your composited ancestor, not your containing block (which is what you are doing above). RenderLayer (which positioned elements have) uses convertToLayerCoords to do this conversion.
> 
> I'll see if converToLayerCoords() will manage it. If so, that would certainly be preferable.

So I tried using convertToLayerCoords, and my attempts thus far still show the same problem this cl hopes to fix, namely the highlight is offset from its correct location in the presence of box shadows.

What I did was:

1) Only use convertToLayerCoords() when the compositing layer is an ancestor (or possibly the same as) the target layer. When the compositing layer I'm going to put the highlight into is NCCH, then we don't do the extra adjustments.

2) I used the target's renderer's enclosing layer as the caller to converToLayerCoords.

3) I convert the absolute quad coords back to local before using convertToLayerCoords().

After this, it seems we get the same (wrong) result as without using convertToLayerCoords(). 

In one of my use cases this makes sense, since the target is already in a composited layer, so the calling and ancestor layer are the same and converToLayerCoords() early exits. Perhaps calling node->absoluteQuads() is not the right thing to do for a box with a shadow?