| Summary: | Negative scroll snap repeat values cause web process to hang indefinitely | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | bfulgham, commit-queue, simon.fraser, webkit-bug-importer, wenson_hsieh | ||||||||||
| Priority: | P1 | Keywords: | InRadar | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Wenson Hsieh
2015-07-14 17:15:09 PDT
Created attachment 256816 [details]
Patch
Comment on attachment 256816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256816&action=review > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:90 > + LayoutUnit repeatOffset = points ? valueForLength(points->repeatOffset, viewSize) : LayoutUnit(1.0f); LayoutUnit(1.0f) should be LayoutUnit::fromPixel(1) > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:92 > + if (repeatOffset.toFloat() < 1.0f) > + repeatOffset = LayoutUnit(1.0f); repeatOffset = std::min<LayoutUnit>(repeatOffset, 1); > LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:39 > + <style> > + #gallery { > + height: 400px; > + width: 400px; > + overflow-x: hidden; > + overflow-y: auto; > + -webkit-overflow-scrolling: touch; > + -webkit-scroll-snap-type: mandatory; > + -webkit-scroll-snap-points-y: repeat(-400px); > + margin: 0; > + padding: 0; > + } > + > + .colorBox { > + width: 400px; > + height: 400px; > + } > + > + #item0 { background-color: red; } > + #item1 { background-color: green; } > + #item2 { background-color: blue; } > + #item3 { background-color: aqua; } > + #item4 { background-color: yellow; } > + #item5 { background-color: fuchsia; } > + </style> > + <script src="../../resources/js-test-pre.js"></script> > + </head> > + <body> > + <div id="gallery"> > + <div id="item0" class="colorBox"></div> > + <div id="item1" class="colorBox"></div> > + <div id="item2" class="colorBox"></div> > + <div id="item3" class="colorBox"></div> > + <div id="item4" class="colorBox"></div> > + <div id="item5" class="colorBox"></div> > + </div> Not sure that any of this is needed. Can't you just have body { -webkit-scroll-snap-type: mandatory; -webkit-scroll-snap-points-y: repeat(-400px); } > LayoutTests/css3/scroll-snap/scroll-snap-subpixel-repeat.html:10 > + -webkit-overflow-scrolling: touch; Remove this. > LayoutTests/css3/scroll-snap/scroll-snap-subpixel-repeat.html:34 > + testRunner.waitUntilDone(); Not sure why the test needs to be async. Comment on attachment 256816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256816&action=review >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:90 >> + LayoutUnit repeatOffset = points ? valueForLength(points->repeatOffset, viewSize) : LayoutUnit(1.0f); > > LayoutUnit(1.0f) should be LayoutUnit::fromPixel(1) Fixed! >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:92 >> + repeatOffset = LayoutUnit(1.0f); > > repeatOffset = std::min<LayoutUnit>(repeatOffset, 1); Fixed! (I assumed std::min was meant to be std::max?) >> LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:39 >> + </div> > > Not sure that any of this is needed. Can't you just have body { -webkit-scroll-snap-type: mandatory; -webkit-scroll-snap-points-y: repeat(-400px); } Good point -- fixed! >> LayoutTests/css3/scroll-snap/scroll-snap-subpixel-repeat.html:10 >> + -webkit-overflow-scrolling: touch; > > Remove this. Removed. Created attachment 256818 [details]
Patch
Comment on attachment 256818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256818&action=review > LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:20 > + #item0 { background-color: red; } > + #item1 { background-color: green; } > + #item2 { background-color: blue; } > + #item3 { background-color: aqua; } > + #item4 { background-color: yellow; } > + #item5 { background-color: fuchsia; } Not needed. > LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:30 > + <div id="item0" class="colorBox"></div> > + <div id="item1" class="colorBox"></div> > + <div id="item2" class="colorBox"></div> > + <div id="item3" class="colorBox"></div> > + <div id="item4" class="colorBox"></div> > + <div id="item5" class="colorBox"></div> You still don't need these. > LayoutTests/css3/scroll-snap/scroll-snap-subpixel-repeat.html:56 > + finishJSTest(); > + testRunner.notifyDone(); finishJSTest calls notifyDone for you. Comment on attachment 256818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256818&action=review >> LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:20 >> + #item5 { background-color: fuchsia; } > > Not needed. I left these in because the length of the scrollable content must be greater than the length of the viewport in order to trigger the code path that computes scroll snap offsets (otherwise, the while loop that causes the infinite loop won't be run and the test will pass even without this patch). However, I can make this a bit simpler -- instead of 6 elements, I can just have one really long one. >> LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:30 >> + <div id="item5" class="colorBox"></div> > > You still don't need these. Good point -- took out the colors. >> LayoutTests/css3/scroll-snap/scroll-snap-subpixel-repeat.html:56 >> + testRunner.notifyDone(); > > finishJSTest calls notifyDone for you. Got it. I'll remember that for next time. Created attachment 256823 [details]
Patch
(In reply to comment #7) > Comment on attachment 256818 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256818&action=review > > >> LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:20 > >> + #item5 { background-color: fuchsia; } > > > > Not needed. Just put a height on the body in CSS. Created attachment 256837 [details]
Patch
Comment on attachment 256837 [details] Patch Clearing flags on attachment: 256837 Committed r186840: <http://trac.webkit.org/changeset/186840> All reviewed patches have been landed. Closing bug. |