Bug 46875 – Web Inspector: content: counter displays decimal number instead of name

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

[Andrew Vit]

Inspect an element with a style rule such as:

h3:before {
  content: counter(section) ". ";
  counter-increment: section;
}

In the styles sidebar of Web Inspector, the style rule shows as:

  content: counter(2.4001e-314), '. ';

It should show the name of the counter ("section") instead.

[Joseph Pecoraro]

The cssText implementation of CSS_COUNTER was missing:
http://trac.webkit.org/browser/trunk/WebCore/css/CSSPrimitiveValue.cpp#L714

> 714    case CSS_COUNTER:
> 715        text = "counter(";
> 716        text += String::number(m_value.num);
> 717        text += ")";
> 718        // FIXME: Add list-style and separator
> 719        break;

I have a patch to fix this, I will put it up in a moment after running tests.

[Joseph Pecoraro]

Created attachment 70350 [details]
[PATCH] Implement CSS_COUNTER cssText() properly

This also includes a fix to the "content" property. I'd appreciate it if
someone familiar with CSS counters took a look at this, as this was my
first experience with them.

[Early Warning System Bot]

Attachment 70350 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4301007

[WebKit Review Bot]

Attachment 70350 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4304007

[Eric Seidel (no email)]

Attachment 70350 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4284008

[Joseph Pecoraro]

Created attachment 70354 [details]
[PATCH] Implement CSS_COUNTER cssText() properly

Added missing export?

[Eric Seidel (no email)]

Attachment 70354 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4366002

[Early Warning System Bot]

Attachment 70354 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4275009

[WebKit Review Bot]

Attachment 70354 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4304010

[Joseph Pecoraro]

Created attachment 70694 [details]
[PATCH] Take 3

Attempted adding an include.

[Early Warning System Bot]

Attachment 70694 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4389017

[Joseph Pecoraro]

Any help on this would be appreciated. This compiled fine on my mac in the first patch.
One of my guesses was that I should add CSSPrimitiveValueMapping.h to Counter.h, but
that was producing really weird compiler errors with SVGPaint?!? I can try looking into
that a bit more.

[Eric Seidel (no email)]

Attachment 70694 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4465013

[WebKit Review Bot]

Attachment 70694 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4458010

[Eric Seidel (no email)]

Attachment 70694 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4597006

[Eric Seidel (no email)]

Comment on attachment 70350 [details]
[PATCH] Implement CSS_COUNTER cssText() properly

Cleared review? from 70350attachment obsolete  so that this bug does not appear in http://webkit.org/pending-review.

[Eric Seidel (no email)]

Comment on attachment 70350 [details]
[PATCH] Implement CSS_COUNTER cssText() properly

Cleared review? from obsolete attachment 70350 [details] so that this bug does not appear in http://webkit.org/pending-review.

[Eric Seidel (no email)]

Comment on attachment 70354 [details]
[PATCH] Implement CSS_COUNTER cssText() properly

Cleared review? from obsolete attachment 70354 [details] so that this bug does not appear in http://webkit.org/pending-review.

[Eric Seidel (no email)]

Comment on attachment 70694 [details]
[PATCH] Take 3

View in context: https://bugs.webkit.org/attachment.cgi?id=70694&action=review

Can't commit this with so many failing EWS bots.

> LayoutTests/fast/css/counters/counters-cssText.html:28
> +        <ul class="second"><li>one.one</li></ul>

do you mean for this to say one.one?

> WebCore/css/CSSPrimitiveValue.cpp:733
> +            if (!counterValue->separator()) {
> +                text = "counter(";
> +                text += counterValue->identifier();
> +                text += ", ";
> +                text += counterValue->listStyle();
> +                text += ")";
> +            } else {
> +                text = "counters(";
> +                text += counterValue->identifier();
> +                text += ", ";
> +                text += quoteCSSStringIfNeeded(counterValue->separator());
> +                text += ", ";
> +                text += counterValue->listStyle();
> +                text += ")";
> +            }
>              break;

It seems using multiple ifs you coudl share more code here.

> WebCore/css/Counter.h:38
> +    String listStyle() const { return m_listStyle ? CSSPrimitiveValue::create((EListStyleType) m_listStyle->getIntValue())->getStringValue() : String(); }

Normally we avoid c-style casts and use static_cast instead.

[Joseph Pecoraro]

(In reply to comment #19)
> (From update of attachment 70694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70694&action=review
> 
> Can't commit this with so many failing EWS bots.

Thanks for reviewing this. Yes, I was aware of the issue and posted comments
regarding it. The EWS output was very confusing.

> > LayoutTests/fast/css/counters/counters-cssText.html:28
> > +        <ul class="second"><li>one.one</li></ul>
> 
> do you mean for this to say one.one?

I should probably change these to something with less implied semantics. Like "foo" and
"foo.bar". The content doesn't really matter, this was just a copy & paste of the previous
<ul>, and this is the second <ul> with a different style.



> > WebCore/css/CSSPrimitiveValue.cpp:733
> > +            if (!counterValue->separator()) {
> > +                text = "counter(";
> > +                text += counterValue->identifier();
> > +                text += ", ";
> > +                text += counterValue->listStyle();
> > +                text += ")";
> > +            } else {
> > +                text = "counters(";
> > +                text += counterValue->identifier();
> > +                text += ", ";
> > +                text += quoteCSSStringIfNeeded(counterValue->separator());
> > +                text += ", ";
> > +                text += counterValue->listStyle();
> > +                text += ")";
> > +            }
> >              break;
> 
> It seems using multiple ifs you coudl share more code here.

My thoughts were, and still are, that this was much easier to read. Because of
how small these code sections are, I don't think there would be a worthwhile
advantage to sharing code at the loss of the readability. This reads very cleanly
for both the counter() and counters() branches. If anyone has a good reason
I should strive to share code here I'd be happy to change it.


> > WebCore/css/Counter.h:38
> > +    String listStyle() const { return m_listStyle ? CSSPrimitiveValue::create((EListStyleType) m_listStyle->getIntValue())->getStringValue() : String(); }
> 
> Normally we avoid c-style casts and use static_cast instead.

Whoops, you're right. Thanks.

I'll try and update this patch when I find some spare time. Otherwise, if someone
comes across this and wants to clean up the patch I'm fine with that. There doesn't
seem to be a rush, and I don't think this affects the Web Inspector anymore (which
uses a new path now).