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.
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.
"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.
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 :)
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?
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).
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.
> 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.
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.
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()?
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.
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.
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.
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.
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.
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.
(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.
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.
(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.
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.
(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.
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.
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.
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.
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
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
2008-03-24 07:13 PDT, Vincent Ricard
2008-04-06 07:48 PDT, Vincent Ricard
2008-05-04 11:19 PDT, Vincent Ricard
2008-09-02 13:26 PDT, Vincent Ricard
2008-10-12 15:10 PDT, Darin Adler
2011-02-08 08:42 PST, Carol Szabo
hyatt: commit-queue-
2011-02-14 08:54 PST, Carol Szabo
2011-02-14 10:05 PST, Carol Szabo
2011-02-14 10:54 PST, Carol Szabo
2011-02-16 14:16 PST, Carol Szabo
2011-02-16 15:05 PST, Carol Szabo
2011-02-18 11:14 PST, Carol Szabo
hyatt: commit-queue-
2011-02-18 15:41 PST, Carol Szabo