966168 - Implement symbols() CSS function

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

Implement symbols() function defined in CSS Counter Styles Level 3, which can be used to define anonymous counter styles.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 4

[Jonathan Kew [: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?

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(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.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: incremental patch

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: incremental patch

Sorry for the noise. One change I forgot to include.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

All tests in Linux x64 Opt are passed: https://tbpl.mozilla.org/?tree=Try&rev=fe8bb377a98f

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch, r=dbaron

The merged version of the two patches.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d53b25dcf2d7

Comment 14

[Carsten Book [:Tomcat]]

sorry had to backout for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=48842844&tree=Mozilla-Inbound

Comment 15

[Jonathan Kew [:jfkthame]]

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.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch, r=dbaron

I add an "#include <nsCSSValue.h>" to CounterStyleManager.h, and it should be good now: https://tbpl.mozilla.org/?tree=Try&rev=9e1e941ba256

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1da6f9114503

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1da6f9114503

Comment 19

[Jean-Yves Perrier [:teoli]]

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

Comment 20

[Jean-Yves Perrier [:teoli]]

:jsx documented this a few months ago. Thank you
https://developer.mozilla.org/en-US/Firefox/Releases/35#CSS
https://developer.mozilla.org/en-US/docs/Web/CSS/symbols
and
https://developer.mozilla.org/en-US/docs/Web/CSS/list-style-type