Implement new logic for maximum number of fragments |
|||||
Issue descriptionCurrently, it's possible to have a huge number of fragments, in cases like this: <div style="columns: 2; height: 10px; width: 10px"> <div style="width: 10px; height: 10000000px"></div> </div> due to the very large height of the inner div. Currently, we limit the maximum number of fragments to 500 (see kMaxNumFragments in PaintPropertyTreeBuilder.cpp). Proposal: 1. Limit to max(10, 100 * (per-column height) / (height of content under fragmentation container)) For cases like the above example, this would limit to just 10 columns But for cases where there really need to be a a large number of columns, each column must consume at least a certain percentage of the total height. 2. For cases where content exceeds the limits from #1, overflow into the last column.
,
Feb 2 2018
The proposed formula [1] simply means that the number of columns will be the percentage of the total content that can be fit into one column. I.e. if a column can take 5% of the total content, you get to use 5 columns in total. If a column can take 20%, you get to use 20 columns. If a column can take 50%, you get to use 50 columns. But if one column can take 50% of the content, you'll typically only need 2 columns. :) So this is a bit backwards. The more content you have, the fewer columns you're allowed to create. [1] 100 * (per-column height) / (height of content under fragmentation container) Maybe just k * "per-column height"? This should work, because LayoutUnit is limited to 2^25 (approx 33 million pixels). With k=1.0, you'll never be able to get more than 5793 columns (2^12.5). With lower k values than 1.0, the maximum column count will be lower.
,
Feb 2 2018
I agree, my formula didn't make much sense. Your proposal is sort of like enforcing an aspect ratio, which sounds nice.
,
Feb 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/469db7dcdae3cea3a26ca5349cb89995bc96fc7c commit 469db7dcdae3cea3a26ca5349cb89995bc96fc7c Author: Morten Stenshorne <mstensho@chromium.org> Date: Fri Feb 02 21:32:30 2018 New logic for maximum number of fragments. Instead of unconditionally clamping at 500 columns, try to identify legitimate use cases for many columns, and allow those. At the same time, limit (what we consider to be) illegitimate reasons even more brutally. Bug: 808189 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: Icf70d655c2da0b4a1af7e256cf7f3ef8c72f8715 Reviewed-on: https://chromium-review.googlesource.com/898438 Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#534158} [add] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/LayoutTests/fast/multicol/column-clamping-expected.html [add] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/LayoutTests/fast/multicol/column-clamping.html [modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/LayoutTests/fast/multicol/scrollable-basic.html [modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/layout/LayoutPagedFlowThread.cpp [modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.cpp [modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroup.h [modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/layout/MultiColumnFragmentainerGroupTest.cpp [modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp [modify] https://crrev.com/469db7dcdae3cea3a26ca5349cb89995bc96fc7c/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
,
Feb 5 2018
,
Apr 30 2018
,
Apr 30 2018
I do not understand this issue, and why it suddenly proposes to limit columns at some low number??? What are "illegitimate reasons" for such division? My reason is that WebView control is used to implement ebook reader-like interface with pages read by swiping sideways. Why at some page (currently around page 150) suddenly all text must be appended to the last column??? This is insane. There was never such limitation before and all worked beautifully, now you are breaking existing apps with your twisted "illegitimate reason" logic... All other browsers I tested (Firefox on Android and Firefox and Microsoft Edge on Windows) do not limit the column number, only Chrome is problematic. I would not care if it was only Chrome browser, but you also broke Android WebView control and many apps that use it. Please do whatever you want with Chrome browser, but stop breaking Android WebView control!
,
Apr 30 2018
Could you please describe an example where more than 500 columns make sense, to help us understand your use case?
,
Apr 30 2018
It is not 500, but around 150. Please download my Android app, @Voice Aloud Reader (https://play.google.com/store/apps/details?id=com.hyperionics.avar), open some long text in it (or e.g. some long PDF). Press the T+/- icon on top, MODE tab, turn on "Divide text into pages, scroll horizontally" option. Now you can swipe pages sideways to read. Suddenly at some point (about screen 150, depending on font size) you'll stop seeing any text on pages, because all the remaining text was appended to the last visible column. The app has been in Google Play for about 4 years, this change broke it. Again, do whatever you want with the browser, but please don't break the WebView control, or make Google provide an alternative stable WebView which is not bound to the wild changes in Chrome...
,
Apr 30 2018
Forgot to add a hint, if you want to test @Voice: to scroll faster sideways, as regular scrolling animates pages, make a pinching gesture like to zoom-out, then use the horizontal scroll bar that appears at the bottom. Email me directly for a no-ads license if you want easier testing without ads popping up every 30 minutes or banner ad at the bottom.
,
May 1 2018
Is there any hope that you can FIX this fix? You are really breaking existing apps without any possibility to work-around the issue, and no alternatives to Android WebView control in sight. Maybe the fix could be to allow very high or unlimited columns in WebView control only, and limit it in the browser? The only work-around I can think of would be to split original text into even smaller chunks, but it will not work right for all users. Read-aloud apps like @Voice tend to have a high percentage of visually impaired users or older users, who set very large font sizes for their reading apps, that would multiply the number of pages (columns), easily exceeding the very low limit you permit even for shorter chunks of text.
,
May 1 2018
I think we need to increase the limit drastically, and also make sure that we don't detect "illegitimate reasons" unless they truly are illegitimate. A whole lot of text certainly *isn't* an illegitimate reason. <div style="height:1234567890px;"></div> with nothing inside *is* an illegitimate reason.
,
May 5 2018
Please change the status of this bug. It is not FIXED. The fix causes serious problems in scenarios described above.
,
May 6 2018
500 is too low. I think we need to base the column count allowance on the amount of content somehow. Sometimes you may have a legitimate reason to use a few thousand columns, just like there are legitimate reasons for a few thousand lines. The difference between lines and columns, though, is that for lines there's a rather linear correspondence between the amount of content and the number of lines you get (roughly: you'll never get more lines than the number of words that you have). Not so for columns. All it takes is a column height of 1px and an empty DIV child that's 1000000px tall. Bam! One million useless columns! That's the situation we want to avoid.
,
May 6 2018
,
May 13 2018
A recent update changed the behavior of css columns. It went from calculating and applying width for everything (but only painting first 500) to only doing so for the first 500 and then render remaining vertically. See video for visual example: https://bugs.chromium.org/p/chromium/issues/attachment?aid=334043&signed_aid=oZo51EyyvuLNMcI0htdR2Q==&inline=1 This was a breaking change for us, since we relied on the scroll width to work around pages above 500 not painting. Applying display: none to previous columns content while maintaining the scroll width by setting the container elements absolute left position property to the hidden elements width. Now, since the scroll width never change when columns count >= 500 and remaining content is rendered vertically, the workaround now have to take scroll height into account. Anyway, our users was suddenly unable to read past page 500 in our ePub reader due to this update. Why does Blink have an opinion about column count? Other engines renders large amounts just fine. 500 columns are very easily reached on a smartphone screen rendering a book. We are now moving away from loading the whole book and instead load one chapter at a time. But there is no guarantee that some book can't contain a chapter taking up more than 500 columns (small screens and users can adjust font size etc.). So please remove this limit.
,
May 14 2018
Thank you @mik... for your support here, till now it looked like I was the only one affected in the world. Here is a simple test web page to demo the issue http://hyperionics.com/test/li.html which I submitted with Issue 837994 , now merged with this one. Maybe with with more developers and end users complaining, the Chrome gods will finally take us seriously...
,
May 14 2018
Also worth mentioning, multi-column support in Chrome and Android WebView is a disaster. There is another related issue, long sitting as "assigned" to some minor deity and forgotten, also happens only in Chrome, not other browsers: https://bugs.chromium.org/p/chromium/issues/detail?id=797591
,
May 14 2018
Thank you for your feedback. I'm going to propose a fix for this today. Just to be clear: This bug is about over-eager clamping of actual column-count, and nothing else.
,
May 15 2018
One more side-effect for this. When rest of content is rendered in last column layer with content can become incredible large. For example I have book that previously rendered into 700 colums. Now it will be rendered into 500 columns and content of 200 columns will be rendered into last. So layer with text content from (700 * screen_height) become (500 * 200 * screen_height). It's extremely large.
,
May 15 2018
One more side-effect for this. When rest of content is rendered in last column layer with content can become incredible large. For example I have book that previously rendered into 700 colums. Now it will be rendered into 500 columns and content of 200 columns will be rendered into last. So layer with text content from (700 * screen_height) become (500 * 200 * screen_height). It's extremely large.
,
May 15 2018
Indeed, thank you for pointing this out, @yavano! This may be the reason why I see now so many crashes related to "out of memory" condition in my app. I hope that the fix is implemented really soon and makes it rapidly into the full release version of Chrome/WebView on Android at least.
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1c7424a87fff7f5c4351719597eae8051e96fba3 commit 1c7424a87fff7f5c4351719597eae8051e96fba3 Author: Morten Stenshorne <mstensho@chromium.org> Date: Fri May 18 23:04:20 2018 Improve used column count allowance calculation. Better detection of "legitimate" reasons to use many columns. Let the allowance be based on the amount of "content". The more content (lines or boxes) inside a multicol container, the more columns will be allowed. We used to base it on the column height, but it turned out that it wasn't good enough. Also, there are legitimate reasons to use more than 500 columns in some cases, so increase the limit to 2000. With the current implementation, this may get very slow, because of quadratic performance complexity, presumably somewhere in paint code. One multicol regressed in LayoutNG. Not sure why. It shouldn't have passed in the first place. Bug: 808189 Change-Id: Ia45d52b23f1246331d4b193839d34d7d9527151b Reviewed-on: https://chromium-review.googlesource.com/1057629 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Commit-Queue: Morten Stenshorne <mstensho@chromium.org> Cr-Commit-Position: refs/heads/master@{#560088} [modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/WebKit/LayoutTests/fast/multicol/scrollable-basic.html [modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.cc [modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.h [modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.cc [modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.h [modify] https://crrev.com/1c7424a87fff7f5c4351719597eae8051e96fba3/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group_test.cc
,
May 19 2018
Please just remove the limit for legitimate columns. If a developer wants X columns shouldn't the browser try to do that, even if the result is taking a long time or even a crash? What does Firefox do? Note that even a basic test like (attached) is currently broken in Chrome, but doesn't "overflow into vertical" in Firefox, Edge, and IE.
,
May 19 2018
Your test should pass with the latest fix, because each line box will effectively increase the column allowance by 1. We need some limit for now, because time to lay out / invalidate / paint sometimes increases quadratically with the number of columns. That's bug 797591 . We can close *this* bug now, since I think all legitimate use cases for many columns will be allowed now. We still set a hard limit of 2000 columns, because 2000 * 2000 is ... a lot. :(
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/215f05199a9205ed75f728ff6343e895c05b4025 commit 215f05199a9205ed75f728ff6343e895c05b4025 Author: calamity <calamity@chromium.org> Date: Tue May 22 06:11:13 2018 Revert "Improve used column count allowance calculation." This reverts commit 1c7424a87fff7f5c4351719597eae8051e96fba3. Reason for revert: Causing flakes on MultiColumnFragmentainerGroupTest.LotsOfContent. See https://crbug.com/845155 Original change's description: > Improve used column count allowance calculation. > > Better detection of "legitimate" reasons to use many columns. Let the > allowance be based on the amount of "content". The more content (lines > or boxes) inside a multicol container, the more columns will be allowed. > We used to base it on the column height, but it turned out that it > wasn't good enough. Also, there are legitimate reasons to use more than > 500 columns in some cases, so increase the limit to 2000. With the > current implementation, this may get very slow, because of quadratic > performance complexity, presumably somewhere in paint code. > > One multicol regressed in LayoutNG. Not sure why. It shouldn't have > passed in the first place. > > Bug: 808189 > Change-Id: Ia45d52b23f1246331d4b193839d34d7d9527151b > Reviewed-on: https://chromium-review.googlesource.com/1057629 > Reviewed-by: Chris Harrelson <chrishtr@chromium.org> > Reviewed-by: Emil A Eklund <eae@chromium.org> > Commit-Queue: Morten Stenshorne <mstensho@chromium.org> > Cr-Commit-Position: refs/heads/master@{#560088} TBR=chrishtr@chromium.org,eae@chromium.org,mstensho@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 808189 , 845155 Change-Id: I28bf3894c4b65a75c7c533a362ca58f3c7e29973 Reviewed-on: https://chromium-review.googlesource.com/1068587 Reviewed-by: calamity <calamity@chromium.org> Commit-Queue: calamity <calamity@chromium.org> Cr-Commit-Position: refs/heads/master@{#560496} [modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/WebKit/LayoutTests/fast/multicol/scrollable-basic.html [modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.cc [modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.h [modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.cc [modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.h [modify] https://crrev.com/215f05199a9205ed75f728ff6343e895c05b4025/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group_test.cc
,
May 22 2018
What does the above post 'Revert "Improve used column count allowance calculation."' mean? Was the fix reverted and we're still stuck with all text appended to certain column after some low limit? In my tests this low limit was not 500, but about 150 screens of text. Please explain the situation, I'm very worried now.
,
May 22 2018
Yes, it was reverted, because the tests included caused timeouts on some test bots. I've uploaded a new patch with less demanding tests, and I hope we'll be able to re-land this today or tomorrow. You can follow the progress here, if you like: https://chromium-review.googlesource.com/c/chromium/src/+/1068929
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d123c14a79589cb40efc367a178213a0d246f06a commit d123c14a79589cb40efc367a178213a0d246f06a Author: Morten Stenshorne <mstensho@chromium.org> Date: Tue May 22 22:47:14 2018 Reland: Improve used column count allowance calculation. Better detection of "legitimate" reasons to use many columns. Let the allowance be based on the amount of "content". The more content (lines or boxes) inside a multicol container, the more columns will be allowed. We used to base it on the column height, but it turned out that it wasn't good enough. Also, there are legitimate reasons to use more than 500 columns in some cases, so increase the limit to 2000. With the current implementation, this may get very slow, because of quadratic performance complexity, presumably somewhere in paint code. One multicol regressed in LayoutNG. Not sure why. It shouldn't have passed in the first place. This CL was previously landed as https://chromium-review.googlesource.com/c/chromium/src/+/1057629 and then reverted because of test flakiness. This new CL has reduced the number of columns in those tests, so that the tests don't occasionally time out on slow bots. Bug: 808189 , 845155 Change-Id: I55d560faba73128518a689ae38f628ee89e58568 Reviewed-on: https://chromium-review.googlesource.com/1068929 Commit-Queue: Emil A Eklund <eae@chromium.org> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Cr-Commit-Position: refs/heads/master@{#560820} [modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/WebKit/LayoutTests/TestExpectations [modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/WebKit/LayoutTests/fast/multicol/scrollable-basic.html [modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.cc [modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.h [modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.cc [modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.h [modify] https://crrev.com/d123c14a79589cb40efc367a178213a0d246f06a/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group_test.cc
,
May 23 2018
Okay, the fix is in again. Hope it sticks this time.
,
May 23 2018
Great, thank you! I already see the fix working in my test when I set WebView to Chrome Canary channel!
,
Jan 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/64752ee4b1b93f069b2d64a4327af81319f45191 commit 64752ee4b1b93f069b2d64a4327af81319f45191 Author: Morten Stenshorne <mstensho@chromium.org> Date: Wed Jan 02 16:21:50 2019 Less restrictive internal column count limit. Just allow up to 10000 columns unconditionally. We used to have severe performance problems with many columns (O(n^2)), but that should be long gone, thanks to https://chromium-review.googlesource.com/c/chromium/src/+/1070562/ This is essentially a revert of https://chromium-review.googlesource.com/c/chromium/src/+/1068929/ (and earlier clamping attempts). Bug: 918112 , 808189 Change-Id: I3c767fa4f2da5197d1a02c8f4ad89fbc35c5614d Reviewed-on: https://chromium-review.googlesource.com/c/1392955 Commit-Queue: Emil A Eklund <eae@chromium.org> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Cr-Commit-Position: refs/heads/master@{#619413} [modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.cc [modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/layout/layout_multi_column_flow_thread.h [modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.cc [modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group.h [modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/layout/multi_column_fragmentainer_group_test.cc [modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc [modify] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/web_tests/TestExpectations [add] https://crrev.com/64752ee4b1b93f069b2d64a4327af81319f45191/third_party/blink/web_tests/external/wpt/css/css-multicol/large-actual-column-count.html [delete] https://crrev.com/702131a0cbed54f73277fe87b544bcc9f74f40b9/third_party/blink/web_tests/fast/multicol/column-clamping-expected.html [delete] https://crrev.com/702131a0cbed54f73277fe87b544bcc9f74f40b9/third_party/blink/web_tests/fast/multicol/column-clamping.html |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by mstensho@chromium.org
, Feb 1 2018