Summary: | Special list-item counter starts from an incorrect number for ::before and ::after | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Roman Komarov <kizmarh> | ||||
Component: | CSS | Assignee: | Darin Adler <darin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, darin, dbates, simon.fraser, webkit-bug-importer, zalan | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Roman Komarov
2017-12-21 09:35:00 PST
Specification language for this is quite unclear. Nothing seems to indicate that the "list-item" counter should be affected by the start attribute on the <ul> element or the value attribute on the <li> element. Nothing clearly defines what the behavior should be if someone specifies counter-increment or counter-reset on the list item. The "fix/workaround" is not compatible with the fix I made for WebKit. I could probably make a better fix if I had a more extensive test suite, demonstrating how counter-increment and counter-reset would affect things. Has the Edge team contributed a test case for this to the CSS working group? There is some "sample HTML style sheet" that helps demonstrate the behavior they want. Created attachment 330668 [details]
Patch
Comment on attachment 330668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330668&action=review > Source/WebCore/rendering/RenderListItem.cpp:203 > + auto* list = enclosingList(*this); > + auto* orderedList = is<HTMLOListElement>(list) ? downcast<HTMLOListElement>(list) : nullptr; > > // FIXME: This recurses to a possible depth of the length of the list. > - // That's not good -- we need to change this to an iterative algorithm. > - if (RenderListItem* previousItem = previousListItem(list, *this)) > - return previousItem->value() + valueStep; > - > - if (oListElement) > - return oListElement->start(); > + // That's not good, and we can and should change this to an iterative algorithm. > + if (auto* previousItem = previousListItem(list, *this)) { > + m_value = previousItem->value() + (orderedList && orderedList->isReversed() ? -1 : 1); Couldn't you just use isInReversedOrderedList() here? > Source/WebCore/rendering/style/CounterDirectives.h:37 > +struct CounterDirectives { > + std::optional<int> resetValue; > + std::optional<int> incrementValue; This is a really nice cleanup (class -> struct). Comment on attachment 330668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330668&action=review >> Source/WebCore/rendering/RenderListItem.cpp:203 >> + m_value = previousItem->value() + (orderedList && orderedList->isReversed() ? -1 : 1); > > Couldn't you just use isInReversedOrderedList() here? I’ll do that, or something like it, in my next patch, which fixes the "iterative algorithm" comment here too. Comment on attachment 330668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330668&action=review >>> Source/WebCore/rendering/RenderListItem.cpp:203 >>> + m_value = previousItem->value() + (orderedList && orderedList->isReversed() ? -1 : 1); >> >> Couldn't you just use isInReversedOrderedList() here? > > I’ll do that, or something like it, in my next patch, which fixes the "iterative algorithm" comment here too. I could do that, but it would re-do the work of calling enclosingList, which climbs the parent tree, and I don’t want to do that. Comment on attachment 330668 [details] Patch Clearing flags on attachment: 330668 Committed r226613: <https://trac.webkit.org/changeset/226613> All reviewed patches have been landed. Closing bug. |