Bug 131811 - [New Multicolumn] Pagination direction not being honored (e.g., BottomToTop-tb.html)
Summary: [New Multicolumn] Pagination direction not being honored (e.g., BottomToTop-t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords: InRadar
Depends on: 131840
Blocks:
  Show dependency treegraph
 
Reported: 2014-04-17 13:42 PDT by Dave Hyatt
Modified: 2014-04-21 11:07 PDT (History)
7 users (show)

See Also:


Attachments
Patch (16.13 KB, patch)
2014-04-17 18:26 PDT, Dave Hyatt
hyatt: review-
Details | Formatted Diff | Diff
Patch (16.23 KB, patch)
2014-04-17 18:29 PDT, Dave Hyatt
dino: review+
Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2014-04-21 10:58 PDT, Dave Hyatt
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2014-04-17 13:42:06 PDT
This test seems to be ignoring the pagination direction with the new multi-column code. The writing mode is fine, but the bottom-to-top pagination direction is being ignored.
Comment 1 Dave Hyatt 2014-04-17 13:44:20 PDT
Same bug occurs with LeftToRight-rl.html.
Comment 2 Dave Hyatt 2014-04-17 13:47:25 PDT
RightToLeft-lr has the problem also.
Comment 3 Dave Hyatt 2014-04-17 13:50:52 PDT
TopToBottom-bt has the problem too.
Comment 4 Dave Hyatt 2014-04-17 18:26:57 PDT
Created attachment 229610 [details]
Patch
Comment 5 Dave Hyatt 2014-04-17 18:29:47 PDT
Created attachment 229612 [details]
Patch
Comment 6 Dean Jackson 2014-04-17 18:48:49 PDT
Comment on attachment 229612 [details]
Patch

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

> LayoutTests/fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb-expected.html:25
> +<div class="box" onclick="this.classList.toggle('changed')"></div>

Did you mean to leave in the onclick and .box.changed style rules?
Comment 7 Dave Hyatt 2014-04-17 19:11:47 PDT
Fixed in r167478
Comment 8 Simon Fraser (smfr) 2014-04-17 20:01:02 PDT
4 tests broken and the new test crashes:
http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r167479%20(5010)/results.html
Comment 9 WebKit Commit Bot 2014-04-17 22:08:01 PDT
Re-opened since this is blocked by bug 131840
Comment 10 Alexey Proskuryakov 2014-04-17 22:13:25 PDT
Rolled out in <https://trac.webkit.org/r167483>. Simon, it would be helpful if you could roll out when you noticed this, to avoid having the tests fail for hours.
Comment 11 Radar WebKit Bug Importer 2014-04-17 23:09:27 PDT
<rdar://problem/16656269>
Comment 12 Dave Hyatt 2014-04-21 10:17:21 PDT
Well that's weird. Completely passes on my machine still.
Comment 13 Alexey Proskuryakov 2014-04-21 10:29:44 PDT
The results link posted above didn't have a crash log. Here is one from a debug build (release one seems to be pretty much the same, but I don't have a link):

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r167479%20(4041)/fast/multicol/newmulticol/compare-with-old-impl/BottomToTop-tb-crash-log.txt

ASSERTION FAILED: hasColumns()
/Volumes/Data/slave/mavericks-debug/build/Source/WebCore/rendering/RenderBlock.cpp(3390) : unsigned int WebCore::RenderBlock::columnCount(WebCore::ColumnInfo *) const
1   0x110df0a00 WTFCrash
2   0x11358355c WebCore::RenderBlock::columnCount(WebCore::ColumnInfo*) const
3   0x1134bd9ff WebCore::Page::pageCount() const
4   0x10ec18f0d WebKit::WebPage::mainFrameDidLayout()
5   0x10eb672a3 WebKit::WebFrameLoaderClient::dispatchDidLayout()
6   0x112755b86 WebCore::FrameView::performPostLayoutTasks()
7   0x1127556fb WebCore::FrameView::layout(bool)
8   0x1123ddea9 WebCore::Document::implicitClose()
9   0x112719d3b WebCore::FrameLoader::checkCallImplicitClose()
10  0x1127199da WebCore::FrameLoader::checkCompleted()
11  0x112718278 WebCore::FrameLoader::finishedParsing()
Comment 14 Dave Hyatt 2014-04-21 10:58:15 PDT
Created attachment 229809 [details]
Patch

Address the issues that caused the rollout.
Comment 15 WebKit Commit Bot 2014-04-21 11:00:27 PDT
Attachment 229809 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderView.cpp:1266:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Dean Jackson 2014-04-21 11:03:48 PDT
Comment on attachment 229809 [details]
Patch

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

> Source/WebCore/rendering/RenderView.cpp:1268
> +    else if (multiColumnFlowThread())

It doesn't like the else here.
Comment 17 Dave Hyatt 2014-04-21 11:07:46 PDT
Fix landed in r167597.