Bug 153299 - Outline should contribute to visual overflow.
Summary: Outline should contribute to visual overflow.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
: 104460 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-01-20 19:24 PST by zalan
Modified: 2016-02-06 19:24 PST (History)
4 users (show)

See Also:


Attachments
Patch (65.14 KB, patch)
2016-01-20 21:14 PST, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.07 MB, application/zip)
2016-01-20 21:52 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-yosemite (814.46 KB, application/zip)
2016-01-20 22:06 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.04 MB, application/zip)
2016-01-20 22:16 PST, Build Bot
no flags Details
Patch (68.56 KB, patch)
2016-01-21 14:19 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (63.04 KB, patch)
2016-01-21 14:30 PST, zalan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (786.99 KB, application/zip)
2016-01-21 15:32 PST, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-yosemite (744.71 KB, application/zip)
2016-01-21 15:56 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (877.33 KB, application/zip)
2016-01-21 18:21 PST, Build Bot
no flags Details
outline auto repaint issues (1.44 KB, text/html)
2016-01-28 13:11 PST, zalan
no flags Details
outline auto repaint issues (1.54 KB, text/html)
2016-02-01 13:46 PST, zalan
no flags Details
Patch (81.31 KB, patch)
2016-02-02 15:12 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (86.54 KB, patch)
2016-02-03 10:02 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (87.16 KB, patch)
2016-02-03 21:07 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (86.91 KB, patch)
2016-02-03 21:25 PST, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-yosemite (793.20 KB, application/zip)
2016-02-03 22:22 PST, Build Bot
no flags Details
Patch (86.91 KB, patch)
2016-02-04 08:39 PST, zalan
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2016-01-20 19:24:07 PST
RenderView::setMaximalOutlineSize is evil.
Comment 1 zalan 2016-01-20 20:52:43 PST
rdar://problem/23486927
Comment 2 Simon Fraser (smfr) 2016-01-20 21:00:46 PST
Exciting!
Comment 3 zalan 2016-01-20 21:14:18 PST
Created attachment 269428 [details]
Patch
Comment 4 zalan 2016-01-20 21:14:37 PST
Comment on attachment 269428 [details]
Patch

WIP patch.
Comment 5 Build Bot 2016-01-20 21:52:14 PST
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
Comment 6 Build Bot 2016-01-20 21:52:17 PST
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
Comment 7 Build Bot 2016-01-20 22:06:29 PST
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
Comment 8 Build Bot 2016-01-20 22:06:32 PST
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
Comment 9 Build Bot 2016-01-20 22:16:39 PST
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
Comment 10 Build Bot 2016-01-20 22:16:43 PST
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
Comment 11 zalan 2016-01-21 14:19:11 PST
Created attachment 269492 [details]
Patch
Comment 12 zalan 2016-01-21 14:23:37 PST
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)
Comment 13 zalan 2016-01-21 14:30:29 PST
Created attachment 269493 [details]
Patch
Comment 14 Build Bot 2016-01-21 15:32:21 PST
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
Comment 15 Build Bot 2016-01-21 15:32:25 PST
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
Comment 16 Build Bot 2016-01-21 15:56:35 PST
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
Comment 17 Build Bot 2016-01-21 15:56:39 PST
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
Comment 18 Build Bot 2016-01-21 18:21:18 PST
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
Comment 19 Build Bot 2016-01-21 18:21:22 PST
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
Comment 20 zalan 2016-01-28 13:11:33 PST
Created attachment 270142 [details]
outline auto repaint issues
Comment 21 zalan 2016-02-01 13:46:03 PST
Created attachment 270420 [details]
outline auto repaint issues
Comment 22 zalan 2016-02-01 14:35:19 PST
*** Bug 104460 has been marked as a duplicate of this bug. ***
Comment 23 zalan 2016-02-02 15:12:13 PST
Created attachment 270526 [details]
Patch
Comment 24 zalan 2016-02-03 10:02:53 PST
Created attachment 270582 [details]
Patch
Comment 25 Dave Hyatt 2016-02-03 11:12:09 PST
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.
Comment 26 zalan 2016-02-03 21:07:21 PST
Created attachment 270626 [details]
Patch
Comment 27 zalan 2016-02-03 21:25:57 PST
Created attachment 270628 [details]
Patch
Comment 28 Build Bot 2016-02-03 22:22:29 PST
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
Comment 29 Build Bot 2016-02-03 22:22:32 PST
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
Comment 30 zalan 2016-02-04 08:37:20 PST
(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.
Comment 31 zalan 2016-02-04 08:39:37 PST
Created attachment 270658 [details]
Patch
Comment 32 Dave Hyatt 2016-02-04 09:12:32 PST
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?
Comment 33 zalan 2016-02-06 15:07:58 PST
Committed r196222: <http://trac.webkit.org/changeset/196222>
Comment 34 Michael Catanzaro 2016-02-06 18:18:30 PST
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?
Comment 35 zalan 2016-02-06 19:24:34 PST
(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.