Bug 145324 - Large repaints when flexbox layout is used
Summary: Large repaints when flexbox layout is used
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 155627
  Show dependency treegraph
 
Reported: 2015-05-22 15:45 PDT by Joseph Pecoraro
Modified: 2017-02-15 18:47 PST (History)
10 users (show)

See Also:


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, zalan
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 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
Comment 1 Radar WebKit Bug Importer 2015-05-22 15:46:09 PDT
<rdar://problem/21083856>
Comment 2 Timothy Hatcher 2015-05-26 14:56:13 PDT
I wonder if this is a cause of slow typing in the console?
Comment 3 Nikita Vasilyev 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.
Comment 4 Timothy Hatcher 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.
Comment 5 Nikita Vasilyev 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.
Comment 6 Nikita Vasilyev 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.
Comment 7 Nikita Vasilyev 2016-03-07 23:06:36 PST
Created attachment 273275 [details]
[Animated GIF] Position absolute
Comment 8 Simon Fraser (smfr) 2016-03-08 08:48:16 PST
A reduced testcase with flex box would really help.
Comment 9 Nikita Vasilyev 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.
Comment 10 zalan 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.
Comment 11 zalan 2016-03-14 09:33:59 PDT
Could you reduce it further so that it does not require a ~9K JS code (codemirror.js)?
Comment 12 Nikita Vasilyev 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/
Comment 13 zalan 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.