Integrate RenderListMarker with @counter-style #10871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
EWS run on previous version of this PR (hash 1cf6c1b)
|
vitorroriz
force-pushed
the
render-list-marker-with-counter-styles
branch
from
1cf6c1b
to
22664b4
Compare
|
EWS run on previous version of this PR (hash 22664b4)
|
alanbaradlay
reviewed
Mar 2, 2023
| auto text = makeString(counterStyle()->prefix(), counterStyle()->text(m_listItem->value())); | ||
| m_textWithSuffix = makeString(text, counterStyle()->suffix()); | ||
| m_textWithoutSuffixLength = text.length(); | ||
| m_textIsLeftToRightDirection = u_charDirection(text[0]) != U_RIGHT_TO_LEFT; |
There was a problem hiding this comment.
is it ok to just checking the first character? (or go with ubidi_getBaseDirection)
There was a problem hiding this comment.
I've followed the pattern that is already done for all other ListStyleTypes (case default) and case String.
There was a problem hiding this comment.
sure, though that does not necessarily mean it is correct.
There was a problem hiding this comment.
oh look, there's a FIXME about this right there.
// FIXME: Depending on the string value, we may need the real bidi algorithm.
There was a problem hiding this comment.
I see, but we don't have tests failing for now. Shouldn't we investigate this in another radar? I'm adding a new value to ListStyleType.
in RenderListMarker::textRun() there is a comment:
// Since the bidi algorithm doesn't run on this text, we instead reorder the characters here. // We use u_charDirection to figure out if the marker text is RTL and assume the suffix matches the surrounding direction.
I've created rdar://106139180 so we can investigate it. I'll update the FIXME comment with the radar link after current pipeline finish running so it don't get reset.
|
I have one comment on the rendering change. Someone should check the parsing part, looks ok otherwise. |
nt1m
approved these changes
Mar 2, 2023
| @@ -1843,13 +1843,19 @@ void RenderListMarker::updateContent() | |||
| auto type = style().listStyleType(); | |||
| switch (type) { | |||
| // FIXME: handle CSSCounterStyle case rdar://102988393. | |||
| @@ -8281,6 +8281,8 @@ RefPtr<CSSValue> consumeCounterStyleAdditiveSymbols(CSSParserTokenRange& range, | |||
| if (!integer) | |||
| return nullptr; | |||
| } | |||
| if (!symbol) | |||
| return nullptr; | |||
There was a problem hiding this comment.
Can we check for !symbol before checking for !integer and remove the !symbol check inside !integer?
There was a problem hiding this comment.
the symbol and weight can appear in any order [<integer [0,∞]> && <symbol>]#
| else if (valueLowerCase == "square"_s) | ||
| addPropertyToPresentationalHintStyle(style, CSSPropertyListStyleType, CSSValueSquare); | ||
| else if (valueLowerCase == "none"_s) | ||
| addPropertyToPresentationalHintStyle(style, CSSPropertyListStyleType, CSSValueNone); |
vitorroriz
force-pushed
the
render-list-marker-with-counter-styles
branch
from
22664b4
to
9e4bf65
Compare
|
EWS run on previous version of this PR (hash 9e4bf65)
|
vitorroriz
added
merge-queue
vitorroriz
force-pushed
the
render-list-marker-with-counter-styles
branch
from
9e4bf65
to
31a2e47
Compare
|
EWS run on previous version of this PR (hash 31a2e47)
|
vitorroriz
force-pushed
the
render-list-marker-with-counter-styles
branch
from
31a2e47
to
516c158
Compare
|
EWS run on previous version of this PR (hash 516c158)
|
vitorroriz
force-pushed
the
render-list-marker-with-counter-styles
branch
from
516c158
to
d0e52df
Compare
|
EWS run on previous version of this PR (hash d0e52df)
|
vitorroriz
force-pushed
the
render-list-marker-with-counter-styles
branch
2 times, most recently
from
f83220e
to
be0825e
Compare
|
EWS run on previous version of this PR (hash be0825e)
|
nt1m
reviewed
Mar 3, 2023
Comment on lines
49
to
50
| unsigned symbolIndex { 0 }; | ||
| symbolIndex = static_cast<unsigned>(value > 0 ? value : value + amountOfSymbols); |
There was a problem hiding this comment.
Suggested change
| unsigned symbolIndex { 0 }; | |
| symbolIndex = static_cast<unsigned>(value > 0 ? value : value + amountOfSymbols); | |
| unsigned symbolIndex = static_cast<unsigned>(value > 0 ? value : value + amountOfSymbols); |
A comment explaining this is to avoid substracting 1 from the minimum integer would be nice.
Comment on lines
1859
to
1860
| } else | ||
| markerAsString(); |
There was a problem hiding this comment.
This seems incorrect? If the counter-style doesn't exist, it should fallback to decimal I think.
vitorroriz
force-pushed
the
render-list-marker-with-counter-styles
branch
from
be0825e
to
3bf57d2
Compare
|
EWS run on previous version of this PR (hash 3bf57d2)
|
vitorroriz
force-pushed
the
render-list-marker-with-counter-styles
branch
from
3bf57d2
to
e081219
Compare
|
EWS run on previous version of this PR (hash e081219)
|
vitorroriz
force-pushed
the
render-list-marker-with-counter-styles
branch
from
e081219
to
15612fd
Compare
|
EWS run on previous version of this PR (hash 15612fd)
|
vitorroriz
added
merge-queue
vitorroriz
force-pushed
the
render-list-marker-with-counter-styles
branch
from
15612fd
to
f547428
Compare
|
EWS run on current version of this PR (hash f547428)
|
🛠 ios-sim
https://bugs.webkit.org/show_bug.cgi?id=253158 rdar://102988393 Reviewed by Tim Nguyen. This patch integrates RenderListMarker with the parsed counter-style at-rules and fixes some bugs catched during rendering. Since we are wiring things up here, we can stop skipping the related tests. Some tests will still fail because we are missing: - Support to shadow DOM scope. - Integration with counters() function. - Integration with symbols() function. - Complex predefined symbols. - Allowing redefinition of predefined symbols. - Bug on wpt references regarding prefix/suffix fallback. The missing items are linked to their issues in TestExpectations. * LayoutTests/TestExpectations: - Stop skipping the related tests. We are also tagging the tests that we still fail due to missing feature/wpt bugs and we are linking the relevant issues. * LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-at-rule/support/ref-common.css: (div, p): * LayoutTests/imported/w3c/web-platform-tests/css/css-counter-styles/counter-style-at-rule/support/test-common.css: (li, p): - Avoid fuzziness in the relevant tests. This change is also pushed to WPT (web-platform-tests/wpt#38773) * Source/WebCore/css/CSSCounterStyle.cpp: (WebCore::CSSCounterStyle::counterForSystemCyclic const): (WebCore::CSSCounterStyle::applyPadSymbols const): - Fixing bugs in padding and cyclic-system. * Source/WebCore/css/CSSCounterStyleRegistry.cpp: (WebCore::CSSCounterStyleRegistry::resolveExtendsReference): - We were forgetting to pass the map for reference resolution during the resolution of author rules, which would make the user agent map to be used instead, by default. * Source/WebCore/css/CSSProperties.json: - Enabling custom-ident parsing in list-style-type. * Source/WebCore/css/counterStyles.css: (@counter-style cjk-decimal): (@counter-style disclosure-open): (@counter-style disclosure-closed): - Update the symbols for disclosure open/closed. * Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp: (WebCore::CSSPropertyParserHelpers::consumeCounterStyleAdditiveSymbols): - Fixing bug where we would not invalidate additive-symbol with missing symbol. This would cause a crash during rendering. * Source/WebCore/rendering/style/RenderStyle.cpp: (WebCore::rareInheritedDataChangeRequiresLayout): - listStyleStringValue change now requires layout, since it defines the name of the counter-style. * Source/WebCore/rendering/RenderListMarker.cpp: (WebCore::RenderListMarker::updateContent): * Source/WebCore/style/StyleBuilderCustom.h: (WebCore::Style::BuilderCustom::applyValueListStyleType): - Rendering Marker according to counter-style. * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/css/css-lists/parsing/list-style-type-computed-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-lists/parsing/list-style-type-valid-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/the-stylepropertymap/properties/list-style-type-expected.txt: - We have new tests passing after fixing presentation hint mapping for LI and UL elements. * Source/WebCore/css/ComputedStyleExtractor.cpp: (WebCore::ComputedStyleExtractor::valueForPropertyInStyle): * Source/WebCore/html/HTMLLIElement.cpp: (WebCore::HTMLLIElement::collectPresentationalHintsForAttribute): * Source/WebCore/html/HTMLUListElement.cpp: (WebCore::HTMLUListElement::collectPresentationalHintsForAttribute): - Fixing presentational hint mapping for LI and UL elements. Canonical link: https://commits.webkit.org/261135@main
webkit-early-warning-system
force-pushed
the
render-list-marker-with-counter-styles
branch
from
f547428
to
2e241f9
Compare
|
Committed 261135@main (2e241f9): https://commits.webkit.org/261135@main Reviewed commits have been landed. Closing PR #10871 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
2e241f9
f547428
🛠 wpe🛠 wincairo🧪 ios-wk2🧪 api-mac🧪 api-ios🧪 gtk-wk2🛠 tv🧪 mac-wk2🧪 api-gtk🧪 mac-AS-debug-wk2