Details
- Reviewers
emilio - Commits
- rMOZILLACENTRALb7e1b4dd94c6: Bug 1421938 - Add an 'auto' value for the CSS 'quotes' property, and make it…
rMOZILLACENTRAL11a8f9bc0418: Bug 1421938 - Add an 'auto' value for the CSS 'quotes' property, and make it…
rMOZILLACENTRAL89a0866d1aa0: Bug 1421938 - Add an 'auto' value for the CSS 'quotes' property, and make it… - Bugzilla Bug ID
- 1421938
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
So I see multiple issues with this:
- As implemented, this creates a different quote pair on the heap for each element with lang attribute. Maybe not a big deal, but not ideal perf wise, would be cool to share the ArcSlice objects.
- This only handles lang, not other ways to set the content document language (like http headers) which AIUI are still somewhat common, and our other lang-aware code handles via the initial value of -x-lang.
- The behavior for nested lang attributes may or may not be what we want (this probably deserves some CSS WG discussion).
For example, this patch is incompatible with what chrome does in this example:
data:text/html,<style>:root { quotes: "" "" "" ""; } </style><div lang="fr">Foo <q>bar</q> <div lang="es">Baz <q>bazz</q>
Where quotes disappear in chrome but not with this patch applied.
When I was discussing this with Fantasai and Jen Simmons, I proposed that the initial value of quotes should just be auto. Then you can achieve Chrome's behavior by default, and this behavior with [lang] { quotes: auto }. WDYT?
Actually, it seems like this is what Chromium does, kind of. The initial computed value of quotes is null (and indeed they serialize to the empty string by default from getComputedStyle()), and when it's null they apply the lang-dependent quotes logic. It would make sense to me to expose this magic initial value as "auto", wdyt?
For example, this patch is incompatible with what chrome does in this example:
data:text/html,<style>:root { quotes: "" "" "" ""; } </style><div lang="fr">Foo <q>bar</q> <div lang="es">Baz <q>bazz</q>
Where quotes disappear in chrome but not with this patch applied.
Yes, I realize this differs from Chrome here; I figured it was a starting point, not the last word on the matter. :) Note that AIUI the Chrome behavior does not match what the HTML standard calls for at https://html.spec.whatwg.org/multipage/rendering.html#quotes, where the rules that "UAs are expected to use" explicitly reset quotes whenever lang changes.
So yes, I totally agree this needs some kind of discussion.
When I was discussing this with Fantasai and Jen Simmons, I proposed that the initial value of quotes should just be auto. Then you can achieve Chrome's behavior by default, and this behavior with [lang] { quotes: auto }. WDYT?
That sounds like a nice solution to me. If you'd like to put a proposal to the WG, that'd be great. (I suppose if the CSS WG comes to a conclusion here, the WHATWG should also be nudged to update their rendering rules section.)
Code analysis found 7 defects in the diff 128164:
- 7 defects found by clang-format
You can run this analysis locally with:
- ./mach clang-format -s -p layout/base/nsQuoteList.cpp (C/C++)
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.
Rest of the patch looks good, sorry for this, most of what you had to do here should be auto-generated.
| servo/ports/geckolib/cbindgen.toml | ||
|---|---|---|
| 326 | Yeah, this should be auto-generated. I should turn on autogeneration of these by default. The way we've been going about this is using cbindgen:derive-tagged-enum-copy-constructor=true in the type (which gives you this function), like this, and then adding a default private constructor like this so that StyleQuotes::Auto() compiles (and use that in the style struct constructor). These two things should be auto-generated by default, I'll get to that, sorry for the hassle. Could you switch to something like that in the meantime for consistency? | |
| servo/ports/geckolib/glue.rs | ||
| 6609–6610 | You can just remove this function and use StyleQuotes::Auto() in the constructor instead. | |
| servo/ports/geckolib/cbindgen.toml | ||
|---|---|---|
| 326 | Thanks - I figured this was far too ugly, and there must be a better way to do this! Will update the patch. | |
| servo/components/style/values/specified/list.rs | ||
|---|---|---|
| 119–120 | Also, if you change this for repr(transparent) you can remove the ugly _0 in the C++ code :) | |
Please send an intent for this? Looks good to me.
The rust code could be cleaned up / be made slightly faster/smaller, but it's mostly pre-existing and not a big deal :)
r=me
| layout/base/nsQuoteList.cpp | ||
|---|---|---|
| 69 | Span<> is cheap to use by value rather than by reference. And actually, given the function is returning a Span by value this is relying on the C++ lengthening of const reference lifetimes which is quite obscure: https://blog.galowicz.de/2016/03/23/const_reference_to_temporary_object/ So I'd prefer if this kept using just the span by value. | |
Code analysis found 7 defects in the diff 128325:
- 7 defects found by clang-format
You can run this analysis locally with:
- ./mach clang-format -s -p layout/base/nsQuoteList.cpp (C/C++)
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.