Implement symbols() CSS function

RESOLVED FIXED in mozilla35

Status

()

Core
CSS Parsing and Computation
--
enhancement
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

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

Trunk
mozilla35
css3, dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

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

Updated

4 years ago
Severity: normal → enhancement
Keywords: css3
Depends on: 966166
Assignee: nobody → quanxunzhen
Status: NEW → ASSIGNED
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)
Created attachment 8445122 [details] [diff] [review]
patch
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)
(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.
(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.
Attachment #8445122 - Flags: review?(dbaron)
Created attachment 8479601 [details] [diff] [review]
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.
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+
Blocks: 1071436
Created attachment 8493572 [details] [diff] [review]
incremental patch
Attachment #8493572 - Flags: review?(dbaron)
Created attachment 8493573 [details] [diff] [review]
incremental patch

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)
All tests in Linux x64 Opt are passed: https://tbpl.mozilla.org/?tree=Try&rev=fe8bb377a98f
Attachment #8493573 - Flags: review?(dbaron) → review+
Created attachment 8494816 [details] [diff] [review]
patch, r=dbaron

The merged version of the two patches.
Attachment #8479601 - Attachment is obsolete: true
Attachment #8493573 - Attachment is obsolete: true
Attachment #8494816 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d53b25dcf2d7
Keywords: checkin-needed
sorry had to backout for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=48842844&tree=Mozilla-Inbound
Keywords: dev-doc-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.
Created attachment 8495133 [details] [diff] [review]
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
Attachment #8494816 - Attachment is obsolete: true
Attachment #8495133 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da6f9114503
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1da6f9114503
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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
: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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.