ol.start may return incorrect value for reversed lists when not explicitly set

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Relevant spec discussion:
https://github.com/whatwg/html/issues/1750

[Chris Dumez]

Created attachment 288185 [details]
Patch

[Build Bot]

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]

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]

Created attachment 288203 [details]
Patch

[Chris Dumez]

antti, kling, could you please take a look?

[Chris Dumez]

Created attachment 288376 [details]
Patch

[zalan]

Comment on attachment 288376 [details]
Patch

Chris and I are discussing it on irc. He is going to update it.

[Chris Dumez]

Created attachment 288380 [details]
Patch

[zalan]

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]

Created attachment 288381 [details]
Patch

[Chris Dumez]

Comment on attachment 288381 [details]
Patch

Clearing flags on attachment: 288381

Committed r205689: <http://trac.webkit.org/changeset/205689>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

*** Bug 148861 has been marked as a duplicate of this bug. ***

[Darin Adler]

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]

Reopen to fix style as suggested by Darin.

[Chris Dumez]

Created attachment 288535 [details]
Fix style

[Build Bot]

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]

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 on attachment 288535 [details]
Fix style

Clearing flags on attachment: 288535

Committed r205806: <http://trac.webkit.org/changeset/205806>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.