Bug 153299 – Outline should contribute to visual overflow.

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

[zalan]

RenderView::setMaximalOutlineSize is evil.

[zalan]

rdar://problem/23486927

[Simon Fraser (smfr)]

Exciting!

[zalan]

Created attachment 269428 [details]
Patch

[zalan]

Comment on attachment 269428 [details]
Patch

WIP patch.

[Build Bot]

Comment on attachment 269428 [details]
Patch

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

New failing tests:
fast/multicol/multicol-with-child-renderLayer-for-input.html
accessibility/mac/selection-element-tabbing-to-link.html
fast/repaint/4776765.html
accessibility/mac/selection-notification-focus-change.html

[Build Bot]

Created attachment 269430 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269428 [details]
Patch

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

New failing tests:
fast/multicol/multicol-with-child-renderLayer-for-input.html
accessibility/mac/selection-element-tabbing-to-link.html
fast/repaint/4776765.html
accessibility/mac/selection-notification-focus-change.html

[Build Bot]

Created attachment 269432 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269428 [details]
Patch

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

New failing tests:
imported/blink/fast/forms/select/styling-option-checked.html
fast/multicol/multicol-with-child-renderLayer-for-input.html
accessibility/mac/selection-element-tabbing-to-link.html
imported/blink/fast/html/crash-on-invalid-selection-index.html
fast/forms/select/selectall-command-crash.html
fast/repaint/4776765.html
accessibility/mac/selection-notification-focus-change.html

[Build Bot]

Created attachment 269433 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[zalan]

Created attachment 269492 [details]
Patch

[zalan]

Remaining issues: 
1. multi-col clips outline now that it's part of visual overflow. Non-multi-column nested layer painting works (no clipping) see: fast/multicol/multicol-with-child-renderLayer-for-input.html (and somewhat related: imported/blink/fast/multicol/outlines-at-column-boundaries.html)

2. Have someone fixed that PostResolutionCallbackDisabler issue (not a blocker)

[zalan]

Created attachment 269493 [details]
Patch

[Build Bot]

Comment on attachment 269493 [details]
Patch

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

New failing tests:
fast/repaint/4776765.html

[Build Bot]

Created attachment 269506 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269493 [details]
Patch

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

New failing tests:
fast/repaint/4776765.html

[Build Bot]

Created attachment 269509 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269493 [details]
Patch

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

New failing tests:
fast/repaint/4776765.html

[Build Bot]

Created attachment 269530 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[zalan]

Created attachment 270142 [details]
outline auto repaint issues

[zalan]

Created attachment 270420 [details]
outline auto repaint issues

[zalan]

*** Bug 104460 has been marked as a duplicate of this bug. ***

[zalan]

Created attachment 270526 [details]
Patch

[zalan]

Created attachment 270582 [details]
Patch

[Dave Hyatt]

Comment on attachment 270582 [details]
Patch

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

Comments in bug.

> Source/WebCore/rendering/InlineFlowBox.cpp:955
> +    if (!lineStyle.hasOutline())
> +        return;

You should check lineStyle.outlineSize() too, since outlineWidth() can be positive, but outlineOffset pushes you back in. If outlineSize() == 0, then you don't have overflow, even if outlineWidth() > 0.

> Source/WebCore/rendering/RenderBox.cpp:4496
> +    if (!style().boxShadow() && !style().hasBorderImageOutsets() && !outlineStyleForRepaint().hasOutline())

Same problem here. Should check outlineSize().

> Source/WebCore/rendering/RenderBox.cpp:4543
> +    if (outlineStyleForRepaint().hasOutline()) {

Same.

> Source/WebCore/rendering/style/RenderStyle.cpp:2034
> +    if (outlineStyleIsAuto())
> +        return RenderTheme::platformFocusRingWidth();

This does not seem right to me. If outline-style is auto, you are ignoring the actual parsed value of outline width? Won't this cause us not to honor outline width at all?

> Source/WebCore/rendering/style/RenderStyle.h:671
> +    float outlineWidth() const;
> +    bool hasOutline() const { return outlineStyle() > BHIDDEN && outlineWidth() > 0; }

You might want to make a helper like "bool hasOutlineInVisualOverflow() const { return hasOutline() && outlineSize() > 0; }" and use it in the places I mentioned earlier.

Consider making some tests with negative outline offsets that pull the ring back in.

[zalan]

Created attachment 270626 [details]
Patch

[zalan]

Created attachment 270628 [details]
Patch

[Build Bot]

Comment on attachment 270628 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection-2.html

[Build Bot]

Created attachment 270635 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[zalan]

(In reply to comment #28)
> Comment on attachment 270628 [details]
> Patch
> 
> Attachment 270628 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/780117
> 
> New failing tests:
> imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-
> collection-2.html

This failure looks completely unrelated.

[zalan]

Created attachment 270658 [details]
Patch

[Dave Hyatt]

Comment on attachment 270658 [details]
Patch

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

r=me

> Source/WebCore/rendering/style/RenderStyle.cpp:483
> +    if (hasOutline() != other.hasOutline()
> +        || (hasOutline()
> +        && (outlineSize() != other.outlineSize())))
> +        return true;

Assuming I understand correctly, and this method should only return true if your visual overflow could change, then couldn't this check be stricter and use hasOutlineInVisualOverflow?

[zalan]

Committed r196222: <http://trac.webkit.org/changeset/196222>

[Michael Catanzaro]

Zalan could you please sanity-check my follow-up in r196235: <http://trac.webkit.org/changeset/196235>

Also, this caused bug #153956, any ideas about that?

[zalan]

(In reply to comment #34)
> Zalan could you please sanity-check my follow-up in r196235:
> <http://trac.webkit.org/changeset/196235>
> 
> Also, this caused bug #153956, any ideas about that?
I was going to ask cfleizach about it since there are some failures on Mac too. Changeset looks good. Thanks for taking care of that.

[Ryosuke Niwa]

*** Bug 106397 has been marked as a duplicate of this bug. ***