Bug 110895 – Focus ring for a child layer is incorrectly offset by ancestor composited layer's position

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Xianzhu Wang]

Created attachment 190333 [details]
Test case

For the attached test case, the focus ring for the dark grey element is incorrectly offset by the absolute position of the element.

I tried to minimize the test case, but all divs in the test seem mandatory to reproduce the bug.

[Xianzhu Wang]

Created attachment 190359 [details]
Simplified test case

Just found the test can be simplified.

[Xianzhu Wang]

BTW Question: do we really need to draw the focus-ring (outline style=='auto") for an overflowed element?

For example, the outline for the following

<div style="outline: red auto thin; width: 50px; height: 50px">
  <div style="width: 100px; height: 25px"></div>
</div>

will be like
 ______________
|              |
|        ______|
|       |
|_______|

Is this intended?

Sometimes if the overflowed child is fully out of the parent box, then the outline may be like:
 ______________
|              |
|______________|
 _______
|       |
|_______|

Even worse thing is that the lower box will only appear when the area is invalidated. If part of the area is invalidated, only part of the box appears.

In addition, if the outline style is not 'auto' (e.g. 'solid' or 'dotted'), then only the rectangle of the element will be drawn, not including overflowed child. Is this also intended?

[Xianzhu Wang]

Considering transformations, drawing focus ring around overflowed children will be more complex, and the visual effect might not be desired.

[Sami Kyöstilä]

Seems like the root cause here is that outline:auto doesn't support transforms, right? I guess we'd need a way of taking the union of arbitrarily transformed quads -- is there code for that already?

As to clipping the outline to the overflow container, I think it would make sense to do that but maybe we should fix it in a different bug.

[Julien Chaffraix]

(In reply to comment #4)
> Seems like the root cause here is that outline:auto doesn't support transforms, right? 

AFAICT this is completely unrelated: the test case has 0 transforms and positioned offset are representable in our current system.

> I guess we'd need a way of taking the union of arbitrarily transformed quads -- is there code for that already?

Focus rings and outlines are represented as rectangles (IntRect / LayoutRect), not quads. You would need to update the code to support quads. See addFocusRingRect and paintOutline for details.

> As to clipping the outline to the overflow container, I think it would make sense to do that but maybe we should fix it in a different bug.

Outlines are specified in CSS 2.1 [1] and also CSS 3 UI [2]. We definitely want to follow what the specifications are saying, not what we believe 'outline' should be. If other browsers don't match our behavior, we should get the specification updated to match the consensus.

Relevant bits:

* "Outlines may be non-rectangular." (note that this is a MAY which makes our implementation using rects compliant)
* "By default, the outline is drawn starting just outside the border edge. However, it is possible to offset the outline and draw it beyond the border edge."
* "For example, if the element is broken across several lines, the outline should be an outline or minimum set of outlines that encloses all the element's boxes."

[1] http://www.w3.org/TR/CSS21/ui.html#dynamic-outlines
[2] http://dev.w3.org/csswg/css3-ui/#outline-properties

[Xianzhu Wang]

(In reply to comment #5)
> Outlines are specified in CSS 2.1 [1] and also CSS 3 UI [2]. We definitely want to follow what the specifications are saying, not what we believe 'outline' should be. If other browsers don't match our behavior, we should get the specification updated to match the consensus.
> 
> Relevant bits:
> 
> * "Outlines may be non-rectangular." (note that this is a MAY which makes our implementation using rects compliant)
> * "By default, the outline is drawn starting just outside the border edge. However, it is possible to offset the outline and draw it beyond the border edge."
> * "For example, if the element is broken across several lines, the outline should be an outline or minimum set of outlines that encloses all the element's boxes."
> 
> [1] http://www.w3.org/TR/CSS21/ui.html#dynamic-outlines
> [2] http://dev.w3.org/csswg/css3-ui/#outline-properties

It only mentions the multiple-line case. The second half of the sentence seems to cover the overflow case but looks unclear. In our current code, the cases are in different code paths.

Another issue is, our behaviors for 'auto' and non-'auto' outlines are different. The former draws outlines of overflowed sub-elements, but the latter draws only a single rectangle enclosing the element itself only.

Tried other browsers:
- Firefox (19.0). a single rectangle enclosing the the element and its sub-elements when outline-style!=auto. Doesn't support 'auto'.
- IE9: a single rectangle enclosing the element only when outline-style!=auto. Doesn't support 'auto'.

IE9 is same as WebKit when outline-style!=auto.

For real focus ring (when an element is really focused):
- Firefox: same as outline, using dotted black and white outline style.
- 

Putting aside transformations, the issue is caused by the following lines in RenderBlock::addFocusRingRects():

                if (box->layer())
                    pos = curr->localToAbsolute();
                else
                    pos = FloatPoint(additionalOffset.x() + box->x(), additionalOffset.y() + box->y());
                box->addFocusRingRects(rects, flooredLayoutPoint(pos));

RenderObject::paintFocusRing() expects that the rects are in coordinates of current paint (in the case, coordinates of the outer GraphicsLayer). However, the above code will use the absolute coordinates of the inner layer, so the focus ring for the inner layer will be offset by the origin of the outer layer. This happens only when the inner layer is a sub-element of the element wanting the focus ring.

I tried to remove the 'if' part leaving only the 'else' part, and the focus ring will be at correct position (for the test case). Do you think this is a correct fix?

[Xianzhu Wang]

CSS3 seems to give browsers more flexibility about drawing outlines:

... User agents should use an algorithm for determining the outline that encloses a region appropriate for conveying the concept of focus to the user.

I don't think drawing a separate outline for the fully overflowed sub-element conveying more information about focus to the user. Instead, it seems to confuse the user.

I believe in many cases, the web page designer doesn't actually want an out-of-flow element to be a visual part of the ancestor elements. Drawing parent's  including out-of-flow sub-elements may not be the intention of the web page designer either.

===========
P.S. to #6:

For real focus ring (when an element is really focused):
- Firefox: position same as outline (enclosing sub-elements), using dotted black-and-white outline style.
- IE9: position same as outline (not enclosing sub-elements), using dotted black-and-white outline style. The sub-layer covers part of the focus ring.

[Simon Fraser (smfr)]

Regardless of what the spec says, there's an obvious bug with composited layers here.

[Xianzhu Wang]

(In reply to comment #8)
> Regardless of what the spec says, there's an obvious bug with composited layers here.

Please run the test case. The focus ring for the sub-layer is not at the correct position.

#2 is not the bug itself, just my questions about the current (correct) behavior.

[Julien Chaffraix]

(In reply to comment #9)
> (In reply to comment #8)
> > Regardless of what the spec says, there's an obvious bug with composited layers here.
> 
> Please run the test case. The focus ring for the sub-layer is not at the correct position.
> 
> #2 is not the bug itself, just my questions about the current (correct) behavior.

I agree with Simon. It's an obvious functional bug if the focus is not drawn at the right position regardless of whether you think it's good UX. The localToAbsolute calls seems wrong as it walks the tree for each child but if you remove it, won't you ignore any offset the (positioned) child may have?

Also please stop piggy-backing random bugs / questions into this bug. Try to keep bugzilla razor-focused on a single bug or it will confuse people (like me) and they will stop reading the comments in full (I already started). Other means of communications (IRC, mailing lists, other bugs) are here to ask such questions if you need to.

[Xianzhu Wang]

(In reply to comment #10)
> I agree with Simon. It's an obvious functional bug if the focus is not drawn at the right position regardless of whether you think it's good UX. The localToAbsolute calls seems wrong as it walks the tree for each child but if you remove it, won't you ignore any offset the (positioned) child may have?
>

Removing the 'if' part seems not to ignore some offset, but will include unnecessary offsets of parent elements (through the additionalOffset parameter to RenderBlock::addFocusRingRects()). Anyway that is incorrect.

What's the good way to know the ancestor layer that the current painting is originated, or other way to know the correct offset of the sub-layer to paint outline?
 
> Also please stop piggy-backing random bugs / questions into this bug. Try to keep bugzilla razor-focused on a single bug or it will confuse people (like me) and they will stop reading the comments in full (I already started). Other means of communications (IRC, mailing lists, other bugs) are here to ask such questions if you need to.

Sorry for the confusing. I asked the question here partly because I thought not drawing outlines around sub-layers also a possible solution to the issue if this could be agreed upon. I should have made it clear :)

[Xianzhu Wang]

Actually the child layer needs not to be composited, but some ancestor layer need to be, to reproduce the bug.

[Xianzhu Wang]

Created attachment 190636 [details]
Patch

[Early Warning System Bot]

Comment on attachment 190636 [details]
Patch

Attachment 190636 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16672629

[Early Warning System Bot]

Comment on attachment 190636 [details]
Patch

Attachment 190636 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16758376

[Simon Fraser (smfr)]

Comment on attachment 190636 [details]
Patch

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

The fix is good, just some naming quibbles.

> Source/WebCore/ChangeLog:31
> +        (RenderBox):

You should remove useless lines like this.

> Source/WebCore/rendering/PaintInfo.h:58
> +        OverlapTestRequestMap* overlapTestRequests = 0, const RenderLayerModelObject* newRepaintContainer = 0)

The name is a bit confusing, since this isn't about "repaint". Let's call it "paintContainer" here. Or we could rename "paintingRoot", and re-use that name for this.

> Source/WebCore/rendering/RenderBlock.h:538
> +    virtual void addFocusRingRects(Vector<IntRect>&, const LayoutPoint&, const RenderLayerModelObject*);

This should name the parameter so it's easy to tell what it's for. Perhaps "rootForRects" or "paintingRoot".

> Source/WebCore/rendering/RenderBox.h:165
> +    virtual void addFocusRingRects(Vector<IntRect>&, const LayoutPoint&, const RenderLayerModelObject*);

Ditto.

> Source/WebCore/rendering/RenderInline.cpp:1368
> +void RenderInline::addFocusRingRects(Vector<IntRect>& rects, const LayoutPoint& additionalOffset, const RenderLayerModelObject* repaintContainer)

repaintContainer->paintingRoot.

> Source/WebCore/rendering/RenderInline.h:84
> +    virtual void addFocusRingRects(Vector<IntRect>&, const LayoutPoint&, const RenderLayerModelObject*);

Same here. If you're touching these functions, you might as well add OVERRIDE.

[Xianzhu Wang]

Created attachment 190739 [details]
Patch

[Xianzhu Wang]

Comment on attachment 190636 [details]
Patch

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

>> Source/WebCore/rendering/PaintInfo.h:58
>> +        OverlapTestRequestMap* overlapTestRequests = 0, const RenderLayerModelObject* newRepaintContainer = 0)
> 
> The name is a bit confusing, since this isn't about "repaint". Let's call it "paintContainer" here. Or we could rename "paintingRoot", and re-use that name for this.

I choose 'paintContainer', as I found that paintingRoot has already been used in PaintInfo for another purpose (to paint only the subtree).

>> Source/WebCore/rendering/RenderBlock.h:538
>> +    virtual void addFocusRingRects(Vector<IntRect>&, const LayoutPoint&, const RenderLayerModelObject*);
> 
> This should name the parameter so it's easy to tell what it's for. Perhaps "rootForRects" or "paintingRoot".

Done.

>> Source/WebCore/rendering/RenderBox.h:165
>> +    virtual void addFocusRingRects(Vector<IntRect>&, const LayoutPoint&, const RenderLayerModelObject*);
> 
> Ditto.

Done.

>> Source/WebCore/rendering/RenderInline.cpp:1368
>> +void RenderInline::addFocusRingRects(Vector<IntRect>& rects, const LayoutPoint& additionalOffset, const RenderLayerModelObject* repaintContainer)
> 
> repaintContainer->paintingRoot.

Changed to paintContainer.

>> Source/WebCore/rendering/RenderInline.h:84
>> +    virtual void addFocusRingRects(Vector<IntRect>&, const LayoutPoint&, const RenderLayerModelObject*);
> 
> Same here. If you're touching these functions, you might as well add OVERRIDE.

Done.

[Xianzhu Wang]

Created attachment 190764 [details]
Rebased. For landing

[WebKit Review Bot]

Comment on attachment 190764 [details]
Rebased. For landing

Clearing flags on attachment: 190764

Committed r144350: <http://trac.webkit.org/changeset/144350>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.