Bug 6503 – content property doesn't support quotes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Duncan Wilcox]

According to http://www.w3.org/TR/REC-CSS2/generate.html#propdef-content the content property 
should allow:

Value:  	[ <string> | <uri> | <counter> | attr(X) | open-quote | close-quote | no-open-quote | no-
close-quote ]+ | inherit

The page doesn't show quotes around the last blockquote because webkit doesn't appear support open-
quote | close-quote | no-open-quote | no-close-quote.

A simple workaround in CSS is to use "\201c" instead of open-quote and "\201d" instead of close-quote.

[Mark Rowe (bdash)]

This looks to be related to bug 3234.  To properly support open-quote and close-quote we should 
support the quotes property as specified at http://www.w3.org/TR/CSS21/generate.html#propdef-
quotes.  From there the correct quotation marks can be used by open-quote and close-quote.

[Mark Rowe (bdash)]

A good test case exercising both open-quote/close-quote and the quotes property can be found at 
http://www.meyerweb.com/eric/css/tests/css2/sec12-04-01.htm

[Benjamin Hawkes-Lewis]

"A simple workaround in CSS is to use "\201c" instead of open-quote and "\201d"
instead of close-quote."

This isn't an effective workaround since it can't properly handle the alternation of quotation marks (e.g. between double and single in US English) when quotations are nested inside one another.

[Vincent Ricard]

Created attachment 20006 [details]
first patch (almost finished) for 'content: *quotes' and 'quotes' properties

Here is a patch working for the both url provoding in this bug report.
I still have to write the corresponding test cases, but i'd like have some comments/suggestions/review since the code is almost done.

This patch also covers #3234.

One thing is not yet fixed: choose the default open/close quote. I think this should be language dependent; but where put that? in html.css ? a new quote-lang.css?

A second thing: i've to add the copyright/license header :)

[Dave Hyatt]

Comment on attachment 20006 [details]
first patch (almost finished) for 'content: *quotes' and 'quotes' properties

Nice work adding this!

The convention of _ arguments in the QuotesValue is not something we use these days.  Also the & should be with const String& in the QuoteValue constructor.

The QuoteValue object should have m_ in front of its member variable names, e.g., m_openQuote, m_closeQuote.

Pointer placement is wrong in the RenderQuote constructor.

Document *node

should be

Document* node

Is redoing pref widths inside dirtyLineBoxes really necessary, or did you just copy this file from RenderCounter and accidentally pick that up?

You can't just do a raw pointer comparison in the equality operator of StyleRareInheritedData.

&& m_quotes == o.m_quotes;

You need to actually check if the quotes objects are equivalent.

Extraneous whitespace change:

-    unsigned wordWrap : 1; // EWordWrap 
+    unsigned wordWrap : 1; // EWordWrap

In CSSStyleSelector for the case where you clear out the quotes (CSSValueNone), can't you just set to 0 instead of to new QuotesData?

[Vincent Ricard]

Created attachment 20365 [details]
Final patch

Here is the final patch (all corrections asked by Dave are inside).

About the overriding of 'dirtyLineBoxes': if i don't do that, it does not work... and i dont know why, i'm still a newbie with the rendering engine :-)

