[css-lists] Implement list numbering using a built-in 'list-item' counter
Categories
(Core :: Layout: Generated Content, Lists, and Counters, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: dbaron, Assigned: MatsPalmgren_bugz, Mentored)
References
(Depends on 1 open bug, Blocks 1 open bug, )
Details
(4 keywords, Whiteboard: [lang=c++][DevRel:P3][wptsync upstream])
Attachments
(6 files, 4 obsolete files)
12.43 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
30.38 KB,
patch
|
emilio
:
review-
|
Details | Diff | Splinter Review |
32.24 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
27.61 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
4.44 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
14.56 KB,
patch
|
emilio
:
feedback+
|
Details | Diff | Splinter Review |
Now that we implement counters (bug 3247), using them for list numbering would fix a significant number of list numbering bugs that we have. A patch containing some of the necessary code removal is in bug 3247 comment 57 (removed from all later patches). The code that would need to be added would probably involve implementing considerable parts of css3-content's proposals for list handling, using -moz- prefixes. Note that we probably don't want to use counters for disc/square/circle/none types since we probably want to keep the customized drawing code that we use for them.
Reporter | ||
Updated•19 years ago
|
Reporter | ||
Comment 1•19 years ago
|
||
This is the old patch merged to the trunk, plus a few tweaks.
Updated•19 years ago
|
Comment 2•19 years ago
|
||
Why isn't it called mozMarker internally? Similar to other Mozilla implementations of CSS3 proposals/extensions?
Reporter | ||
Comment 3•19 years ago
|
||
(In reply to comment #2) > Why isn't it called mozMarker internally? Similar to other Mozilla > implementations of CSS3 proposals/extensions? Because that just creates more work when we want to drop the -moz- (for things that are in a spec).
Reporter | ||
Comment 4•19 years ago
|
||
(In reply to comment #0) > Note that we probably don't want to use counters for > disc/square/circle/none types since we probably want to keep the customized > drawing code that we use for them. Well, we do need to use counters so that 'list-style-type' on an individual item still works.
Reporter | ||
Comment 5•19 years ago
|
||
Nothing significant here, just some comments reflecting current thoughts.
Reporter | ||
Comment 6•18 years ago
|
||
For a second I was thinking this would be easy, and then I ran into problems with the spec; see comments.
Reporter | ||
Comment 7•18 years ago
|
||
I realized that this bug doesn't require implementing ::marker, and given the dependency on bug 309217 if I want to do it using ::marker, there's probably good reason to do it without.
Reporter | ||
Updated•18 years ago
|
Reporter | ||
Comment 8•18 years ago
|
||
And, FWIW, I wasn't thinking straight about the spec problems in those comments; it's actually fine.
Updated•18 years ago
|
Updated•17 years ago
|
Reporter | ||
Comment 9•17 years ago
|
||
Just a little more work that I had lying around in a tree.
Reporter | ||
Comment 10•17 years ago
|
||
Er, that last patch is smaller -- I think it had the stuff that I thought was step 2 taken out, and maybe a little other work added.
Updated•11 years ago
|
Comment 11•10 years ago
|
||
Is there any chance of this (and related counter stuff) being resurrected any time soon?
Reporter | ||
Comment 12•10 years ago
|
||
I don't think it's a priority for me right now, though I'd be willing to help mentor someone. It involves a good bit of discussion with the CSS working group, figuring out the current state of spec proposals/drafts, etc.
Reporter | ||
Updated•10 years ago
|
Updated•9 years ago
|
Comment 13•9 years ago
|
||
Hi I'm new to the Mozilla developers network as well as OSS contribution and want to work on this bug. Any chance I could get some mentored help and a basic how to start? Thanks!
Reporter | ||
Comment 14•9 years ago
|
||
You probably don't want to try working on this bug until you've successfully worked on a number of other patches first, since this is relatively involved.
Comment 15•7 years ago
|
||
::marker pseudo-element returned to the CSS Pseudo Elements module level 4 spec: https://www.w3.org/TR/css-pseudo-4/#marker-pseudo. Would it help fixing this bug now? It's a pity that we still have the 17-year-old https://bugzilla.mozilla.org/show_bug.cgi?id=4522 blocked by this. Other browsers have no problems with numeric list-style values for any element.
Updated•7 years ago
|
Updated•7 years ago
|
Updated•7 years ago
|
Assignee | ||
Comment 16•5 years ago
|
||
I have some patches that implements this...
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 17•5 years ago
|
||
Assignee | ||
Comment 18•5 years ago
|
||
While I'm here, I'm removing the support for <ul start> which isn't
supported by Chrome, nor is it in the HTML spec. There's an open
issue about <ul> attributes here: https://github.com/whatwg/html/issues/3979
but until that is resolved I think compat with Chrome is better.
I'm disabling the WPT css/css-lists/inheritance.html for now, since
it now triggers bug 1405176. I'm guessing because of
the list-style-image: url("https://example.com/"). It seems to me
this is an existing issue that's just triggered by coincidence.
The change to WPT css/CSS2/lists/counter-reset-increment-002.xht is
simply a bug in the test (both <li> and <li::before> increments
the counter). Removing it on ::before makes the test pass in
both Chrome and Firefox and I think it's in line with the intention
of the test, which is to test incrementing from a negative value to
a positive).
Assignee | ||
Comment 19•5 years ago
|
||
Assignee | ||
Comment 20•5 years ago
|
||
... and as usual there were some HTML tags suppressed in those comments.
Please read the raw text if you're interested :-)
(I really f*cking hate this newfangled Markdown-by-default thing.)
Comment 21•5 years ago
|
||
Comment on attachment 9035117 [details] [diff] [review]
part 2 - [css-lists] Implement display:list-item counters using a built-in 'list-item' CSS counter.
Review of attachment 9035117 [details] [diff] [review]:
::: servo/components/style/style_adjuster.rs
@@ +752,5 @@
// Map <li value=INTEGER> to 'counter-set: list-item INTEGER'.
// https://drafts.csswg.org/css-lists-3/#ua-stylesheet
let e = element.as_ref().unwrap();
if e.local_name() == &*local_name!("li") && e.has_attr(&ns!(), &atom!("value")) {
hmm, so this makes the counter-set property depend on display, and the style of two elements depend on whether the value
attribute is present...
I wonder if this violates some assumptions the style sharing code may be making...
::: testing/web-platform/meta/css/css-lists/inheritance.html.ini
@@ +1,2 @@
+[inheritance.html]
Looks like this test is just testing syntax. Can we replace https://example.com by a local URI instead of disabling the test?
Assignee | ||
Comment 22•5 years ago
|
||
hmm, so this makes the counter-set property depend on display
Yes, this is what the spec says:
https://drafts.csswg.org/css-lists-3/#declaring-a-list-item
A list item is any element with its display property set to list-item.
[...] list items automatically increment the special list-item counter. Unless the counter-increment property manually specifies a different increment for the list-item counter, it must be incremented by 1 on every list item...
Can we replace https://example.com by a local URI instead of disabling the test?
Good idea, I'll try that...
Assignee | ||
Comment 23•5 years ago
|
||
Changing "https://example.com" to use http: instead gives the same error.
Changing it to url("#foo") makes the test fail:
assert_equals: expected "url("#foo")" but got "url("http://web-platform.test:8000/css/css-lists/inheritance.html#foo\")"
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10dbf733e21aff72cc9aced98ae593426c88c3c4&selectedJob=220693190
Interestingly, even after disabling the test completely the same assertion occurs,
but somewhere else:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d31e7f89b92ac249da0145b251eb0925d700423&selectedJob=220623487
I'm not really sure what to make of this... perhaps this assertion is
a permafail on Try at the moment?
Comment 24•5 years ago
|
||
Comment on attachment 9035115 [details] [diff] [review] part 1 - [css-lists] Add an inherited internal UA sheet property (-moz-list-reversed:true|false) to propagate <ol reversed> to its relevant decendants. Review of attachment 9035115 [details] [diff] [review]: ----------------------------------------------------------------- Could you add this property to layout/style/test/test_non_content_accessible_properties.html? How does this interact with display: contents? If I have: <ol reversed> <ol style="display: contents"> <li>First <li>Second <li>Third </ol> </ol> This would change behavior in that case, right? Maybe worth a spec clarification? I'm not quite sure what's supposed to happen here (though it's enough of an edge case I don't think anybody would rely on it). ::: servo/components/style/values/specified/list.rs @@ +124,5 @@ > } > } > + > +/// Specified and computed `-moz-list-reversed` property (for UA sheets only). > +#[cfg(feature = "gecko")] No need for the #[cfg] stuff. It's fine to compile it on Servo as long as it's not used. @@ +134,5 @@ > + /// exclusively used for <ol reversed> in our html.css UA sheet > + True, > +} > + > +impl From<bool> for MozListReversed { If you don't mind much, I'd rather add MozListReversed to: https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/servo/components/style/cbindgen.toml#41 and: https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/layout/style/ServoBindings.toml#386 (changing `bool mMozListReversed;` to `mozilla::StyleMozListReversed mMozListReversed;`) But it's not a big deal I guess.
Comment 25•5 years ago
|
||
Comment on attachment 9035117 [details] [diff] [review] part 2 - [css-lists] Implement display:list-item counters using a built-in 'list-item' CSS counter. Review of attachment 9035117 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I took such a long time to take a look at this :( I think the <li value> part, which is what concerned me, should be removed instead, using the general mapped attributes mechanism. ::: servo/components/style/style_adjuster.rs @@ +750,5 @@ > + return; > + } > + > + // Map <li value=INTEGER> to 'counter-set: list-item INTEGER'. > + // https://drafts.csswg.org/css-lists-3/#ua-stylesheet So this doesn't match the spec, and it seems to me matching the spec is much easier. We should do this independently of the display value, using the mapped attributes stuff. That would make it a normal rule in the presentation hints level, and this block of code and get_li_value_attribute wouldn't be needed. Plus you could override counter-set: none too :) Let me know if I missed something or I can help fixing this. @@ +757,5 @@ > + if self.style.get_counters().clone_counter_set().is_empty() { > + self.style > + .mutate_counters() > + .set_counter_set(ComputedSet::new(vec![CounterPair { > + name : CustomIdent(Atom::from("list-item")), nit: This should use the `atom!()` macro. @@ +764,5 @@ > + } > + return; // 'counter-increment:none' by default in this case > + } > + > + if !self.style.get_counters().clone_counter_increment().is_empty() { So the intention of this is to let the author override this, right? But this wouldn't let authors set counter-increment: none, which is unfortunate... @@ +771,5 @@ > + > + // TODO(mats): <summary> has display:list-item in our UA sheet which we > + // need to workaround here. > + // (e.g breakage: layout/reftests/details-summary/details-in-ol.html) > + if e.local_name() == &*local_name!("summary") { I'd remove this hack if possible, even if it means changing the rendering of that test... ::: servo/ports/geckolib/glue.rs @@ +4463,5 @@ > + use style::values::generics::counters::{CounterPair, CounterSetOrReset}; > + use style::properties::{PropertyDeclaration}; > + > + let prop = PropertyDeclaration::CounterReset(CounterSetOrReset::new(vec![CounterPair { > + name: CustomIdent(Atom::from("list-item")), atom!
Comment 26•5 years ago
|
||
Comment on attachment 9035118 [details] [diff] [review] part 3 - Make nsBulletFrame use the built-in 'list-item' CSS counter and remove the old implementation. Review of attachment 9035118 [details] [diff] [review]: ----------------------------------------------------------------- This looks great assuming we're fine with the behavior change I pointed out for the first patch, and the second patch is fixed-up, thanks for working on this! ::: layout/generic/nsBulletFrame.cpp @@ +812,5 @@ > + auto* fc = PresShell()->FrameConstructor(); > + auto* cm = fc->CounterManager(); > + auto* list = cm->CounterListFor(NS_LITERAL_STRING("list-item")); > + MOZ_ASSERT(list && !list->IsDirty()); > + nsIFrame* listItem = GetParent()->GetContent()->GetPrimaryFrame(); I wonder if there's any inside-bullet + first-line + span case or such this wouldn't handle...
Assignee | ||
Comment 27•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)
if !self.style.get_counters().clone_counter_increment().is_empty() {
So the intention of this is to let the author override this, right? But this
wouldn't let authors set counter-increment: none, which is unfortunate...
Yeah, ideally we should change the initial value for all the counter-*
properties so that we can easily detect an author-specified 'none' value
in this code, e.g. 'auto'. The root of the problem is that the whole
spec rests on the foundation that only 'display:list-item' element's are
"list items", which we can't express in a selector in the UA sheet.
https://drafts.csswg.org/css-lists-3/#declaring-a-list-item
Since you're at the F2F, could you check if the spec people would be OK
with that? (something like: 'auto' computes to 'none' when 'display'
isn't 'list-item', otherwise computes to the relevant 'list-item'
counter value for the respective property)
Assignee | ||
Comment 28•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)
I think the <li value> part, which is what concerned me, should be removed
instead, using the general mapped attributes mechanism.
Mapping <li value=N> to "counter-set: list-item N; counter-increment: none"
means that adjust_for_list_item() will think that counter-increment isn't
set and add thus the default value (assuming it has display:list-item).
It would be implementable if the initial value for counter-increment
is changed to auto though.
I think I'll keep it as is for now... we can adjust it later if
the CSSWG agrees to change the initial value.
nit: This should use the
atom!()
macro.
0:02.14 761 | name : CustomIdent(atom!("list-item")),
0:02.14 | ^^^^^^^^^^^ no rules expected this token in macro call
¯_(ツ)_/¯
Assignee | ||
Comment 29•5 years ago
|
||
I'll fold this into part 1, but it might be easier to review as
a standalone patch on top of that.
How does this interact with display: contents?
I think display:contents shouldn't matter, and that
we handle your example incorrectly currently (3-2-1).
These patches makes us compatible with Chrome (1-2-3).
(changing
bool mMozListReversed;
tomozilla::StyleMozListReversed mMozListReversed;
)
I guess I could've used different values than True/False,
but it seems worth fixing this in any case to avoid name
collisions in the future.
Comment 30•5 years ago
|
||
Comment on attachment 9047090 [details] [diff] [review] Part 1b Review of attachment 9047090 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/src/X11UndefineNone.h @@ +26,5 @@ > #ifdef Always > # undef Always > #endif > + > +// X11/Xlib.h also defines True and False, get rid of those too for Sigh :(
Comment 31•5 years ago
|
||
I got locked out of Bugzilla this morning, so just some conversation that happened over mail then irc:
I mailed:
I got locked out of bugzilla due to too many requests looks like, so I'll comment when I get a different ip, but in case it's helpful before that:
I talked a bit with fantasai yesterday about it, but still didn't get to any conclusion about whether to change the default value or not, can try again today I guess.
Mapping <li value=N> to "counter-set: list-item N; counter-increment:
none"
means that adjust_for_list_item() will think that counter-increment isn't
set and add thus the default value (assuming it has display:list item).
It would be implementable if the initial value for counter-increment
is changed to auto though.I see... Unfortunately adding style adjustments that depend on attribute values is a no-go and will cause incorrect style sharing, so it's not sound and we'd need to think harder about it.
^^^^^^^^^^^ no rules expected this token in macro call
You need to add it to StaticAtoms.py, the same way you'd add them if you wanted to use
nsGkAtoms::list_item
or such.
Mats replied:
I see... Unfortunately adding style adjustments that depend on attribute
values is a no-go and will cause incorrect style sharing, so it's not
sound and we'd need to think harder about it.But the patch does add 'value' to IsAttributeMapped, even though it
also does a style adjustment with the value later... does that still
cause a style sharing problem?Anyway, I think we have a strong case for changing the initial value.
Let me know if I should file a github issue about it...You need to add it to StaticAtoms.py, the same way you'd add them if you
wanted to usensGkAtoms::list_item
or such.Thanks, I'll try that.
/Mats
Then on IRC:
08:39 <@emilio> mats: sent a mail about list items since I cannot comment on bugzilla right now (will probably be able in about half an hour)
08:39 <@emilio> mats: would applying counter-set after counter-increment be an acceptable solution?
08:40 <mats> emilio: thx, I just replied
08:40 <@emilio> mats: then you just need to map <li value> to counter-set
08:42 <@emilio> mats: yeah, even with value in IsAttributeMapped it is still a problem, I think. Style sharing only cares about declarations and selectors. Mapped attributes usually get handled via having a different declaration
08:43 <@emilio> mats: but I can apply the patches and try to come up with a testcase
08:44 <@emilio> mats: we could fix by never inserting <li> elements with value attributes in the style sharing cache, but that's not great...
08:46 <mats> emilio: changing the way we apply counter-set/increment would break normal CSS counters though, right?
08:46 <mats> emilio: what we really need is a different initial value ;-)
08:49 <@emilio> mats: well we haven't shipped counter-set yet, but yes, if done for counter-reset it'd be a behavior change. And yeah, I agree. I can try to talk to tab or fantasai about it for a few minutes, but if you can file the issue that'd be great.
08:50 <mats> emilio: ok, I'll file one
08:51 <@emilio> ty!
08:52 <@emilio> mats: also, yesterday we happened to talk about how broken all list implementations are, and how you were fixing them in Gecko ;)
Assignee | ||
Comment 32•5 years ago
|
||
I'll fold this into part 2.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)
// Map <li value=INTEGER> to 'counter-set: list-item INTEGER'.
// https://drafts.csswg.org/css-lists-3/#ua-stylesheet
So this doesn't match the spec, and it seems to me matching the spec is much
easier. We should do this independently of the display value, using the
mapped attributes stuff.
Yeah, but as we discussed on IRC, I don' think it's possible
to implement it any better ATM. We really need a different
initial value for the counter-* properties for this.
I'll file a github issue about it and hope the CSSWG agrees...
nit: This should use the
atom!()
macro.
fixed
// need to workaround here.
// (e.g breakage: layout/reftests/details-summary/details-in-ol.html)
if e.local_name() == &*local_name!("summary") {
I'd remove this hack if possible, even if it means changing the rendering of
that test...
Fair enough, I added "counter-increment: list-item 0" to the UA sheet
instead, which is what the HTML spec suggests:
https://html.spec.whatwg.org/multipage/rendering.html#the-details-and-summary-elements
(and yeah, that UA rule kind of sucks too, I know)
Comment 33•5 years ago
|
||
Comment on attachment 9047148 [details] [diff] [review] part 2b Review of attachment 9047148 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine, though I think we still need to change the value attribute to use mapped attributes instead of style_adjuster.rs.
Assignee | ||
Comment 34•5 years ago
|
||
addendum for part 2, I'll fold it into that part.
(In reply to Emilio Cobos Álvarez (:emilio) from comment #33)
This looks fine, though I think we still need to change the value attribute
to use mapped attributes instead of style_adjuster.rs.
OK, but we need a different initial value for 'counter-increment' for
that to work properly. This patch implements alternative (A) in
https://github.com/w3c/csswg-drafts/issues/3686#issuecomment-468098431
which seems to be the solution the CSSWG favors.
That is, keep 'none' as the initial value, but don't add the default
'counter-increment' for list items if the property is already set
and includes a value for 'list-item'. This patch makes <li value=N>
map to 'counter-set: list-item N; counter-increment: list-item 0;'
so it inhibits the default increment.
(sorry for abusing the feedback flag as a review flag;
I haven't figured out this archanist stuff yet...)
Comment 35•5 years ago
|
||
Comment on attachment 9047770 [details] [diff] [review] part 2c Review of attachment 9047770 [details] [diff] [review]: ----------------------------------------------------------------- I see, so the advantage of doing proposal (C) in that issue is that you don't need to explicitly map the value attribute to `counter-increment: list-item 0`. But I think this is simpler if web-compatible, so this looks good. I added https://github.com/w3c/csswg-drafts/issues/3686 to the agenda to get a resolution on that issue. If you want to comment the details of the solution you ended up using (in particular the important detail of mapping <li value> to two declarations) that's great, otherwise let me know and I'll do that. Thanks! r=me with the nits addressed. ::: dom/html/HTMLLIElement.cpp @@ +70,5 @@ > + // Map <li value=INTEGER> to 'counter-set: list-item INTEGER; > + // counter-increment: list-item 0;'. > + const nsAttrValue* attrVal = aAttributes->GetAttr(nsGkAtoms::value); > + if (attrVal && attrVal->Type() == nsAttrValue::eInteger) { > + if (!aDecls.PropertyIsSet(eCSSProperty_counter_set)) { I think you could just assert that they're not set, but we should do that all over the place basically, so this is fine for now. ::: servo/components/style/style_adjuster.rs @@ +743,5 @@ > return; > } > + // Note that we map <li value=INTEGER> to 'counter-set: list-item INTEGER; > + // counter-increment: list-item 0;' so we'll return here unless the author > + // explicitly specified something else. Maybe point to the CSSWG issue? @@ +744,5 @@ > } > + // Note that we map <li value=INTEGER> to 'counter-set: list-item INTEGER; > + // counter-increment: list-item 0;' so we'll return here unless the author > + // explicitly specified something else. > + use crate::values::CustomIdent; nit: Maybe move all the `use` statements to the of the function? We don't have many mid-function, but not a big deal. @@ +747,5 @@ > + // explicitly specified something else. > + use crate::values::CustomIdent; > + use crate::values::generics::counters::{CounterPair}; > + let mut counter_increments: Vec<CounterPair<i32>> = vec![]; > + for i in self.style.get_counters().clone_counter_increment().iter() { This reallocates a lot unnecessarily. We could avoid the unnecessary allocation when not mutating the counter with something like a counter_increment_references_list_item() directly in gecko.mako.rs or something, but that's probably not too important since that's not the common case. A trivial improvement that avoids a copy and would also make it slightly easier to follow IMO would be: ``` let mut increments = self.style.get_counters().clone_counter_increment(); if increments.iter().any(|i| i.name.as_atom() == atom!("list-item")) { return; } let reversed = ..; increments.0.push(..); self.style.mutate_counters().set_counter_increment(increments); ``` @@ +759,5 @@ > use crate::values::specified::list::MozListReversed; > let reversed = self.style.get_list().clone__moz_list_reversed() == MozListReversed::True; > let increment = if reversed { -1 } else { 1 }; > + counter_increments.push(CounterPair { > + name : CustomIdent(atom!("list-item")), nit: no space to the right of `name`.
Assignee | ||
Comment 36•5 years ago
•
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #35)
I added https://github.com/w3c/csswg-drafts/issues/3686 to the agenda to get
a resolution on that issue. If you want to comment the details of the
solution you ended up using (in particular the important detail of mapping
<li value> to two declarations) that's great, otherwise let me know and I'll
do that.
I amended my last comment with that detail.
- if (!aDecls.PropertyIsSet(eCSSProperty_counter_set)) {
I think you could just assert that they're not set, but we should do that
all over the place basically, so this is fine for now.
Yeah, I noticed this too, but figured I should follow the established
pattern. But you're right, it's probably better to assert in general.
I'll file that as a follow-up bug.
use crate::values::CustomIdent;
nit: Maybe move all the
use
statements to the of the function? We don't
have many mid-function, but not a big deal.
OK, idk what the preferred style is for these, so I followed a "closest
to first use" rule of thumb.
A trivial improvement that avoids a copy and would also make it slightly
easier to follow IMO would be:
Sounds good. Fwiw, the part that I couldn't figure out how to do was:
increments.0.push(..);
I didn't realize I could access the internal vector like that...
Assignee | ||
Comment 37•5 years ago
|
||
1:12.84 error[E0599]: no method named `as_atom` found for type `values::CustomIdent` in the current scope`
1:12.84 --> servo/components/style/style_adjuster.rs:754:45
1:12.84 |
1:12.84 754 | if increments.iter().any(|i| i.name.as_atom() == atom!("list-item")) {
1:12.84 | ^^^^^^^
1:12.84 |
1:12.84 ::: servo/components/style/values/mod.rs:153:1
1:12.84 |
1:12.84 153 | pub struct CustomIdent(pub Atom);
1:12.84 | --------------------------------- method `as_atom` not found for this
1:12.84 error[E0616]: field `0` of struct `values::generics::counters::CounterIncrement` is private
1:12.84 --> servo/components/style/style_adjuster.rs:760:9
1:12.84 |
1:12.84 760 | increments.0.push(CounterPair {
1:12.84 | ^^^^^^^^^^^^
1:12.85 error[E0599]: no method named `push` found for type `values::generics::counters::Counters<i32>` in the current scope
1:12.85 --> servo/components/style/style_adjuster.rs:760:22
1:12.85 |
1:12.85 760 | increments.0.push(CounterPair {
1:12.85 | ^^^^
It seems I can use i.name.0 to fix the first one (yay, I learned something :) ).
Not sure what to do about the second one though - maybe just make it public somehow?
Comment 38•5 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #37)
Not sure what to do about the second one though - maybe just make it public somehow?
Yeah, you can make it public with pub Box<..>
in:
But I misremembered how our counters stuff is stored, thought it was unconditionally a vector. It's a boxed slice, which is more compact, but not growable.
Given push will reallocate unconditionally, we could do something like:
let increments = self.style.get_counters().clone_counter_increment();
if increments.iter().any(|i| i.name.0 == atom!("list-item")) {
return;
}
let list_increment = CounterPair {
...
};
let increments = increments.iter().cloned().chain(std::iter::once(list_increment));
set_counter_increment(CounterIncrement::new(increments.collect());
Or such. Sorry for the hassle.
Assignee | ||
Comment 39•5 years ago
|
||
No worries, always happy to pick up new Rust skills. :-)
Comment 40•5 years ago
|
||
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/84c996219204 part 1 - [css-lists] Add an inherited internal UA sheet property (-moz-list-reversed:true|false) to propagate <ol reversed> to its relevant decendants. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/ae4e4daebdc4 part 2 - [css-lists] Implement display:list-item counters using a built-in 'list-item' CSS counter. r=emilio https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2e7cfa5484 part 3 - Make nsBulletFrame use the built-in 'list-item' CSS counter and remove the old implementation. r=emilio
Updated•5 years ago
|
Comment 41•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/84c996219204
https://hg.mozilla.org/mozilla-central/rev/ae4e4daebdc4
https://hg.mozilla.org/mozilla-central/rev/6c2e7cfa5484
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16095 for changes under testing/web-platform/tests
Reporter | ||
Comment 43•5 years ago
|
||
I notice that testing/web-platform/meta/css/css-lists/inheritance.html.ini marks a test as disabled due to a bug that's already fixed... is that intentional?
Assignee | ||
Comment 44•5 years ago
|
||
(In reply to David Baron :dbaron: 🏴 ⌚UTC-7 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it) from comment #43)
I notice that testing/web-platform/meta/css/css-lists/inheritance.html.ini marks a test as disabled due to a bug that's already fixed... is that intentional?
Good catch, I didn't notice that bug got fixed.
The test PASS now so I'll just remove that .ini file:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2313d92d15911673321417dcd60c2acac3140aa6
Comment 45•5 years ago
|
||
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7970abe6388b Follow-up: remove unnecessary .ini since the test pass. r=me DONTBUILD
Comment 46•5 years ago
|
||
bugherder |
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/16095 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/cJoUW5k_TAy0JBDYXKs1aw)
Updated•4 years ago
|
Comment 48•4 years ago
|
||
I looked at this for developer docs, and have added a note to the 68 release notes. I don't think there is anything else here that needs documenting but please let me know if I have missed something.
Description
•