Bug 161713

Summary: ol.start may return incorrect value for reversed lists when not explicitly set
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, esprehn+autocc, gyuyoung.kim, kling, koivisto, kondapallykalyan, rniwa, zalan
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Patch
none
Patch
none
Patch
none
Patch
none
Fix style
none
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 none

Chris Dumez
Reported 2016-09-07 14:19:04 PDT
ol.start may return incorrect value for reversed lists when not explicitly set. This is because we're supposed to return the number of rendered <li> child elements, which relies on layout. However, we do not make sure the layout is up-to-date before counting the number of li child elements.
Attachments
Patch (5.63 KB, patch)
2016-09-07 14:23 PDT, Chris Dumez
no flags
Archive of layout-test-results from ews112 for mac-yosemite (2.14 MB, application/zip)
2016-09-07 15:56 PDT, Build Bot
no flags
Patch (6.58 KB, patch)
2016-09-07 16:38 PDT, Chris Dumez
no flags
Patch (6.54 KB, patch)
2016-09-08 19:16 PDT, Chris Dumez
no flags
Patch (8.04 KB, patch)
2016-09-08 19:41 PDT, Chris Dumez
no flags
Patch (8.92 KB, patch)
2016-09-08 19:59 PDT, Chris Dumez
no flags
Fix style (3.04 KB, patch)
2016-09-11 15:36 PDT, Chris Dumez
no flags
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (5.80 MB, application/zip)
2016-09-11 17:11 PDT, Build Bot
no flags
Chris Dumez
Comment 1 2016-09-07 14:19:30 PDT
Chris Dumez
Comment 2 2016-09-07 14:23:10 PDT
Build Bot
Comment 3 2016-09-07 15:56:04 PDT
Comment on attachment 288185 [details] Patch Attachment 288185 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2028634 New failing tests: imported/w3c/web-platform-tests/html/semantics/grouping-content/the-ol-element/ol.start-reflection-2.html imported/blink/fast/html/reversed-ol.html fast/lists/ol-reversed-simple.xhtml fast/lists/ol-reversed-dynamic-simple.html imported/w3c/web-platform-tests/html/semantics/grouping-content/the-ol-element/grouping-ol-rev-reftest-001.html fast/regions/counters/extract-ordered-lists-in-regions-002.html fast/lists/ol-reversed-nested-list.html imported/w3c/web-platform-tests/html/semantics/grouping-content/the-ol-element/grouping-ol.html fast/lists/ol-reversed-simple.html fast/lists/ol-reversed-dynamic.html imported/blink/fast/dom/shadow/ol-with-distribution-recalc-crash.html fast/regions/counters/extract-list-items-007.html fast/regions/counters/extract-ordered-lists-in-regions-003.html fast/lists/ol-reversed-nested-items.html
Build Bot
Comment 4 2016-09-07 15:56:08 PDT
Created attachment 288198 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Chris Dumez
Comment 5 2016-09-07 16:38:38 PDT
Chris Dumez
Comment 6 2016-09-08 19:07:51 PDT
antti, kling, could you please take a look?
Chris Dumez
Comment 7 2016-09-08 19:16:26 PDT
zalan
Comment 8 2016-09-08 19:37:28 PDT
Comment on attachment 288376 [details] Patch Chris and I are discussing it on irc. He is going to update it.
Chris Dumez
Comment 9 2016-09-08 19:41:36 PDT
zalan
Comment 10 2016-09-08 19:50:33 PDT
Comment on attachment 288380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288380&action=review > Source/WebCore/ChangeLog:12 > + ol.start may return incorrect value for reversed lists when not explicitly set. > + This is because we're supposed to return the number of rendered <li> child > + elements, which relies on layout. However, we did not make sure the layout is > + up-to-date before counting the number of li child elements. This patch fixes > + the issue. Could you also mention that the reason why we have this 2 functions is because one of the codepath is for the case when the render tree is being mutated while in layout. (which is completely bogus and should be fixed.)
Chris Dumez
Comment 11 2016-09-08 19:59:03 PDT
Chris Dumez
Comment 12 2016-09-08 20:33:23 PDT
Comment on attachment 288381 [details] Patch Clearing flags on attachment: 288381 Committed r205689: <http://trac.webkit.org/changeset/205689>
Chris Dumez
Comment 13 2016-09-08 20:33:30 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 14 2016-09-08 21:58:55 PDT
*** Bug 148861 has been marked as a duplicate of this bug. ***
Darin Adler
Comment 15 2016-09-11 09:25:34 PDT
Comment on attachment 288381 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288381&action=review > Source/WebCore/html/HTMLOListElement.h:53 > + enum class ShouldLayout { No, Yes }; > + WEBCORE_EXPORT unsigned itemCount(ShouldLayout) const; Style-wise, I would much rather see two functions rather than a function with this kind of argument. The one that triggers layout can call the other.
Chris Dumez
Comment 16 2016-09-11 15:35:46 PDT
Reopen to fix style as suggested by Darin.
Chris Dumez
Comment 17 2016-09-11 15:36:08 PDT
Created attachment 288535 [details] Fix style
Build Bot
Comment 18 2016-09-11 17:11:05 PDT
Comment on attachment 288535 [details] Fix style Attachment 288535 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2055654 New failing tests: fast/scrolling/ios/scroll-events-back-forward.html
Build Bot
Comment 19 2016-09-11 17:11:10 PDT
Created attachment 288537 [details] Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
WebKit Commit Bot
Comment 20 2016-09-12 02:30:56 PDT
Comment on attachment 288535 [details] Fix style Clearing flags on attachment: 288535 Committed r205806: <http://trac.webkit.org/changeset/205806>
WebKit Commit Bot
Comment 21 2016-09-12 02:31:02 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.