Bug 145324

Summary: Large repaints when flexbox layout is used
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: ASSIGNED    
Severity: Normal CC: graouts, hyatt, joepeck, jonowells, mattbaker, mike.sherov, nvasilyev, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=155386
https://bugs.webkit.org/show_bug.cgi?id=155387
Bug Depends on:    
Bug Blocks: 155627    
Attachments:
Description Flags
[IMAGE] Issue - large paint flash
none
[Animated GIF] Reduction — small paint area
none
[Patch] Position absolute
nvasilyev: review-, nvasilyev: commit-queue-
[Animated GIF] Position absolute
none
Test reduction none

Joseph Pecoraro
Reported 2015-05-22 15:45:45 PDT
Created attachment 253613 [details] [IMAGE] Issue - large paint flash * SUMMARY Large repaints when typing any character in console. See attached screenshot. Particularly with the QuickConsole this is unexpected. * STEPS TO REPRODUCE 1. Inspect about:blank 2. Show Elements Tab 3. Focus quick console 4. Type in the quick console => each keystroke a large portion of the window gets a paint flash
Attachments
[IMAGE] Issue - large paint flash (63.14 KB, image/png)
2015-05-22 15:45 PDT, Joseph Pecoraro 		no flags
Details
[Animated GIF] Reduction — small paint area (216.04 KB, image/gif)
2016-03-07 22:20 PST, Nikita Vasilyev 		no flags
Details
[Patch] Position absolute (3.80 KB, text/plain)
2016-03-07 23:04 PST, Nikita Vasilyev 		nvasilyev: review-
nvasilyev: commit-queue-
Details
[Animated GIF] Position absolute (783.54 KB, image/gif)
2016-03-07 23:06 PST, Nikita Vasilyev 		no flags
Details
Test reduction (470 bytes, text/html)
2016-03-18 12:22 PDT, alan 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2015-05-22 15:46:09 PDT
<rdar://problem/21083856>
Timothy Hatcher
Comment 2 2015-05-26 14:56:13 PDT
I wonder if this is a cause of slow typing in the console?
Nikita Vasilyev
Comment 3 2016-03-03 21:11:43 PST
Two observations: 1. Chrome DevTools also flashes the whole window on every keystroke. It doesn't use CodeMirror, it uses `-webkit-user-modify: read-write-plaintext-only` instead. It does use flexbox the same way Web Inspector does. 2. CodeMirror forces layout on every key stroke (that causes text to change): // Read the actual heights of the rendered lines, and update their // stored heights to match. function updateHeightsInViewport(cm) { var display = cm.display; var prevBottom = display.lineDiv.offsetTop; https://github.com/WebKit/webkit/blob/master/Source/WebInspectorUI/UserInterface/External/CodeMirror/codemirror.js#L783 If we don't wrap text, we would need to updateHeightsInViewport only when a line break is inserted or removed, theoretically. While this would mitigate the issue, it doesn't explain large repaints.
Timothy Hatcher
Comment 4 2016-03-04 09:29:50 PST
I suspect the large repaints are due to a bug in WebKit's flexbox code. It has had similar issues in the past. Lets just give this to someone in WebKit to look at.
Nikita Vasilyev
Comment 5 2016-03-07 22:20:05 PST
Created attachment 273272 [details] [Animated GIF] Reduction — small paint area I created a reduction, but it did NOT reproduce the bug: http://nv.github.io/webkit-inspector-bugs/flexbox-large-repaints/ As you can see on the attached GIF, the paint area is small.
Nikita Vasilyev
Comment 6 2016-03-07 23:04:20 PST
Created attachment 273274 [details] [Patch] Position absolute As a proof of concept, I replaced flexbox with `position: absolute` and some JavaScript. It indeed solved the large repaints issue. However, I don't think we should do this. It adds a lot of unnecessary complexity. We should really find what's wrong with flexbox here.
Nikita Vasilyev
Comment 7 2016-03-07 23:06:36 PST
Created attachment 273275 [details] [Animated GIF] Position absolute
Simon Fraser (smfr)
Comment 8 2016-03-08 08:48:16 PST
A reduced testcase with flex box would really help.
Nikita Vasilyev
Comment 9 2016-03-10 21:22:53 PST
(In reply to comment #8) > A reduced testcase with flex box would really help. I just made one: http://nv.github.io/webkit-inspector-bugs/flexbox-large-repaints/ Typing in the CodeMirror editor causes the whole <body> to repaint.
alan
Comment 10 2016-03-10 21:39:09 PST
(In reply to comment #9) > (In reply to comment #8) > > A reduced testcase with flex box would really help. > > I just made one: > http://nv.github.io/webkit-inspector-bugs/flexbox-large-repaints/ > Typing in the CodeMirror editor causes the whole <body> to repaint. Great! Thanks, I'll look into it.
alan
Comment 11 2016-03-14 09:33:59 PDT
Could you reduce it further so that it does not require a ~9K JS code (codemirror.js)?
Nikita Vasilyev
Comment 12 2016-03-17 00:35:20 PDT
(In reply to comment #11) > Could you reduce it further so that it does not require a ~9K JS code > (codemirror.js)? Sure. I replaced CodeMirror with <div style="-webkit-user-modify:read-write-plaintext-only">. http://nv.github.io/webkit-inspector-bugs/flexbox-large-repaints/
alan
Comment 13 2016-03-18 12:22:07 PDT
Created attachment 274445 [details] Test reduction Unfortunately while flexing the inner flexbox, we also issue repaints for the intermediate positions/sizes. We should reconsider a different repaint invalidation logic for flexboxes (maybe delaying repaint calls until after flexing is done) I am not sure if there's an easy fix for this, though Hyatt might disagree.
Note You need to log in before you can comment on or make changes to this bug.