Implement symbols() CSS function

RESOLVED FIXED in mozilla35

Status

()

enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks 2 bugs, {css3, dev-doc-complete})

Trunk
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
Implement symbols() function defined in CSS Counter Styles Level 3, which can be used to define anonymous counter styles.
Severity: normal → enhancement
Keywords: css3
(Assignee)

Updated

5 years ago
Depends on: 966166
(Assignee)

Updated

5 years ago
Assignee: nobody → quanxunzhen
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Why there is an eCSSUnit_Function with many independent CSS function units? What's the difference between them? Should we merge all of them into a unified mechanism? What is the preferable method to add this new function, using eCSSUnit_Function or adding a new unit?

I think we may need a new list similar to nsCSSPropList (maybe call CSSFuncList) to provide a unified mechanism to process CSS functions.
Flags: needinfo?(dbaron)
Why do you need a unified mechanism?

eCSSUnit_Function is for when it makes more sense to have a set of functions all have the same unit(and distinguish them as keywords); other units are for when it makes more sense to have a specific unit for that function.
Flags: needinfo?(dbaron)
(Assignee)

Comment 3

5 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #8445122 - Flags: review?(jfkthame)
Comment on attachment 8445122 [details] [diff] [review]
patch

Review of attachment 8445122 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but I think David should take a look, as I'm not really familiar with all the CSS parsing stuff, etc.