The default quotes put in html4.css are the english ones (we could merge the whole list provided in #3234).

[Eric Seidel (no email)]

Comment on attachment 20365 [details]
Final patch

There seem to be a bunch of little issues in this patch.

We'll see how many I get this first round.  I fear we may have to got a couple rounds.

1.  There are a bunch of style guideline violations re: placement of "*"
http://webkit.org/coding/coding-style.html

No { } here:
 1921             if (value2 && value2->unit == CSSPrimitiveValue::CSS_STRING) {
 1922                 values->append(new QuotesValue(value1->string, value2->string));
 1923             } else

or here:

 3449         if (primitiveValue && primitiveValue->getIdent() == CSSValueNone) {
 3450             m_style->setQuotes(0);
 3451         } else {


I'm confused by this?
/ FIXME: we should use the version present in with CSSPrimitiveValue.cpp
 37 static String quoteString(const String& string)
 

Again, {}
 66     if (m_quote == CLOSE_QUOTE || m_quote == NO_CLOSE_QUOTE) {
 67         --level;
 68     }


This seems hackish?

 111 void RenderQuote::calcPrefWidths(int lead)
 112 {
 113     setTextInternal(originalText());
 114     RenderText::calcPrefWidths(lead);
 115 }

(the setTextInternal during a calcPrefsWidths call).  Why is that needed/correct?

0 <= m_level (for isVisible) would be more clear as m_level >= 0 IMO.


Why can't:
 839 bool StyleRareInheritedData::quotesDataEquivalent(const StyleRareInheritedData& o) const

just be
return m_quotes == o.m_quotes && *m_quotes == *o.m_quotes ?
yes, that depends on the compiler short-circuting the &&, but that's safe.


Bah.  I wish the various setContent calls could share more code.

Any time you're returning an AtomicString, return emptyAtom instead of ""; there are at least 2 times where you could do that.

Generally each initializer gets its own line:
 1068     QuotesData(const QuotesData& o)
 1069         : m_openQuotes(o.m_openQuotes), m_closeQuotes(o.m_closeQuotes) { }


I think m_quotes is leaking when set.  Can't m_quotes just be an OwnPtr?  I don't think it needs to be a DataRef, but that would be another option.

In general the patch looks good.  r- for the various issues above.

[Vincent Ricard]

> 1.  There are a bunch of style guideline violations re: placement of "*"
> http://webkit.org/coding/coding-style.html

Fixed.

> I'm confused by this?
> / FIXME: we should use the version present in with CSSPrimitiveValue.cpp
>  37 static String quoteString(const String& string)

Because CSSPrimitiveValue.h does not expose quoteString() for the subclasses (the function is only declared in CSSPrimitiveValue.cpp). So i had to copy/paste it.

> (the setTextInternal during a calcPrefsWidths call).  Why is that
> needed/correct?

I fixed this by calling setTextInternal in dirtyLineBoxes. In fact, i don't know how/when set the internal text; i need the render tree to compute the depth, so i can't do that in the constructor (since the parent is not yet known).

> 0 <= m_level (for isVisible) would be more clear as m_level >= 0 IMO.

Fixed.

> Why can't:
> just be
> return m_quotes == o.m_quotes && *m_quotes == *o.m_quotes ?
> yes, that depends on the compiler short-circuting the &&, but that's safe.

I missed the trick :-/ 'm_quotes == o.m_quotes' does not mean '*m_quotes == *o.m_quotes' ?

> Bah.  I wish the various setContent calls could share more code.

I'd prefer my patch only corrects the css property. We could open an other bugs dedicated to this refactoring.

> Any time you're returning an AtomicString, return emptyAtom instead of "";
> there are at least 2 times where you could do that.

Fixed.

> Generally each initializer gets its own line:
>  1068     QuotesData(const QuotesData& o)
>  1069         : m_openQuotes(o.m_openQuotes), m_closeQuotes(o.m_closeQuotes) 

Fixed.

> I think m_quotes is leaking when set.  Can't m_quotes just be an OwnPtr?  I
> don't think it needs to be a DataRef, but that would be another option.

Fixed with OwnPtr.

[Vincent Ricard]

Created attachment 20959 [details]
Patch with some fix

Here is the new patch

Comment on attachment 20959 [details]
Patch with some fix

Presumably Vincent meant to ask for a review :-)

[Eric Seidel (no email)]

Comment on attachment 20959 [details]
Patch with some fix

I am confused by:

1987     if (values->length()) {
 1988         addProperty(propId, values.release(), important);
 1989         if (value2)
 1990             valueList->next();
 1991         return true;
 1992     }

Couldn't that have been handled in the previous loop?

Also the previous loop:
 while (value1 = valueList->current()) {
 1974         if (value1->unit == CSSPrimitiveValue::CSS_STRING) {
 1975             value2 = valueList->next();
 1976             if (value2 && value2->unit == CSSPrimitiveValue::CSS_STRING)
 1977                 values->append(new QuotesValue(value1->string, value2->string));
 1978             else
 1979                 break;
 1980         } else
 1981             break;
 1982 
 1983         value2 = 0;
 1984         valueList->next();
 1985     }
 
Would be easier to understand using an early-return.

if (value1->unit != CSSPrimitiveValue::CSS_STRING)
  break;

It's not clear to me why you advance valueList only if values->length() (my first comment above).

* goes next to the type name in WebKit style:
 3577             CSSValueList *list = static_cast<CSSValueList*>(value);
That block has as bunch of misplaced *'s

Probably better to use a StringBuilder here:
47     String result("");
 48 
 49     result += quoteString(m_openQuote);
 50     result += " ";
 51     result += quoteString(m_closeQuote);

 44 void RenderQuote::computeLevel() const
looks horribly inefficient.  Can't it use the pre-computed levels from other quotes it encounters?  I guess the question would be how to invalidate them when the tree changes...
But if I'm reading that correctly, every RenderQuote will walk through *every* RenderObject which corresponds to an element before it in a file.  Am I reading that correctly?

* spacing:
 91     QuotesData *quotes = style()->quotes();

it seems this would be more efficient:
974     if (!m_quotes && o.m_quotes || m_quotes && !o.m_quotes)
 975         return false;
 976     if (m_quotes && o.m_quotes && (*m_quotes != *o.m_quotes))
 977         return false;
 978     return true;
written as:
if (m_quotes == o.m_quotes)
   return true;
return (m_quotes && o.m_quotes && (*m_quotes == *o.m_quotes));


This:
if (m_closeQuotes.size() <= level)
 1911         level = m_closeQuotes.size() - 1;
would read more clear to me as:
if (level >= m_closeQuotes.size())
   return m_closeQuotes.last();

spacing:
 1207     void addLevel(const AtomicString &open, const AtomicString &close);


You patch also needs a test case which covers syntax errors in "quotes" property.

r- for the lack of syntax error test case.

[Vincent Ricard]

Created attachment 23125 [details]
Patch reusing the previous computed level

Here is a patch fixing all Eric's requirements.
About the level computation and the tree invalidation, maybe could we do as for the counters: have an "invalidQuote" method, and call it from RenderObject::destroy()?

[Dave Hyatt]

Comment on attachment 23125 [details]
Patch reusing the previous computed level

r=me

[Dave Hyatt]

Comment on attachment 23125 [details]
Patch reusing the previous computed level

This does need to add RenderQuote.cpp/h to other projects though (Mac/Windows) etc. or the build will be broken.

Whoever lands it will need to do that.

[Darin Adler]

Comment on attachment 23125 [details]
Patch reusing the previous computed level

This also has a bug in the inherit case that will result in multiple styles sharing the same QuoteData and multiply deleting it.

review-

I'm going to fix this and land it.

[Darin Adler]

This patch also didn't include expected results for the tests. Also, the patch changes the results of some existing tests; seemingly breaking fast/css-generated-content/005.html because the quotes aren't defaulting to straight quotes.

[Darin Adler]

Created attachment 24311 [details]
newer patch -- not ready to check in

This patch has some of the problems with the previous patch fixed. But it breaks our existing regression test fast/css-generated-content/005.html, so it's not ready to check in yet.

I'm also uncertain if the results of the tests are good. I'd really prefer to find a way to write dumpAsText tests, but that may not be possible for generated content.

[Daniel Upstone]

This bug has still not been resolved and is a serious browser inconsistency issue for CSS2 implementation.
It may be worth noting that IE8 is the only browser to do this correctly (yes, you read that correctly). Firefox does render correctly, but c&p of the output shows that it's still using forced quotes on the q element rather than providing the rendered result or a no-generated-content result. Opera seems to have it's own issues (eg. a stray " sneaking in) but is otherwise fine. Webkit still lacks full support and renders incorrectly.

Test Case: http://www.zealous-studios.co.uk/content-quotes.xhtml
The *-quote values for the content property are still not supported. This breaks any attempt to correctly fix https://bugs.webkit.org/show_bug.cgi?id=3234

This bug needs to be fixed first before any progress on bug 3234 can be made.

[Carol Szabo]

Created attachment 81638 [details]
Proposed Patch. Similar to Darin's, but taking advantage of the recent changes to RenderObject

[WebKit Review Bot]

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

[Dave Hyatt]

Comment on attachment 81638 [details]
Proposed Patch. Similar to Darin's, but taking advantage of the recent changes to RenderObject

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

Glad you are tackling this!  Some initial comments:

> Source/WebCore/css/CSSStyleSelector.cpp:4599
> +        HANDLE_INHERIT(quotes, Quotes)

Where is the handling of "initial"?

> Source/WebCore/css/html.css:28
> +    quotes: '"' '"' "'" "'";

This really shouldn't be necessary.  The CSS 2.1 specification says the initial value "depends on the user agent," which implies to me that the initial value is the quotes you want to use and not none.  Also, quotes should obviously work for XML documents, so let's just fix the initial value.

The initial value should probably just be a special keyword like -webkit-quotes.  Then your front end code can eventually use the current document language to pick the correct set of quotes (instead of hard-coding the quotes listed above).

> Source/WebCore/rendering/RenderQuote.cpp:80
> +    for (RenderObject* predecesor = head->previousInPreOrder(); predecesor; predecesor = predecesor->previousInPreOrder())

Should be predecessor.

> Source/WebCore/rendering/RenderQuote.cpp:178
> +        if (descendant->isQuote()) {

I think this for loop would read nicer if you just did

if (!descendant->isQuote())
    continue;

and then you don't have to indent all the code that comes after.

[Carol Szabo]

(In reply to comment #21)
> > Source/WebCore/css/CSSStyleSelector.cpp:4599
> > +        HANDLE_INHERIT(quotes, Quotes)
> 
> Where is the handling of "initial"?
I do not see that initial is allowed for quotes. But maybe we should support it anyway.
> 
> > Source/WebCore/css/html.css:28
> > +    quotes: '"' '"' "'" "'";
> 
> This really shouldn't be necessary.  The CSS 2.1 specification says the initial value "depends on the user agent," which implies to me that the initial value is the quotes you want to use and not none.  Also, quotes should obviously work for XML documents, so let's just fix the initial value.
> 
> The initial value should probably just be a special keyword like -webkit-quotes.  Then your front end code can eventually use the current document language to pick the correct set of quotes (instead of hard-coding the quotes listed above).
I understand the part with using the quotes as per document language, but I am not sure how to implement that. An example of another feature using this would be helpful.
What I understand so far is that I could use a keyword (such as initial) instead of an actual list of quotes in html.css and in CSSStyleSelector::applyProperty, transform that keyword into a real list when creating the RenderStyle object for an element.
What I do not know is how to specify this keyword to the root element of any type of document so that it is inherited by all elements unless one element in the tree defines it otherwise, and then its children will inherit that redefined value.

[Dave Hyatt]

Comment on attachment 81638 [details]
Proposed Patch. Similar to Darin's, but taking advantage of the recent changes to RenderObject

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

> Source/WebCore/rendering/style/StyleRareInheritedData.h:91
> +    Vector<String> quotes;

Basically I'd consider turning the Vector into a pointer and only allocate it when there are concrete quotes specified explicitly in CSS.  Then you could add a member like bool m_useUAQuotes; and have that be set to true for the initial value (and false when quotes is explicitly set, either to none or to a concrete value).

Vector<String>* m_authorQuotes;
bool m_useUAQuotes;

Then you just need a good place to resolve to a real value if m_useUAQuotes is set instead of having a Vector.  It seems like you could do that in placeQuote and have a global hashmap in RenderQuote that just stores the different languages.  For the first landing you can just make a static Vector sitting right in placeQuote for English and use that if the user agent value is set.

You still need to patch CSSComputedStyleDeclaration as well, although doing that in a follow-up patch would be fine.

[Carol Szabo]

Created attachment 82320 [details]
Proposed Patch. Added support for language specific defaults.

[WebKit Review Bot]

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

[Build Bot]

Attachment 82320 [details] did not build on win:
Build output: http://queues.webkit.org/results/7913581

[Carol Szabo]

Created attachment 82329 [details]
Proposed Patch. Fixed problem with Chromium build system.

[WebKit Review Bot]

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

[Carol Szabo]

Created attachment 82340 [details]
Proposed Patch. Fixed problem with Chromium build system.

[Build Bot]

Attachment 82329 [details] did not build on win:
Build output: http://queues.webkit.org/results/7908669

[WebKit Review Bot]

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

[Build Bot]

Attachment 82340 [details] did not build on win:
Build output: http://queues.webkit.org/results/7910694

[Carol Szabo]

Created attachment 82693 [details]
Proposed Patch. Rebased and fixed typing warning for some indexes.

[Eli Grey (:sephr)]

(In reply to comment #33)
I'm opposed to the simpleQuotes default, and feel that the UA's preferred locale should be the default, or englishQuotes if this is not possible (which possibly should be changed to latinQuotes). {"'} isn't that appropriate in a case where the start and end of the quote are both known.

[WebKit Review Bot]

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

[Carol Szabo]

Created attachment 82706 [details]
Proposed Patch. Fixed typo in WebCore.gypi

[Build Bot]

Attachment 82706 [details] did not build on win:
Build output: http://queues.webkit.org/results/7917606

[Build Bot]

Attachment 82706 [details] did not build on win:
Build output: http://queues.webkit.org/results/7929298

[Carol Szabo]

Created attachment 82985 [details]
Proposed Patch. Fixed problem with Windows build.

[Dave Hyatt]

Comment on attachment 82985 [details]
Proposed Patch. Fixed problem with Windows build.

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

> Source/WebCore/css/CSSStyleSelector.cpp:4729
> +    case CSSPropertyQuotes:
> +        if (isInherit) {
> +            if (m_parentStyle)
> +                m_style->setQuotes(m_parentStyle->quotes());
> +            return;
> +        }

Still don't see the isInitial case here for handling the initial value.  You should probably make a test case that tests -webkit-initial explicitly, since this isn't being covered.  Put an object with quotes:-webkit-initial inside an object with quotes:none to see the problem.  If I'm understanding how you represent the initial value correctly (with a null pointer), then all your isInitial case needs to do is m_style->setQuotes(0).

> Source/WebCore/rendering/RenderQuote.cpp:245
> +    ASSERT(m_depth != UNKNOWN_DEPTH);
> +     const QuotesData* quotes = style()->quotes();
> +     if (!quotes)
> +         quotes = defaultQuotes(this);

Some funny indenting here.

[Carol Szabo]

Created attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

[Eli Grey (:sephr)]

What is your response to my concerns?

[Carol Szabo]

(In reply to comment #34)
> (In reply to comment #33)
> I'm opposed to the simpleQuotes default, and feel that the UA's preferred locale should be the default, or englishQuotes if this is not possible (which possibly should be changed to latinQuotes). {"'} isn't that appropriate in a case where the start and end of the quote are both known.

I believe the default is a minor issue, a matter of preference. The quotes you do not like are used only when the language of the page cannot be determined. I feel that simple ASCII characters, which are available on most systems, should be an option.
The way the code is designed, any implementor of a WebKit based browser can change this default easily. My goal is to provide basic support for quotes and compliance with the standard. The defaults are not specified in the standard, so the only reason why I included a relatively elaborate logic for defaults is so that my patch gets approved by reviewers. I would have considered such a logic out of scope for now.
If you want a different default wait for my patch to be landed and make your proposal. See if reviewers approve it.

As far as the englishQuotes vs. latinQuotes part of your comment I have no idea what you mean. I come from a culture based on a Latin language, very close to Italian and we use different quotes; so even with my limited knowledge of correct orthography in different cultures, I do not find latinQuotes an appropriate name for the quotes used customarily in English texts. Please explain.

[Dave Hyatt]

Comment on attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

r=me

Looks great!

[Dave Hyatt]

Comment on attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

Two gotchas:

(1) You need to patch RenderStyle::diff for when quotes change dynamically to supply the appropriate hint (layout hint I think).
(2) It seems like:

&& *quotes.get() == *o.quotes.get()

will crash if you are comparing against objects with quotes:none, since you'll deref 0.

[Dave Hyatt]

Comment on attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

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

> Source/WebCore/rendering/style/QuotesData.cpp:42
> +    if (!&other || !this)
> +        return false;

I see you did handle the null check.  This would probably seem a little less strange if you made the operator== a friend function instead, but I don't feel that strongly about it.

[Darin Adler]

Also, no need to use get() if you are using *. The * operator works on the smart pointers directly.

[Dave Hyatt]

Comment on attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

My concerns have been met.  Looks good.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80037 might have broken SnowLeopard Intel Release (Build)

[Carol Szabo]

Comment on attachment 83029 [details]
Proposed Patch. Addressed DHyatt's concerns.

Manually committed due to failure to apply changes to the vcproj in some ews. r80037.

[Csaba Osztrogonác]

Reopen, because it was rolled out by http://trac.webkit.org/changeset/80041

[Csaba Osztrogonác]

The patch was rolled out because of SL build break, but I'd like to ask
some minor changes for the fixed patch:

Please add "#include <wtf/text/CString.h>" line to Source/WebCore/rendering/RenderQuote.cpp to make Qt-V8 build happy.

And please update the 3 layout test result: http://build.webkit.org/results/Qt%20Linux%20Release/r80040%20%2829126%29/results.html

[Carol Szabo]

Changeset 80151. Recommitted my patch after fixing the XCode project and including expected result changes and style changes suggested by Ossy.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80151 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
fast/css-generated-content/005.html
fast/css-generated-content/beforeAfter-interdocument.html
fast/css-generated-content/no-openclose-quote.html
fast/invalid/residual-style.html