Bug 236580

Summary:

[CSS Container Queries] Implement full query parser and evaluator

Product:

WebKit

Reporter:

Antti Koivisto <koivisto>

Component:

CSS

Assignee:

Antti Koivisto <koivisto>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, sam, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=236687

Bug Depends on:

Bug Blocks:

229659

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

Antti Koivisto

Reported 2022-02-14 02:03:04 PST

So far we have been using the media query parser to parse container queries. It does not support nesting and other necessary features. Also the produced data structure are awkward to evaluate.

Attachments
Patch (45.45 KB, patch) 2022-02-14 02:09 PST, Antti Koivisto	no flags	Details Formatted Diff Diff
Patch (45.60 KB, patch) 2022-02-14 03:38 PST, Antti Koivisto	no flags	Details Formatted Diff Diff
Patch (45.94 KB, patch) 2022-02-14 04:08 PST, Antti Koivisto	no flags	Details Formatted Diff Diff
Patch (48.59 KB, patch) 2022-02-14 05:21 PST, Antti Koivisto	no flags	Details Formatted Diff Diff
Patch for landing (48.87 KB, patch) 2022-02-14 10:16 PST, Antti Koivisto	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Antti Koivisto

Comment 1 2022-02-14 02:09:06 PST

Created attachment 451874 [details] Patch

Antti Koivisto

Comment 2 2022-02-14 03:38:51 PST

Created attachment 451880 [details] Patch

Antti Koivisto

Comment 3 2022-02-14 04:08:04 PST

Created attachment 451884 [details] Patch

Antti Koivisto

Comment 4 2022-02-14 05:21:17 PST

Created attachment 451894 [details] Patch

Sam Weinig

Comment 5 2022-02-14 09:30:47 PST

Comment on attachment 451894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451894&action=review > Source/WebCore/css/ContainerQuery.h:45 > +enum class ComparisonOperator : uint8_t { Less, LessOrEqual, Equal, Greater, GreaterOrEqual, True }; I would replace `Less` and `Greater` with `LessThan` and `GreaterThan` (also in the OrEqual variants) as that is usually what we call these. > Source/WebCore/css/ContainerQueryParser.cpp:42 > + bool isSizeQuery = equalIgnoringASCIICase(range.peek().value(), "size"); Could you instead do: bool isSizeQuery = range.peek().functionId() == CSSValueSize; ? > Source/WebCore/css/ContainerQueryParser.cpp:89 > + if (equalIgnoringASCIICase(range.peek().value(), "not")) { Again, can this be compared against the CSSValueNot ident rather than the explicit string check? > Source/WebCore/css/ContainerQueryParser.cpp:143 > + auto name = nameToken.value().toAtomString(); I think delaying the toAtomString() until it is needed might be a little more efficient and avoid an unnecessary atomization in some cases. > Source/WebCore/css/ContainerQueryParser.cpp:146 > + return CQ::SizeFeature { CQ::ComparisonOperator::True, name, { } }; I would WTFMove(name) here. > Source/WebCore/css/ContainerQueryParser.cpp:161 > + name = StringView(name).substring(4).toAtomString(); I can't exactly remember how this works with AtomStrings, but I think you want to instead do something like: name = name.string().substring(4).toAtomString(); to potentially get the shared substring optimization. > Source/WebCore/css/ContainerQueryParser.cpp:170 > + return CQ::SizeFeature { op, name, length }; I would WTFMove(name) here. > Source/WebCore/style/ContainerQueryEvaluator.cpp:186 > + // FIXME: Support all features. An evergreen comment :).

Antti Koivisto

Comment 6 2022-02-14 10:16:40 PST

Created attachment 451921 [details] Patch for landing

EWS

Comment 7 2022-02-14 11:07:12 PST

Committed r289742 (247226@main): <https://commits.webkit.org/247226@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 451921 [details].

Radar WebKit Bug Importer

Comment 8 2022-02-14 11:08:21 PST

<rdar://problem/88916234>

Note You need to log in before you can comment on or make changes to this bug.