::: layout/style/CounterStyleManager.cpp
@@ +1706,5 @@
> +
> +/* virtual */ void
> +AnonymousCounterStyle::GetNegative(NegativeType& aResult)
> +{
> +  aResult.before.AssignLiteral(MOZ_UTF16("-"));

Any reason to use this rather than simply

  aResult.before = '-';

(by analogy with GetSuffix above)?

::: layout/style/nsCSSParser.cpp
@@ +7114,5 @@
> +
> +  bool first = true;
> +  nsCSSValueList* item = symbols.SetListValue();
> +  for (;;) {
> +    // Should also include VARIANT_IMAGE

File a followup bug to add support for this?

::: layout/style/nsRuleNode.cpp
@@ +7063,5 @@
>        break;
>      }
> +    case eCSSUnit_Symbols:
> +      // XXX We should save the raw CSS value so that
> +      // it can be serialized in nsComputedDOMStyle.

What do we currently get?
Attachment #8445122 - Flags: review?(jfkthame) → review?(dbaron)
(Assignee)

Comment 5

5 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #4)
> Comment on attachment 8445122 [details] [diff] [review]
> patch
> 
> Review of attachment 8445122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me, but I think David should take a look, as I'm not
> really familiar with all the CSS parsing stuff, etc.
> 
> ::: layout/style/CounterStyleManager.cpp
> @@ +1706,5 @@
> > +
> > +/* virtual */ void
> > +AnonymousCounterStyle::GetNegative(NegativeType& aResult)
> > +{
> > +  aResult.before.AssignLiteral(MOZ_UTF16("-"));
> 
> Any reason to use this rather than simply
> 
>   aResult.before = '-';
> 
> (by analogy with GetSuffix above)?

I'd prefer replacing the statement in GetSuffix, as it seems that assigning a single char could also cause an additional memory allocating in nsString. It could be a problem for GetNegative as it uses nsString to store the symbols, while it might not a true problem for GetSuffix as the caller could use nsAutoString to receive the result.

> ::: layout/style/nsCSSParser.cpp
> @@ +7114,5 @@
> > +
> > +  bool first = true;
> > +  nsCSSValueList* item = symbols.SetListValue();
> > +  for (;;) {
> > +    // Should also include VARIANT_IMAGE
> 
> File a followup bug to add support for this?

Maybe bug 1024179 is enough? I think they are highly related, and could be fixed together.

> ::: layout/style/nsRuleNode.cpp
> @@ +7063,5 @@
> >        break;
> >      }
> > +    case eCSSUnit_Symbols:
> > +      // XXX We should save the raw CSS value so that
> > +      // it can be serialized in nsComputedDOMStyle.
> 
> What do we currently get?

We currently get an empty string for this case.
(Assignee)

Comment 6

5 years ago
(In reply to Xidorn Quan (UTC+10) from comment #5)
> (In reply to Jonathan Kew (:jfkthame) from comment #4)
> > Comment on attachment 8445122 [details] [diff] [review]
> > patch
> > 
> > ::: layout/style/nsRuleNode.cpp
> > @@ +7063,5 @@
> > >        break;
> > >      }
> > > +    case eCSSUnit_Symbols:
> > > +      // XXX We should save the raw CSS value so that
> > > +      // it can be serialized in nsComputedDOMStyle.
> > 
> > What do we currently get?
> 
> We currently get an empty string for this case.

I have an idea to fix it now.
(Assignee)

Updated

5 years ago
Attachment #8445122 - Flags: review?(dbaron)
(Assignee)

Comment 7

5 years ago
Posted patch patch (obsolete) — Splinter Review
The updated patch.

dbaron: It seems you are busy now. I'm fine with either you redirecting the review request to someone else, or doing the review when you are back from your busyness.
Attachment #8445122 - Attachment is obsolete: true
Attachment #8479601 - Flags: review?(dbaron)
Comment on attachment 8479601 [details] [diff] [review]
patch

nsCounterManager.cpp:

>+            mCounterStyle = manager->BuildCounterStyle(
>+                    nsDependentString(style.GetStringBufferValue()));

This should instead use:

  nsString ident; /* or other appropriate name for variable */
  style.GetStringValue(ident);
  manager->BuildCounterStyle(ident);

This reference counts the string buffer instead of building a dependent
buffer, which allows further sharing the buffer with the cache table.


symbols-function-invalid{,-ref}.html:

  You should make the first declaration be lower-greek or similar, so
  that it's more clear that that's the one declaration that's used
  rather than one of the later ones.

CounterStyleManager.cpp:

>+/* virtual */ bool
>+AnonymousCounterStyle::IsOrdinalInAutoRange(CounterValue aOrdinal)
>+{
>+  return IsOrdinalInRange(aOrdinal);
>+}

Maybe return AnonymousCounterStyle::IsOrdinalInRange(aOrdinal) so that
you don't go through virtual dispatch a second time?

>+/* virtual */ uint8_t
>+AnonymousCounterStyle::GetSpeakAs()
>+{
>+  switch (mSystem) {
>+    case NS_STYLE_COUNTER_SYSTEM_ALPHABETIC:
>+      return NS_STYLE_COUNTER_SPEAKAS_SPELL_OUT;
>+    case NS_STYLE_COUNTER_SYSTEM_CYCLIC:
>+      return NS_STYLE_COUNTER_SPEAKAS_BULLETS;
>+    default:
>+      return NS_STYLE_COUNTER_SPEAKAS_NUMBERS;
>+  }
>+}

Maybe put this in a common function to share with the second half
of CustomCounterStyle::GetSpeakAsAutoValue?

(In general, I wonder if it might make sense to share a little more
code with CustomCounterStyle, at the cost of having slightly weaker
assertions about mSystem?   A bunch of these functions could be shared
a bit better.  Maybe worth doing in a followup patch?)

nsCSSParser.cpp:

>+  bool ParseCounterStyleName(nsCSSValue& aValue);

I'm not happy about having three functions with the same name that
aren't true overloads (i.e., same semantics but different parameters).
Is there any better naming that could be done here?

(If you were starting from the beginning, it also would have been
nice to put that refactoring in a separate patch.)

CSSParserImpl::ParseSymbols:

>+  if (!ParseEnum(type, nsCSSProps::kCounterSystemKTable)) {
>+    type.SetIntValue(NS_STYLE_COUNTER_SYSTEM_SYMBOLIC, eCSSUnit_Enumerated);
>+  } else {
>+    switch (type.GetIntValue()) {
>+      case NS_STYLE_COUNTER_SYSTEM_CYCLIC:
>+      case NS_STYLE_COUNTER_SYSTEM_NUMERIC:
>+      case NS_STYLE_COUNTER_SYSTEM_ALPHABETIC:
>+      case NS_STYLE_COUNTER_SYSTEM_SYMBOLIC:
>+      case NS_STYLE_COUNTER_SYSTEM_FIXED:
>+        break;
>+      default:
>+        SkipUntil(')');
>+        return false;
>+    }
>+  }

You should create a separate nsCSSProps::...KTable with just the 5
values that are valid here.  Then you can remove the entire else{} above.

>+    // Should also include VARIANT_IMAGE

Could you file a followup bug on adding support for image,
and reference it in this comment?  (Also, start the comment with
"FIXME: ".)

You also need to add "case ecSSUnit_Symbols:" to
nsCSSValue::SizeOfExcludingThis.

nsComputedDOMStyle.cpp:

>+    tmp.Truncate(tmp.Length() - 1);
>+    tmp.Append(')');

This could be a single call to ReplaceLiteral.

nsRuleNode.cpp:

>+    case eCSSUnit_Symbols:
>+      list->SetListStyleType(NS_LITERAL_STRING(""),
>+                             mPresContext->CounterStyleManager()->
>+                             BuildCounterStyle(typeValue->GetArrayValue()));
>+      break;

Please indent as:

+    case eCSSUnit_Symbols:
+      list->SetListStyleType(NS_LITERAL_STRING(""),
+                             mPresContext->CounterStyleManager()->
+                               BuildCounterStyle(typeValue->GetArrayValue()));
+      break;

(that is, with two more spaces on the line that's a continuation from ->.)

I think you also need to revise nsComputedDOMStyle::DoGetContent.

property_database.js:

Please add tests to the counter() and counters() functions as well.

reftests:

Please add reftests covering symbols() inside of counter() and counters().

It might also make sense to add tests that the symbols() function is
not accepted as a value of @counter-style descriptors:
  system: extends symbols();
  speak-as: symbols();
  fallback: symbols();

r=dbaron with that, although I'm a little suspicious that there may be
additional code missing for the case of symbols() inside of counter() and
counters(), in which case I'd like to review that code as well.  (I should
probably review the DoGetContent changes, at least.)
Attachment #8479601 - Flags: review?(dbaron) → review+
(Assignee)

Updated

5 years ago
Blocks: 1071436
(Assignee)

Comment 9

5 years ago
Posted patch incremental patch (obsolete) — Splinter Review
Attachment #8493572 - Flags: review?(dbaron)
(Assignee)

Comment 10

5 years ago
Posted patch incremental patch (obsolete) — Splinter Review
Sorry for the noise. One change I forgot to include.
Attachment #8493572 - Attachment is obsolete: true
Attachment #8493572 - Flags: review?(dbaron)
Attachment #8493573 - Flags: review?(dbaron)
(Assignee)

Comment 11

5 years ago
All tests in Linux x64 Opt are passed: https://tbpl.mozilla.org/?tree=Try&rev=fe8bb377a98f
Attachment #8493573 - Flags: review?(dbaron) → review+
(Assignee)

Comment 12

5 years ago
Posted patch patch, r=dbaron (obsolete) — Splinter Review
The merged version of the two patches.
Attachment #8479601 - Attachment is obsolete: true
Attachment #8493573 - Attachment is obsolete: true
Attachment #8494816 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Looks like an #include problem affecting non-unified builds. You can add the configure flag --disable-unified-compilation in your mozconfig to reproduce and test this locally.
(Assignee)

Comment 16

5 years ago
I add an "#include <nsCSSValue.h>" to CounterStyleManager.h, and it should be good now: https://tbpl.mozilla.org/?tree=Try&rev=9e1e941ba256
Attachment #8494816 - Attachment is obsolete: true
Attachment #8495133 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1da6f9114503
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
We triaged recent bugs in need of doc (new process).
Documenting this bug means updating this page (and Fx 35 for devs):
https://developer.mozilla.org/en-US/docs/Web/CSS/list-style-type
and creating this one:
https://developer.mozilla.org/en-US/docs/Web/CSS/symbol
You need to log in before you can comment on or make changes to this bug.