Closed
Bug 35768
Opened 25 years ago
Closed 22 years ago
[review]CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes [SELECT]
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla1.2alpha
People
(Reporter: ess, Assigned: dbaron)
References
Details
(Keywords: css2, Whiteboard: [patch](py8ieh: need a thorough test case) (py8ieh:need test case which dynamically changes the xml:lang of a grandparent element))
Attachments
(11 files, 22 obsolete files)
155 bytes,
text/html
|
Details | |
288 bytes,
text/html
|
Details | |
3.14 KB,
text/xml
|
Details | |
425 bytes,
text/html
|
Details | |
2.75 KB,
text/html
|
Details | |
2.96 KB,
text/xml
|
Details | |
401 bytes,
text/xml
|
Details | |
32.67 KB,
patch
|
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
2.54 KB,
patch
|
Details | Diff | Splinter Review | |
857 bytes,
patch
|
sicking
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
591 bytes,
text/xml
|
Details |
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m14)
BuildID: 2000041210
HTML elements with their LANG attributes set are not styled according to
appropriate :lang() stylesheet rules. This is probably the fault of bug 26237
(if I read correctly), but may be a fault in handling of CSS2.
Reproducible: Always
Steps to Reproduce:
1. Create style which depends upon :lang pseudo-class
2. Create HTML element with matching LANG attribute
3. View
Actual Results: The style is not applied.
Expected Results: The style should be applied to matching elements.
As in:
<html>
<head>
<style>
span:lang(fr) { text-transform: uppercase; }
</style>
<body>
<p>Foo, <span lang="fr">foo,</span> foo.</p>
</body>
</html>
Comment 1•25 years ago
|
||
Style System or Parser maybe. Trying Style System first.
Assignee: asadotzler → pierre
Component: Browser-General → Style System
QA Contact: jelwell → chrisd
Comment 3•25 years ago
|
||
I STRONGLY suggest that this be marked "LATER" (i.e., fix in next version).
We have not committed to implementing CSS2, and this is not the easiest of
features to get right (it involves inheriting attributes down the DOM, and other
interesting excursions). We have plenty of CSS1 bugs to keep us busy for a
while yet. Furthermore, since we already have the dash match ("|=") attribute
selector implemented, eager stylesheet authors can already, to some extent, get
this functionality.
BTW, we will need a thorough test case for this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: css2
OS: Windows NT → All
Priority: P3 → P4
Hardware: PC → All
Summary: CSS2 :lang pseudo-class won't match on LANG attributes → CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes
Whiteboard: LATER? (py8ieh: need a thorough test case)
Comment 4•25 years ago
|
||
Wholeheartedly approved.
Marked LATER.
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → LATER
Updated•25 years ago
|
Status: RESOLVED → VERIFIED
Whiteboard: LATER? (py8ieh: need a thorough test case) → (py8ieh: need a thorough test case)
Comment 5•25 years ago
|
||
Ok. We'll reopen this for 5.1...
Comment 6•24 years ago
|
||
Reopening and moving to Future. There probably exists duplicates of this bug.
Status: VERIFIED → REOPENED
Keywords: correctness
QA Contact: chrisd → py8ieh=bugzilla
Resolution: LATER → ---
Target Milestone: --- → Future
Reporter | ||
Comment 7•24 years ago
|
||
Reporter | ||
Comment 8•24 years ago
|
||
Updated•24 years ago
|
Summary: CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes → CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes [SELECT]
Updated•24 years ago
|
Blocks: Default-Quotes
Comment 10•24 years ago
|
||
Nominating this bug for nsbeta1 on behalf of gerardok@netscape.com.
Keywords: nsbeta1
Comment 11•23 years ago
|
||
I'll append a first simple patch toward an implementation of the lang pseudo
class. This allows the parser to handle it correctly. I hope this patch is
uncontroversial. It changes the behavior of Mozilla a bit as far as to allow
the rules with the :lang() pseudo-class to be used. It's just that the
restrictions coming from the lang attribute are not checked. I don't think
this situation is worse than what exists today.
Anyhow, I'd like to implement this completely and already looked at the code.
But there are some major decisions to be made first (see also
http://bugzilla.mozilla.org/show_bug.cgi?id=24861
especially the comment by Daniel Glazman; I think the problem is similar). The
problem here is that at all time it is necessary to know about the lang
attribute for each tag. The initial language might even be defined by the HTML
reply and/or the language selected by the user. Now it would be possible to
always walk the tree from the current node to the root and check for the
attributes but I guess this would be considered a problem by some people. But
what is the alternative? Forward propagation of the lang attribute is costly as
well. And what do you do with a DOM call changing/adding/removing the attribute?
IMHO the tree-walking approach cannot be avoided without some major
reorganizations of the code. To speed things up is is probably a good thing to
handle the lang attribute special in the nsIContent class, always add it as an
nsAtom* if set. This will reduce the lookup to pointer comparisons.
I don't want to go ahead and do any of the work without some guidance. What do
the owners of this code think is the right solution? I know that you declared
this problem as "future" but to increase parallelism in the development this
kind of guidance is necessary. Your priorities don't have to be shared by
everybody and everybody should be fine to implement the missing pieces.
I'd be happy to implement the simple, tree-walking approach now (which will have
impacts almost only on those pages using the lang pseudo-class) and modify it
later for a better solution.
Comment 12•23 years ago
|
||
Comment 13•23 years ago
|
||
Comment 14•23 years ago
|
||
Please ignore the patch is attached on 07/05/01. It was missing and error
check. The new patch has it and I'm using a build with the patch for 1 1/2 days
now.
Comment 15•23 years ago
|
||
I believe we already "know" the language of each element, although I'm not sure
how we do it. For example, right clicking on an element and choosing Properties
will give you the language of the element, so the data is there somewhere.
Hyatt? dbaron?
Comment 16•23 years ago
|
||
Ian Hickson wrote:
> I believe we already "know" the language of each element
I could see how. I looked at the information available in the CSS functions and
there is no lang info anywhere.
So I went on and finished my patch. I'll shortly attach it and in supercedes
the patches I've provided so far. It should be a complete implementation with
one defficiency which is as far as I can say not in the code I touched here.
You can see the defficiency in the test input file I'll also attach (it's about
changing the lang attribute of an element with the DOM functions). Creating new
elements with DOM works as can be seen in the test case, too. Oh, the test case
included in this bugzilla entry also works.
As for the patch, it is I think the best you can do given the current set of
information available in the nsIContent data. The parser stores the :lang()
parameter as a string and the selector matching function tries to find an
element with a lang attribute set and then performs a string comparison. Quite
simple but potentially slow. There is, however, no negative effect on the
performance for input files which Mozilla managed to handle so far. Only input
with :lang() usage in the style sheet will use the new code. I don't know the
standards of the Mozilla team but I would say this is good enough to include the
code.
For the future, I have some ideas but would really like to have some input
first. I think that the lang attribute is important enough to be treated
specially. Therefore a member GetLang() should be added to nsIContent.
Alternative a data member mLang could be used. The language should be
represented by an nsIAtom type object. The value will be automatically set if
an attribute named "lang" is set using the SetAttribute members. When building
the document structure the language of the document itself should be determined
by the content-language specification in the HTTP reply. The patch I've
provided would have to be changed only slightly: the nsAtomStringList type would
be changed to nsAtomPairList (with mString becoming a nsIAtom* object), the CSS
parser stores the language as an atom, and the selector matcher simply compares
the nsIAtom from the CSS selector with the return value of data.GetLang(). No
loops needed.
While this solution is fast at display time it'll require more work when
creating the document representation and also in the SetAttribute() functions.
This should be acceptable especially since to solve the problem exposed in my
test case (using the DOM function setAttribute to change the lang value) also
requires the special treatment of such a call to initiated update of the display.
Any comment welcome.
Comment 17•23 years ago
|
||
Comment 18•23 years ago
|
||
Comment 19•23 years ago
|
||
Assignee | ||
Comment 20•23 years ago
|
||
Result of discussion in #mozilla about attachment 42159 [details]: the style hint for the
'quotes' property may need to be changed from REFLOW to FRAMECHANGE:
http://lxr.mozilla.org/seamonkey/source/content/shared/public/nsCSSPropList.h#188
Comment 21•23 years ago
|
||
I've compiled Mozilla with the proposed
CSS_PROP(quotes, quotes, FRAMECHANGE)
and have seen no difference. This doesn't fix the problem.
Assignee | ||
Comment 22•23 years ago
|
||
Hmm. The FRAMECHANGE change may or may not be necessary. However, what *is*
necessary (should have thought of this earlier...) is changing
CSSStyleSheetImpl::CheckRuleForAttributes at
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsCSSStyleSheet.cpp#2037
to check for a lang selector and, if it exists, add the lang attribute (and
xml:lang attribute, although this seems not to be namespace-aware -- it's just a
performance hack) to the sheet's mRelevantAttributes. (Are there any other
potentially relevant attributes?)
Comment 23•23 years ago
|
||
> However, what *is* necessary (should have thought of this
> earlier...) is changing CSSStyleSheetImpl::CheckRuleForAttributes at
>[...]
I've looked at this (actually implemented it) and think this still
cannot solve the problem.
This code would help if the lang attribute of the element which has a
CSS rule is changed. But the lang attribute is transitive to all the
children (unless it's reset). So, in a situation like
<p lang="de">some <q>quoted</q> text.</p>
where there is a CSS rule with :lang() for <q> no the change would be
recognized.
The lang attribute is really special and might indeed need special
attention.
What makes things even stranger is that everything seems to work fine if I'm
using colors instead of quotes. I've an updated test at
http://www.cygnus.com/~drepper/quotes.html
With the patch applied the DOM code changing the lang attribute of the DIV
element with the ID "foo" has the effect that the color changes.
Assignee | ||
Comment 24•23 years ago
|
||
> This code would help if the lang attribute of the element which has a
> CSS rule is changed. But the lang attribute is transitive to all the
> children (unless it's reset).
That shouldn't be a problem, since even with existing combinators we have
to re-resolve style for all descendants when an attribute that affects style
changes.
Comment 25•23 years ago
|
||
> That shouldn't be a problem, since even with existing combinators we have
> to re-resolve style for all descendants when an attribute that affects style
> changes.
OK, then maybe you want to inspect what code I added:
if (iter->mPseudoClassList) {
// Search for the :lang() pseudo class. If it is there add
// the lang attribute to the relevant attribute list.
nsAtomStringList *runp = iter->mPseudoClassList;
do {
if (runp->mAtom == nsCSSAtoms::langPseudo) {
// Found it.
DependentAtomKey langKey(nsHTMLAtoms::lang);
mInner->mRelevantAttributes.Put(&langKey, nsHTMLAtoms::lang);
break;
}
} while ((runp = runp->mNext) != nsnull);
}
This is additional code in the for(iter=) loop in the function you mentioned.
I've verified that the Put() function is actually called. Anything wrong with this?
Assignee | ||
Comment 26•23 years ago
|
||
Hmmm. Maybe you're also running into bug 57226. Can you tell if the style is
now actually being re-resolved, using the debugging tools in viewer? Or you
could try another selector with "[lang]" as selector to work around the problem
for now in case that code is wrong...
Comment 27•23 years ago
|
||
> Hmmm. Maybe you're also running into bug 57226. Can you tell if the style is
> now actually being re-resolved, using the debugging tools in viewer?
You mean Debug|DOM Viewer? When I select this menu entry I get
JavaScript error:
line 0: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMLocation.href]" nsresult: "0x80004005
(NS_ERROR_FAILURE)" location: "JS frame :: <unknown filename> :: oncommand ::
line 0" data: no]
Not when I press a button but when I select the menu entry.
> Or you could try another selector with "[lang]" as selector to work around
> the problem for now in case that code is wrong...
If I replace :lang(de) with [lang="de"] nothing at all works. And I'd expect
this since the lang attribute isn't set for the elements mentioned in the CSS.
Assignee | ||
Comment 28•23 years ago
|
||
> You mean Debug|DOM Viewer?
no, mozilla-viewer.sh or viewer.exe
> If I replace :lang(de) with [lang="de"] nothing at all works.
Not replace, but add an additional rule so that the stylesheet is noted as
having something that responds to the lang attribute.
Comment 29•23 years ago
|
||
> no, mozilla-viewer.sh or viewer.exe
OK. I normally don't build with tests enabled. Have to recompile first.
> Not replace, but add an additional rule so that the stylesheet is noted as
> having something that responds to the lang attribute.
Done that, no difference.
Comment 30•23 years ago
|
||
> Hmmm. Maybe you're also running into bug 57226. Can you tell if the style is
> now actually being re-resolved, using the debugging tools in viewer?
I'm not at all experienced with the viewer debug tools but I think I tried every
combination without getting any result. If you can suggest a specific procedure
I'll do that.
Comment 31•23 years ago
|
||
Comment 32•23 years ago
|
||
Please ignore the first added patch. I really shouldn't edit the patch file
directly. But I have so many accumulated changes in those files that it's hard
to regenerate the patch from scratch.
Comment 33•23 years ago
|
||
Comment 34•23 years ago
|
||
Looks good (but that's not a formal review!). Do I take it you do not (yet)
support the xml:lang attribute?
I notice that as far as I can tell you explicitly look for the 'lang' attribute
in the HTML namespace (right?). That seems wrong to me. You should look for the
'lang' attribute in NO namespace IF the element is an HTML element. The HTML
spec doesn't say anything about a global 'lang' attribute in the HTML namespace.
If I misread that code then my apologies.
Comment 35•23 years ago
|
||
> Looks good (but that's not a formal review!). Do I take it you do not (yet)
> support the xml:lang attribute?
You're right, xml_lang isn't supported. But it's not because of lack of trying.
I couldn't figure out how. If you can point me in the right direction I'm
happy to update my patch.
Comment 36•23 years ago
|
||
The xml:lang attribute SHOULD be the 'lang' attribute in the XML namespace. I'm
not sure exactly how we do that.
nisheeth/jst/heikki: could you help us here? How does one find the xml:lang
attribute?
Comment 37•23 years ago
|
||
> The xml:lang attribute SHOULD be the 'lang' attribute in the XML namespace. I'm
> not sure exactly how we do that.
Unless I'm mistaken the GetAttr function in the HTML code will not allow using
the kNameSpaceID_XML value (I think this is
content/src/nsGenericHTMLElement.cpp:1876).
And when looking through the data structures genereated for theXML file I
couldn't find the xml:lang attribute altogether.
I think I haven't yet attached a XML test case. Will do so after verifying it.
Comment 38•23 years ago
|
||
Have you checked if you can access xml:lang attribute in an XML document that
has no declared namespaces?
I know there is a bug regarding HTML/XHTML content: they do not support
additional namespaces. Therefore, if you try to add xml:lang to an XHTML element
it won't work. See bug 41983.
Comment 40•23 years ago
|
||
> Have you checked if you can access xml:lang attribute in an XML document that
> has no declared namespaces?
What I actually did is starting moz in gdb and looking at the code accessing the
attributes to see how the xml:lang attributes are represented. The result: they
are not there at all. In the XHTML test case I attached the only attributes
visible are the xmlns attribute at the toplevel and the lang attributes in the
preference test. No sign whatsoever of the xml:lang attributes.
I might have look at the wrong place but it sure looks to me as if xml:lang
isn't recognized as a valid attribute name and therefore not added to the
internal representation.
Comment 41•23 years ago
|
||
There is a bug which makes them not appear for elements in the XHTML namespace.
Look again, but on elements that have no namespace, e.g. in the following document:
<foo xml:lang="en"/>
Comment 42•23 years ago
|
||
I've just created bug 98929 for a patch to implement Content-Language handling
on document level. This implementation than can be used to (almost) finish the
:lang() pseudo-class implementation (only xml:lang is missing). The patch is
tested, see bug 98929 for an explanation how to configure Apache.
Comment 43•23 years ago
|
||
Comment 44•23 years ago
|
||
Comment 45•23 years ago
|
||
Now that nsDocument provides language information (bug 98929) I've updated the
patch once more (attachment 49051 [details] [diff] [review]). This patch avoids converting from Unicode
and does all conversion with Unicode chars.
There is one more change: the language string returned from nsDocument can be a
list. I think it only makes sense to use the first listed language and ignore
the others when looking for a match.
Comment 46•23 years ago
|
||
Comment 47•23 years ago
|
||
This is not a full review - I thought the first 2 points below should be
discussed before going further.
1) I haven't looked into it in detail but wouldn't have been simpler to add the
language directly into the nsCSSSelector instead of putting it inside the
mPseudoClassList?
2) I think you should not a match only on the first item in the 'Content-
Language' list. You wrote in the patch that doing otherwise "might lead to not
reproducible results". If you consider the example at
http://www.w3.org/TR/REC-CSS2/selector.html#x42, the first pair of rules defines
a precedence of HTML:lang(de) over HTML:lang(fr) in the case of a document that
defines "fr,de" simply because the "de" rule comes after the "fr" rule. With the
proposed patch, only the first rule applies, which is wrong - and it is even more
wrong for documents that define "en,fr,de" since none of the 2 rules apply.
3) In nsAtomStringList::Equals() and SelectorMatches(), you should use
EqualsIgnoreCase() to check the two strings.
4) I saw some NS_RELEASE where nsCOMPtr would be more appropriate.
Comment 48•23 years ago
|
||
Regarding point one, I'm not entirely sure I understand it, but :lang() is indeed
just a pseudo-class, and rules like
:lang(en):lang(de):lang(fr)
...and perfectly valid (if pointless) as are rules like:
:not(:lang(x-klingon)):not(:lang(x-vulcan)):lang(x)
Updated•23 years ago
|
Severity: enhancement → normal
Status: REOPENED → ASSIGNED
Priority: P4 → P3
Target Milestone: Future → mozilla0.9.6
Comment 49•23 years ago
|
||
I'll attach a patch that fixes #2, 3 and 4 above (#1 isn't such a good idea in
case we need to implement more pseudo-classes that take a string as a parameter).
I tested with a couple of testsuites and a simple testcase. Before loading the
testcase, go to Prefs -> Navigator -> Languages and add "French" or "German" in
addition to "English/US".
Daniel or DBaron: please review
Marc: please s/r
Comment 50•23 years ago
|
||
Comment 51•23 years ago
|
||
Comment on attachment 54201 [details] [diff] [review]
patch 2.0
r=glazman
I have only one concern : memory footprint... the pseudo-class list turned into a nsAtomStringList seems quite an expensive choice for a selector we don't use for the moment in our app stylesheets.
Attachment #54201 -
Flags: review+
Comment 53•23 years ago
|
||
We should be fine regarding memory footprint:
- The string is allocated only when needed, which means never except for :lang.
- nsAtomStringList takes 3 pointers instead of 2 for nsAtomList, which is still
ok even if we have hundreds of pseudo-classes.
Note: I modified the patch a little bit in my local tree to optimize the use of
language.Length().
Assignee | ||
Comment 54•23 years ago
|
||
Comment on attachment 54201 [details] [diff] [review]
patch 2.0
>Index: mozilla/content/html/style/src/nsCSSStyleSheet.cpp
>===================================================================
>RCS file: /m/pub/mozilla/content/html/style/src/nsCSSStyleSheet.cpp,v
>retrieving revision 3.182
>diff -u -2 -r3.182 nsCSSStyleSheet.cpp
>--- nsCSSStyleSheet.cpp 2001/09/29 08:25:58 3.182
>+++ nsCSSStyleSheet.cpp 2001/10/19 10:42:08
>@@ -2293,5 +2293,18 @@
> mInner->mRelevantAttributes.Put(&key, sel->mAttr);
> }
>+ if (iter->mPseudoClassList) {
>+ // Search for the :lang() pseudo class. If it is there add
>+ // the lang attribute to the relevant attribute list.
>+ nsAtomStringList *runp = iter->mPseudoClassList;
>+ do {
>+ if (runp->mAtom == nsCSSAtoms::langPseudo) {
>+ // Found it.
>+ DependentAtomKey langKey(nsHTMLAtoms::lang);
>+ mInner->mRelevantAttributes.Put(&langKey, nsHTMLAtoms::lang);
>+ break;
>+ }
>+ } while ((runp = runp->mNext) != nsnull);
> }
>+ }
> } /* fall-through */
> default:
Why not a while-do without the extra if? (Well, OK, this may be better on
some processors, so leave it if you want.) And also, the indentation seems
odd (as in a few other places). Are there tabs here? This isn't a diff -w.
>@@ -3331,5 +3344,5 @@
> if(PR_FALSE == mIsHTMLLink &&
> mHasAttributes &&
>- !(aContent->IsContentOfType(nsIContent::eHTML) || aContent->IsContentOfType(nsIContent::eXUL)) &&
>+ !(PR_TRUE == mIsHTMLContent || aContent->IsContentOfType(nsIContent::eXUL)) &&
> nsStyleUtil::IsSimpleXlink(aContent, mPresContext, &mLinkState)) {
> mIsSimpleXLink = PR_TRUE;
I really don't like the |PR_TRUE ==| -- it's especially dangerous for truth
tests since occasionally booleans get set to a true value other than 1.
>@@ -3450,4 +3463,30 @@
>
>
>+static PRBool DashMatchCompare(const nsString& comparedValue, const nsString& baseValue, const PRBool aCaseSensitive) {
Parameters should be |const nsAString&| rather than |const nsString&| unless
you have good reason to use things on |nsString| that aren't on |nsAString|,
which you don't here. Also, below I'm going to point out a place where you
need to be passing in an nsAString that isn't an nsString.
>+ PRBool result;
>+ PRUint32 baseLen = baseValue.Length();
>+ PRUint32 compLen = comparedValue.Length();
>+ if (baseLen > compLen) {
>+ result = PR_FALSE;
>+ }
>+ else {
>+ if (baseLen != compLen &&
>+ comparedValue.CharAt(baseLen) != PRUnichar('-')) {
>+ // to match, the comparedValue must have a dash after the end of
>+ // the baseValue's text (unless the baseValue and the comparedValue
>+ // have the same text)
>+ result = PR_FALSE;
>+ }
>+ else {
>+ if (aCaseSensitive)
>+ result = PRBool(NS_OK == Compare(Substring(comparedValue, 0, baseLen), baseValue, nsDefaultStringComparator()));
>+ else
>+ result = PRBool(NS_OK == Compare(Substring(comparedValue, 0, baseLen), baseValue, nsCaseInsensitiveStringComparator()));
No need to cast to bool, and testing against NS_OK is wrong, it should
be a test against 0 (which happens to be NS_OK, but Compare is like
strcmp, it's not returning an nsresult).
>+ }
>+ }
>+ return result;
>+}
>+
>+
> static PRBool SelectorMatches(SelectorMatchesData &data,
> nsCSSSelector* aSelector,
>@@ -3576,6 +3615,66 @@
> }
> else if (nsCSSAtoms::langPseudo == pseudoClass->mAtom) {
>- // XXX not yet implemented
>- result = PR_FALSE;
>+ NS_ASSERTION(nsnull != pseudoClass->mString, "null lang parameter");
>+ result = localFalse;
>+ if (pseudoClass->mString) {
>+ // We have to determine the language of the current element. Since
>+ // this is currently no property and since the language is inherited
>+ // from the parent we have to be prepared to look at all parent
>+ // nodes. The language itself is encoded in the LANG attribute.
>+ PRBool decided = PR_FALSE;
>+
>+ nsCOMPtr<nsIContent> element = data.mContent;
>+ while (element) {
>+ PRInt32 attrCount = 0;
>+ element->GetAttrCount(attrCount);
>+ if (attrCount > 0) {
>+ nsAutoString value;
>+ nsresult attrState = element->GetAttr(kNameSpaceID_HTML,
>+ nsHTMLAtoms::lang, value);
>+ if (attrState == NS_CONTENT_ATTR_HAS_VALUE) {
>+ // Compare the attribute string with the language string from
>+ // the style sheet.
>+ result = PRBool(localTrue == DashMatchCompare(value, *pseudoClass->mString, PR_FALSE));
>+ decided = PR_TRUE;
>+ break;
>+ }
>+ }
>+ nsIContent *parent;
>+ element->GetParent(parent);
>+ element = dont_AddRef(parent);
>+ };
This is an extremely expensive loop for selector matching. Could you lazily
cache the resulting language on the SelectorMatchesData (i.e., access it through
a getter and store a state variable whether you've gotten it or not)? (We should
start lazily caching other things on the SelectorMatchesData as well, so now is as
good a time as any to start.)
Also, there's a stray semicolon right at the end there.
>+
>+ if (!decided) {
>+ nsIDocument *doc;
>+ if (NS_SUCCEEDED(data.mContent->GetDocument(doc))) {
>+ // Try to get the language from the HTTP header or if this
>+ // is missing as well from the preferences.
>+ // The content language can be a comma-separated list of
>+ // language codes.
>+ nsAutoString language;
>+ if (NS_SUCCEEDED(doc->GetContentLanguage(language)) && (language.Length() > 0)) {
>+ if (nsnull != pseudoClass->mString) {
>+ language.StripWhitespace();
>+ PRInt32 begin = 0;
>+ PRInt32 end = 0;
>+ while (begin < language.Length()) {
>+ end = language.FindChar(PRUnichar(','), PR_FALSE, begin);
>+ if (end == kNotFound) {
>+ end = language.Length();
>+ }
>+ nsAutoString httpLang;
>+ language.Mid(httpLang, begin, end);
>+ PRBool match = DashMatchCompare(*pseudoClass->mString, httpLang, PR_FALSE);
Ugh, don't construct an nsAutoString and copy into it, just use
|Substring(language, begin, end-begin)|. This requires that the
parameter be a |const nsAString&| as I said above.
>+ if (match) {
>+ result = PRBool(localTrue == match);
>+ break;
>+ }
>+ begin = end + 1;
>+ }
>+ }
>+ }
>+ }
>+ }
>+ }
> }
> else if (IsEventPseudo(pseudoClass->mAtom)) {
>Index: mozilla/content/html/style/src/nsICSSStyleRule.h
>===================================================================
>RCS file: /m/pub/mozilla/content/html/style/src/nsICSSStyleRule.h,v
>retrieving revision 3.22
>diff -u -2 -r3.22 nsICSSStyleRule.h
>--- nsICSSStyleRule.h 2001/09/25 01:31:18 3.22
>+++ nsICSSStyleRule.h 2001/10/19 10:41:47
>@@ -64,4 +64,17 @@
> };
>
>+struct nsAtomStringList {
>+public:
>+ nsAtomStringList(nsIAtom* aAtom, const nsString *aString = nsnull);
>+ nsAtomStringList(const nsString& aAtomValue, const nsString *aString = nsnull);
>+ nsAtomStringList(const nsAtomStringList& aCopy);
>+ ~nsAtomStringList(void);
>+ PRBool Equals(const nsAtomStringList* aOther) const;
>+
>+ nsIAtom* mAtom;
>+ const nsString* mString;
>+ nsAtomStringList* mNext;
>+};
Bloat is a concern here, as Daniel said. Perhaps the language string
should be an atom rather than a string? Also, mAtom should probably
be an |nsCOMPtr<nsIAtom> mAtom| since it is an owning pointer, rather
than dealing with manual NS_ADDREF/NS_RELEASE. There is a precedent
for atomizing language strings -- see nsILanguageAtom in addition to
nsIAtom. Using |nsString*| rather than |nsString&| and |nsString| is
rather unconventional, although in this case we certainly don't want
the bloat in all cases.
Also, one could construct the linked list
out of two classes, one derived from the other, and having an nsString
member at the end being the only difference in the derived class, ensuring
that you always had an instance of the derived class if the pseudo is
lang, and casting. This would cause zero bloat increase except when there
is a :lang pseudo.
Assignee | ||
Comment 55•23 years ago
|
||
> Why not a while-do without the extra if?
Or, even better, a |for| loop.
Comment 56•23 years ago
|
||
Thanks for the review. I made the changes above except:
- kept nsString: one of the methods is used.
- did not convert mString to an atom: it would not bring much benefits if any,
the language comparisons need to be case-insensitive, nsILanguageAtom is not an
atom either.
- did not add the casting: the total bloat is 8k (approx. 2000 strucs throughout
the app), I chose legibility and maintainability over footprint but it can be
debated - your call, really.
Marc: s/r please.
Comment 57•23 years ago
|
||
Assignee | ||
Comment 58•23 years ago
|
||
> - kept nsString: one of the methods is used.
Which method? Many of the methods have replacements. (See nsReadableUtils.h.)
Assignee | ||
Comment 59•23 years ago
|
||
The performance of this patch looks even worse than the previous one.
SelectorMatchesData::GetLang should return an nsAFlatString&, not fill in an
nsAWritableString&. I think you should also fix the other problems I mentioned.
Comment 60•23 years ago
|
||
- DashMatchCompare() needs CharAt().
- The casting would hurt the performance on the copy constructor.
Result:
- I just made a small change in my tree for GetLang() to return a pointer.
Marc: s/r please.
Summary: CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes [SELECT] → [review]CSS2 :lang pseudo-class won't match on LANG and xml:lang attributes [SELECT]
Assignee | ||
Comment 61•23 years ago
|
||
Instead of CharAt you can use:
nsAString::const_iterator iter;
...
*comparedValue.BeginReading(iter).advance(baseLen) != PRUnichar('-')
You should also address the other comments.
Assignee | ||
Comment 62•23 years ago
|
||
... or once I check in the patch on bug 104651 you could just use const
|nsASingleFragmentString&| with the result of |Substring| on a known flat or
single fragment string.
Comment 63•23 years ago
|
||
waterson/attinasi: please take a look at the performance problem below...
> Instead of CharAt you can use:
> nsAString::const_iterator iter;
> *comparedValue.BeginReading(iter).advance(baseLen) != PRUnichar('-')
The proposed patch creates a nsAutoString on the stack (guaranteed single
fragment) and then calls CharAt(), both of fairly negligible CPU time. I'll
leave it to super-reviewers to decide which solution is better but at first
sight, the code above seems less legible and not really more efficient.
> You should also address the other comments.
I think that I included all the other changes except for the atomization of
language strings and the overlapping structs because of the reasons listed in my
previous comments.
Comment 64•23 years ago
|
||
I stand corrected: your solution with "Substring/const_iterator" is twice as fast
as "Mid/CharAt". The times are quite variable but in average I get approximately
8 microseconds instead of 16. Thanks!
Comment 65•23 years ago
|
||
What is the overall performance impact on pages that do not make use of this
feature? Also, there is some bloat here that is not going to help us make
Mozilla smaller. Do we really need this now?
As for the code, assuming that the base performance is unchanged, sr=attinasi,
although it might be nice to move the logic out of SelectorMatches into an
inline method to keep SelectorMatches more compact.
Assignee | ||
Comment 66•23 years ago
|
||
Creating an nsAutoString on the stack is *not* cheap when you do it many times.
It used to be a significant percentage of time in SelectorMatches (above 10%, I
think) was spent in the constructor and destructor of nsAutoString.
Assignee | ||
Comment 67•23 years ago
|
||
If you check this in, I'm going to have to go clean up the performance problems
myself. I guess I'll just become the one who cleans up after you since you seem
intent on checking this in.
Comment 68•23 years ago
|
||
My apologies if I did not make myself clear: the patch I intended to checkin
addresses all your concerns about performance. The measurements I took after
your comments yesterday showed that your solution resulted in an execution time
of 8 microseconds instead of 16 microseconds and, of course, that's the one I had
in my tree.
Anyhow, it is not the right time to work on features like this. We'll reconsider
later if the :lang() pseudo-class is worth 8k of bloat. MacIE5 implements this
feature but not WinIE5 so the number of pages that currently use it is
negligible.
The 8k bloat seems to me unavoidable unless we accept a loss of performance.
I'll attach an updated patch as a backup for when we come back to it.
Target Milestone: mozilla0.9.6 → mozilla1.0
Comment 69•23 years ago
|
||
Assignee | ||
Comment 70•23 years ago
|
||
There are two problems with that patch that I'd see at a quick glance:
* GetLang could just return nsAFlatString&, not nsString*, and the IsEmpty
check could be done by the caller.
* GetLang should be caching the entire lang check, not just the first part of it.
Comment 71•23 years ago
|
||
>- did not convert mString to an atom: it would not bring much benefits if any,
>the language comparisons need to be case-insensitive, nsILanguageAtom is not an
>atom either.
The langGroup is an atom (which by the way is used in the font engine to select
adequate fonts). Could someone take heed to my request to have other string-like
comparison APIs directly on nsIAtom? nsIAtom::StringEquals[IgnoreCase](aString),
nsIAtom::StringEqualsIgnoreCase(anotherAtom), so that the ever changing string
fu can be localized in one place. The usefulness of these APIs seems to be
manifest in several contexts.
Assignee | ||
Comment 72•23 years ago
|
||
I don't think those APIs make sense -- I'd rather see a DependentAtomString
function that returns a |const nsAFlatString&| (really a const
nsDependentString) that is a dependent string on the atom, and then you can test
(Compare(DependentAtomString(atom1), DependentAtomString(atom2),
nsCaseInsensitiveStringComparator()) == 0).
Comment 73•23 years ago
|
||
Although this wouldn't be |nsresult|-like, so long as there is an easy way to do
these string-fu operations which occur frequently and are often handled by
people in suboptimal ways (e.g., string copies).
Updated•23 years ago
|
Keywords: mozilla1.0
Comment 74•23 years ago
|
||
David Baron told me the existence of this bug when he was reivewing
bug 105199. Both bugs are handling "lang" html attribute, but probably
for different purpose. Could the experts here take a look at that bug?
We probably need a single patch to address both problems.
Comment 75•23 years ago
|
||
> We'll reconsider later if the :lang() pseudo-class is worth 8k of bloat.
Please note that this blocks bug 16206, which is an HTML 4 compliance issue.
While the Q element doesn't handle different languages (and nesting levels)
intelligently, there's not much point in using it instead of using the
presentational equivalent.
Assignee | ||
Comment 76•23 years ago
|
||
See also bug 115121.
Comment 77•23 years ago
|
||
Moving to mozilla1.1. Engineers are overloaded!
Target Milestone: mozilla1.0 → mozilla1.1
Comment 78•23 years ago
|
||
Bulk moving from Moz1.1 to future-P1. I will pull from this list when scheduling
work post Mozilla1.0.
Priority: P3 → P1
Target Milestone: mozilla1.1 → Future
Comment 79•23 years ago
|
||
cc'ing myself
Comment 80•22 years ago
|
||
There is a test case at http://www.richinstyle.com/test/language/lang2.php.
Assignee | ||
Updated•22 years ago
|
Target Milestone: Future → mozilla1.2alpha
Assignee | ||
Comment 81•22 years ago
|
||
Assigning pierre's remaining Style System-related bugs to myself.
Assignee: pierre → dbaron
Status: ASSIGNED → NEW
Priority: P1 → P2
Assignee | ||
Updated•22 years ago
|
Whiteboard: (py8ieh: need a thorough test case) → [patch](py8ieh: need a thorough test case)
Comment 82•22 years ago
|
||
Essentially patch 2.2, but the return value of GetLang is not tested for being
an empty string twice. GetLang simply always returns a string reference.
Comment 83•22 years ago
|
||
The patch in attachment 90923 [details] [diff] [review] is basically patch 2.2 but with one little
optimization dbaron mentioned: the return value of GetLang should not be tested
twice for being empty. GetLang now always returns a nsString*.
With regard to other optimizations mentioned: I don't think it is good to change
GetLang to take a nsAutoString& parameter and put the value there. This would
involve copying a string for no reason. If we don't have mLanguage in
SelectorMatchesData we'd at least have to call element->GetAttr() for every
GetLang call and copy the string. I hardly think this is an optimization.
With regard to caching, is this really beneficial? Where do you suggest it to
happen? We might be able to merge the DashMatchCompare functionality into
GetLang (renaming it to IsLang or so) and cache the results for various lookups
with different input strings. But this would make the SelectorMatchesData class
even heavier. Is it worth it?
Assignee | ||
Comment 84•22 years ago
|
||
This is the previous patch, but merged to the current trunk (the diffs for that
patch are against a really really old tree). I fixed a few style nits and
other things, but there's still a few other things that I'd like to fix.
Also, what's the deal with xml:lang?
Assignee | ||
Comment 85•22 years ago
|
||
Comment on attachment 91006 [details] [diff] [review]
variant of previous patch merged to current trunk
ignore this patch. There's something wrong with the selector matching code.
(I should stop trying to do three things at once...)
Attachment #91006 -
Attachment is obsolete: true
Comment 86•22 years ago
|
||
> Also, what's the deal with xml:lang?
At the time when I wrote the patch the namespace handling was b0rken. Does it
work correctly nowadays?
I think the only necessary change is in GetLang where
if (attrCount > 0) {
nsAutoString value;
nsresult attrState = element->GetAttr(kNameSpaceID_HTML,
nsHTMLAtoms::lang, value);
if (attrState == NS_CONTENT_ATTR_HAS_VALUE) {
mLanguage = value;
break;
}
has to be replaced with something like
if (attrCount > 0) {
nsAutoString value;
if ((IS_XML_DTD &&
element->GetAttr(kNameSpaceID_XML, nsHTMLAtoms::lang, value) ==
NS_CONTENT_ATTR_HAS_VALUE)
|| (element->GetAttr(kNameSpaceID_HTML, nsHTMLAtoms::lang, value)
== NS_CONTENT_ATTR_HAS_VALUE)) {
mLanguage = value;
break;
}
What I don't know is how IS_XML_DTD must look like.
Assignee | ||
Comment 87•22 years ago
|
||
This is a mostly-working merged patch (modulo some issues with attachment
54202 [details]). I probably won't have a chance to look at this more for at least a few
days, though, and I haven't looked through the whole thing yet closely.
I did add the xml:lang support very quickly (and it seems to work based on
attachment 48154 [details]). The attribute problems on HTML content should be fixed
(thanks to sicking, I think).
Comment 88•22 years ago
|
||
More complete test based on 42159 and 42160.
Attachment #42159 -
Attachment is obsolete: true
Attachment #42160 -
Attachment is obsolete: true
Comment 89•22 years ago
|
||
I've attached a bit more complete test based on earlier tests of mine.
With the patch applied the other tests in the attachments here seem to work.
The just attached test also mostly works. The only problem is the second test
with dynamically changing quotes (I am using the quotes patch from bug 24861).
The change discussed in comment #20 does not fix the problem.
Not that a very similar test which uses colors instead of quotes works just fine.
Updated•22 years ago
|
Whiteboard: [patch](py8ieh: need a thorough test case) → [patch](py8ieh: need a thorough test case) (py8ieh:need test case which dynamically changes the xml:lang of a grandparent element)
Comment 90•22 years ago
|
||
Comment 91•22 years ago
|
||
Attachment 91210 [details] contains the XHTML versio of the previous attachment. The
results are not as good:
- the same test as in the HTML test fails (expected, I guess)
- the last test, adding a paragraph, fails in two ways
+ no coloring happens
+ the paragraph is not added as a paragraph. I.e., it's not on a separate
line
The latter might be my mistake. Let me know if it is.
- when using setAttribute("xml:lang", la) instead of setAttribute("lang", la)
the button-press tests fail. When using "lang" the coloring test succeeds.
Note that this does contain setting the xml:lang of a grandfather node.
Comment 92•22 years ago
|
||
> The change discussed in comment #20 does not fix the problem.
Since 'lang' is a special attribute with cascading effects, it may be that its
impact has to become FRAMECHANGE too:
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsGenericHTMLElement.cpp#3504
Comment 93•22 years ago
|
||
> Since 'lang' is a special attribute with cascading effects, it may be that its
> impact has to become FRAMECHANGE too:
Yes, that seems to work:
diff -u -r1.367 nsGenericHTMLElement.cpp
--- content/html/content/src/nsGenericHTMLElement.cpp 4 Jul 2002 04:30:19 -0000
1.367
+++ content/html/content/src/nsGenericHTMLElement.cpp 19 Jul 2002 04:09:40 -0000
@@ -3509,7 +3509,7 @@
return PR_TRUE;
}
else if (nsHTMLAtoms::lang == aAttribute) {
- aHint = NS_STYLE_HINT_REFLOW; // LANG attribute affects font selection
+ aHint = NS_STYLE_HINT_FRAMECHANGE; // LANG attribute affects font selection
return PR_TRUE;
}
/*
The only strange effect is that after pressing the butten the layout is a bit
different. It seems as if an additional empty line got added. I get the same
result with the XHTML test case.
The only difference between the HTML and XHTML test case is now that adding a
new paragraph simply appends the new text to the last line instead of adding a
new line. And this only for XHTML.
Comment 94•22 years ago
|
||
> The only strange effect is that after pressing the butten the layout is a bit
> different.
Might be a layout bug then.
> The only difference between the HTML and XHTML test case is now that adding a
> new paragraph simply appends the new text to the last line instead of adding a
> new line. And this only for XHTML.
Looking at your script, the XHTML namespace seems to be missing:
- var newp = document.createElement("p");
+ var newp = document.createElementNS("http://www.w3.org/1999/xhtml", "p");
- newp.setAttribute("xml:lang", la);
+ newp.setAttribute("lang", la);
Comment 95•22 years ago
|
||
Fixing the Javascript code: use createElementNS.
Attachment #91210 -
Attachment is obsolete: true
Comment 96•22 years ago
|
||
> Looking at your script, the XHTML namespace seems to be missing:
You're right, thanks. I've updated the test, the result is in attachment 91921 [details].
The only issue remaining is the layout change after executing the DOM code for
the first time.
The patch at hand here seems to work fine. Can we get it reviewed and checked in?
Comment 97•22 years ago
|
||
Comment on attachment 91026 [details] [diff] [review]
mostly working merged patch
Is the following XXX comment still necessary? Looks like the patch is taking
care of "xml:base" too, no?
+ // XXX What about xml:lang ?
+ PRInt32 attrCount = 0;
+ content->GetAttrCount(attrCount);
+ if (attrCount > 0) {
+ nsAutoString value;
+ nsresult attrState = content->GetAttr(kNameSpaceID_XML,
+ nsHTMLAtoms::lang, value);
+ if (attrState != NS_CONTENT_ATTR_HAS_VALUE &&
+ content->IsContentOfType(nsIContent::eHTML)) {
+ attrState = content->GetAttr(kNameSpaceID_None,
+ nsHTMLAtoms::lang, value);
+ }
Mind attaching a patch for the whole lot, i.e., with the other fixup?
Comment 98•22 years ago
|
||
s/xml:base/xml:lang/
Comment 99•22 years ago
|
||
Also, the comment here should probably tips at the :lang() pseudo too...
+ aHint = NS_STYLE_HINT_FRAMECHANGE; // LANG attribute affects font selection
Comment 100•22 years ago
|
||
This is exactly dbaron's patch without the comment questioning the xml:lang
implementation.
Attachment #90923 -
Attachment is obsolete: true
Comment 101•22 years ago
|
||
Patch misses the FRAMECHANGE bit, is it intentional?
Comment 102•22 years ago
|
||
> Patch misses the FRAMECHANGE bit, is it intentional?
Yes, I don't know what exactly you want.
Comment 103•22 years ago
|
||
I mean the diff that you have in comment #93 (for which the comment needs to be
updated to reference :lang too).
Comment 104•22 years ago
|
||
> I mean the diff that you have in comment #93 (for which the comment needs to
> be updated to reference :lang too).
Should this change really be part of this patch or should it be separately filed?
If it should be here, how do you think the comment should change?
return PR_TRUE;
}
else if (nsHTMLAtoms::lang == aAttribute) {
// LANG attribute affects font selection and the :lang() pseudo class
// in CSS can cause arbitrary changes
aHint = NS_STYLE_HINT_FRAMECHANGE;
return PR_TRUE;
}
/*
Comment 105•22 years ago
|
||
> Should this change really be part of this patch or should it be separately
> filed?
I think so, since it is needed for the patch to pass the testcases. It is the
layout glitch that you mentioned that needs another bug (which might linger
there as usual. In the meantime, let's have information wins over presentation.
Plus it will be so rare to see a real website with a JS that flips the 'lang'
anyway...).
> If it should be here, how do you think the comment should change?
Your proposed comment looks okay to me.
Comment 106•22 years ago
|
||
Again, dbaron's patch plus the comment change plus the patch described in
comment 93 with updated comment in the sources.
Attachment #41289 -
Attachment is obsolete: true
Attachment #41526 -
Attachment is obsolete: true
Updated•22 years ago
|
Attachment #42158 -
Attachment is obsolete: true
Attachment #46332 -
Attachment is obsolete: true
Attachment #46333 -
Attachment is obsolete: true
Attachment #48738 -
Attachment is obsolete: true
Attachment #49051 -
Attachment is obsolete: true
Attachment #49616 -
Attachment is obsolete: true
Attachment #91922 -
Attachment is obsolete: true
Comment 107•22 years ago
|
||
Comment on attachment 91928 [details] [diff] [review]
Same as last patch + the change to set FRAMECHANGE hint for lang attribute
r/sr=rbs. Nice job guys. This bug has been an exercise of determination and
resilience. An edge over IE6 since my copy of it is failing miserably on the
testcases.
Attachment #91928 -
Flags: review+
Comment 108•22 years ago
|
||
I have applied the patch to check out the layout glitch... It is one of those
flickers that sometimes happen in layout. It goes away at the slightest
intention to resize the window -- meaning it is layout.
All the valid attached testcases pass. The one in comment #80 has a bug. It
claims that the lang of the document is 'de' when there is actually no lang
there (e.g., the context menu doesn't display the 'Properties'). But continuing
the test and clicking on the ':lang()' link shows that the rest works fine
(again 'Properties' is a good indicator to see if the lang is set).
Comment 109•22 years ago
|
||
Comment on attachment 91928 [details] [diff] [review]
Same as last patch + the change to set FRAMECHANGE hint for lang attribute
>+++ mozilla/content/html/content/src/nsGenericHTMLElement.cpp 19 Jul 2002 08:02:01 -0000
>@@ -3509,7 +3509,9 @@
> return PR_TRUE;
> }
> else if (nsHTMLAtoms::lang == aAttribute) {
Existing else after return is a non-sequitur -- fix while you're hacking here?
/be
>- aHint = NS_STYLE_HINT_REFLOW; // LANG attribute affects font selection
>+ // LANG attribute affects font selection and the :lang() pseudo class
>+ // in CSS can cause arbitrary changes, bug 35768
>+ aHint = NS_STYLE_HINT_FRAMECHANGE;
> return PR_TRUE;
> }
> /*
Comment 110•22 years ago
|
||
Following Brendan's comment I've removed all the useless elses.
Attachment #91928 -
Attachment is obsolete: true
Assignee | ||
Comment 112•22 years ago
|
||
Making the change hint for a lang attribute unconditionally be a framechange is
incorrect. When attributes change, we will reresolve style if necessary, and
compute the correct things that need to happen as a result of the style change.
If AttributeAffectsStyle is implemented correctly, this should be unnecessary.
(It probably isn't yet, for xml:lang.)
There were also a few other changes I wanted to make to this patch.
Assignee | ||
Comment 113•22 years ago
|
||
That |#if 0|ed code about the class attribute being a framechange (in
nsGenericHTMLElement.cpp) should just be removed completely.
The mRelevantAttributes code should actually be sufficient for the
AttributeAffectsStyle changes, since it's not namespace sensitive (although
perhaps it's worth a comment that if it ever becomes namespace-sensitive, what
we care about is the un-namespaced lang attribute on HTML-namespaced content and
the lang attribute in the XML namespace.
Comment 114•22 years ago
|
||
> The mRelevantAttributes code should actually be sufficient
It is not sufficient since the patch didn't pass all the tescases. I suspect the
reason why it is not sufficient is because the frames that handle the quotes are
created as generated frames and these have to be changed when the lang is
changed. I know that it is possible to fix that, but as I noted, the dynamic
change of 'lang' is so rare that I wouldn't give it such a high prioririty to
the point of totally blocking the patch (it was mostly idealism vs.
practicality).
I can comment more about the peculiarity of xml:lang now that I have traced the
code. But anyone tracing the code will see too.
Assignee | ||
Comment 115•22 years ago
|
||
Is the testcase that doesn't pass one that dynamically changes the 'quotes'
property? It seems much more likely that the problem is one with dynamic
changes to a specific property, rather than dynamic changes that affect certain
types of selectors.
Comment 116•22 years ago
|
||
> Is the testcase that doesn't pass one that dynamically changes the 'quotes'
> property?
The test failing without the extra patch is the one qhich selects the quotes
based on the lang attribute. The very similar test which selects a color based
on the attribute succeeds.
Assignee | ||
Updated•22 years ago
|
Attachment #91998 -
Flags: needs-work+
Assignee | ||
Comment 117•22 years ago
|
||
Comment on attachment 91998 [details] [diff] [review]
Same as last patch with useless 'else's removed
That means the lang attribute change in nsGenericHTMLElement.cpp should be
replaced by a change to nsStyleQuotes::CalcDifference in nsStyleStruct.cpp
(perhaps with a comment that it could be a reflow rather than a framechange if
our quotes code worked differently).
Comment 118•22 years ago
|
||
> That means the lang attribute change in nsGenericHTMLElement.cpp should be
> replaced by a change to nsStyleQuotes::CalcDifference in nsStyleStruct.cpp
I've tried this patch after removing the nsGenericHTMLElement.cpp change.
diff -u -r3.34 nsStyleStruct.cpp
--- content/shared/src/nsStyleStruct.cpp 3 Jul 2002 17:14:31 -0000
3.34
+++ content/shared/src/nsStyleStruct.cpp 23 Jul 2002 07:18:42 -0000
@@ -1251,7 +1251,7 @@
PRUint32 ix = (mQuotesCount * 2);
while (0 < ix--) {
if (mQuotes[ix] != aOther.mQuotes[ix]) {
- return NS_STYLE_HINT_REFLOW;
+ return NS_STYLE_HINT_FRAMECHANGE;
}
}
I think this is what you went. The result isn't good: the test case does not
work. Did you have something else in mind?
Assignee | ||
Comment 119•22 years ago
|
||
You need to change both occurrences of NS_STYLE_HINT_REFLOW in that function.
(Which test case is it that doesn't work?)
Comment 120•22 years ago
|
||
> You need to change both occurrences of NS_STYLE_HINT_REFLOW in that function.
Nope, no improvement.
> (Which test case is it that doesn't work?)
The one in attachment 91046 [details] and the one in 91921. In both cases the first test
with buttons. The other tests work just like before the
nsGenericHTMLElement.cpp change.
Assignee | ||
Comment 121•22 years ago
|
||
I don't see how it would make a difference in the testcase in question, but we
also need to move the quotes section in nsStyleContext::CalcStyleDifference into
the framechange section rather than the reflow section.
Comment 122•22 years ago
|
||
Incorporating dbaron's suggestions. This actually does produce the correct
results.
Comment 123•22 years ago
|
||
I've implemented and tested what dbaron suggested. The patch in attachment
92599 [details] [diff] [review] removes the change to nsGenericHTMLElement infavor of changes to
nsStyleStruct and nsStyleContext. I've also added appropriate comments. If
this patch acceptable?
Assignee | ||
Comment 124•22 years ago
|
||
Comment on attachment 92599 [details] [diff] [review]
patch for Change StyleStruct and StyleContext instead of GenericHTMLElemtn
A bunch of nits, and two significant comments (first, moving the
document language stuff out of SelectorMatches and into the
RuleProcessorData; second, whether AtomStringList should hold raw
PRUnichar* rather than nsString*). I *think* these should be it. (Yes,
I realize I'm making this drag on forever...):
> Index: mozilla/content/html/style/src/nsCSSParser.cpp
> - nsIAtom* pseudo = NS_NewAtom(buffer);
> + nsCOMPtr<nsIAtom> pseudo(dont_AddRef(NS_NewAtom(buffer)));
> +
Could be the equivalent form
nsCOMPtr<nsIAtom> pseudo = do_GetAtom(buffer);
and there's no need to introduce extra whitespace, I don't think.
The parsing code is written in the style of the parser, which I can't
stand much (since early returns are underused), but I won't complain
since that's the style of the parser...
> Index: mozilla/content/html/style/src/nsCSSStyleRule.cpp
> - mAtom = NS_NewAtom(aAtomValue);
> + mAtom = dont_AddRef(NS_NewAtom(aAtomValue));
Likewise, |mAtom = do_GetAtom(aAtomValue);|.
> Index: mozilla/content/html/style/src/nsCSSStyleSheet.cpp
> CSSStyleSheetImpl::GetEnabled(PRBool& aEnabled) const
> {
> - aEnabled = ((PR_TRUE == mDisabled) ? PR_FALSE : PR_TRUE);
> + aEnabled = (mDisabled ? PR_FALSE : PR_TRUE);
Ugh. How about |aEnabled = !mDisabled|?
> @@ -2063,7 +2063,7 @@
> CSSStyleSheetImpl::SetEnabled(PRBool aEnabled)
> {
> PRBool oldState = mDisabled;
> - mDisabled = ((PR_TRUE == aEnabled) ? PR_FALSE : PR_TRUE);
> + mDisabled = (aEnabled ? PR_FALSE : PR_TRUE);
Likewise.
> else if (nsCSSAtoms::langPseudo == pseudoClass->mAtom) {
> - // XXX not yet implemented
> - result = PR_FALSE;
> + NS_ASSERTION(nsnull != pseudoClass->mString, "null lang parameter");
> + result = localFalse;
> + if (pseudoClass->mString) {
> + // We have to determine the language of the current element. Since> + // this is currently no property and since the language is inherited
> + // from the parent we have to be prepared to look at all parent
> + // nodes. The language itself is encoded in the LANG attribute.
> + const nsString* lang = data.GetLang();
> + if (!lang->IsEmpty()) {
> + result = PRBool(localTrue == DashMatchCompare(*lang, *pseudoClass->mString, PR_FALSE));
> + }
> + else {
> + nsIDocument *doc;
> + if (NS_SUCCEEDED(data.mContent->GetDocument(doc))) {
> + // Try to get the language from the HTTP header or if this
> + // is missing as well from the preferences.
> + // The content language can be a comma-separated list of
> + // language codes.
> + nsAutoString language;
> + if (NS_SUCCEEDED(doc->GetContentLanguage(language))) {
This really needs to be in the RuleProcessorData (perhaps a second
lazy getter function?).
> + language.StripWhitespace();
> + PRInt32 begin = 0;
> + PRInt32 end = 0;
> + PRInt32 len = language.Length();
> + while (begin < len) {
> + end = language.FindChar(PRUnichar(','), begin);
> + if (end == kNotFound) {
> + end = len;
> + }
> + PRBool match = DashMatchCompare(*pseudoClass->mString, Substring(language, begin, end-begin), PR_FALSE);
> + if (match) {
> + result = PRBool(localTrue == match);
This could be simplified to
result = localTrue;
> + break;
> + }
> + begin = end + 1;
> Index: mozilla/content/html/style/src/nsICSSStyleRule.h
> +struct nsAtomStringList {
It's worth a comment above this that the class is optimized for the case
that there is no string and it should not be used in cases where the
string is usually present. However, that might not be necessary given
the following...
Also, is there a reason we want this class to store an |nsString*|
rather than a raw |PRUnichar*| (which requires one allocation rather
than two)? I certainly would have used the latter if I were writing
this myself.
Assignee | ||
Comment 125•22 years ago
|
||
Two notes:
* we should add a comment pointing to http://www.w3.org/TR/xhtml1/#C_7 which
says that the xml:lang takes precedence over lang on an HTML-namespace element.
* xml:lang="" should probably be taken to "un-define" the language as far as
the document is concerned. We should double-check that we do that.
Assignee | ||
Comment 126•22 years ago
|
||
Never mind about the content language stuff needing to be in the rule processor
data -- that won't speed it up significantly, since there's a good bit of
matching to do even once we have it.
Assignee | ||
Comment 127•22 years ago
|
||
This is the previous patch, with the changes I mentioned in comment 124,
excluding the one I mentioned in comment 126, plus a fix for a leaked reference
to an nsIDocument in SelectorMatches and an additional comment change I had
lying around in my tree.
Attachment #54201 -
Attachment is obsolete: true
Attachment #55546 -
Attachment is obsolete: true
Attachment #56308 -
Attachment is obsolete: true
Attachment #91026 -
Attachment is obsolete: true
Attachment #91998 -
Attachment is obsolete: true
Attachment #92599 -
Attachment is obsolete: true
Comment 128•22 years ago
|
||
nsCSSParser::ParseLangSelector:
+ if (! GetToken(aErrorCode, PR_FALSE)) { // premature eof
+ REPORT_UNEXPECTED_EOF(NS_LITERAL_STRING("argument to :lang selector"));
+ aParsingStatus = SELECTOR_PARSING_STOPPED_ERROR;
+ return;
+ }
Should that really be a PR_FALSE in GetToken()? Is whitespace not allowed after
the '('? (We need that CSS3 grammar ;) ). Especially since we do
"ExpectSymbol(aErrorCode, ')', PR_TRUE)" later...
The code in nsAtomStringList::Equals has lots of comparisons to "nsnull" that
should, imo, be simple boolean tests (eg |if (aOther)| instead of
|if (nsnull != aOther)|. I guess that's the style of the code in that file,
but... (David's call as module owner here)
nsAtomStringList is checking the "rest of the list" before checking the
existence and equality of mString and aOther.mString. Was this done for a
reason? If so, add a comment explaining that reason so some well-meaning soul
does not "fix" it. (David mentioned this in comment 124, I see).
It feels like there should be a comment where you declare mPseudoClassList as an
nsAtomStringList that explains what the "string" part of nsAtomStringList means
for a pseudo-class.... (that this is used for "function pseudo-classes and the
string is whatever was in the parens).
Would it be worthwhile to make mLanguage in RuleProcessorData an nsString* (or
even nsAutoString*) instead of an nsAutoString and to allocate it on the first
call to GetLang()? The null-ness (or not) of the pointer could then be used in
place of mIsLanguageValid, methinks. It should make the common-case
RuleProcessorData struct smaller and should not really make the uncommon case
that much slower, imo... (just a suggestion; feel free to tell me I'm on crack
here).
+ result = localTrue == DashMatchCompare(*lang,
+ nsDependentString(pseudoClass->mString), PR_FALSE);
...
+ nsDependentString langString(pseudoClass->mString);
...
+ if (DashMatchCompare(langString,
+ Substring(language, begin, end-begin),
+ PR_FALSE)) {
One of those two DashMatchCompare calls has the string args in the wrong order.
It would be clear which is wrong if DashMatchCompare had argument names that
made it clear which argument comes from the selector and which argument comes
from the content node or if it had a comment on the topic. As far as I can
tell, the DashMatchCompare against the Substring() should have Substring() and
langString reversed.
If there is no content-language on the document and there is no lang attr on
anything, we will treat :lang() as a no-op with this code, no? That is, both
a:lang(foo) and a:lang(bar) will match an anchor. Further, both :lang(foo) and
:not(:lang(foo)) will match everything... Is this really what we want?
> + result = PRBool(localTrue == DashMatchCompare(value,
attr->mValue, isCaseSensitive));
Do you really need the PRBool() part of that?
Please adjust the comments in nsStyleContext.cpp that currently say:
568 // FRAMECHANGE Structs: Display, XUL, Content, UserInterface
637 // REFLOW Structs: Font, Margin, Padding, Border, List, Position, Text,
TextReset,
638 // Visibility, Quotes, Table, TableBorder
since you are making Quotes a FRAMECHANGE struct.
The rest looks good.
Assignee | ||
Comment 129•22 years ago
|
||
http://www.w3.org/TR/css3-selectors/#grammar is clear that whitespace is allowed
in functional pseudos.
Assignee | ||
Comment 130•22 years ago
|
||
> If there is no content-language on the document and there is no lang attr on
> anything, we will treat :lang() as a no-op with this code, no? That is, both
> a:lang(foo) and a:lang(bar) will match an anchor. Further, both :lang(foo) and
> :not(:lang(foo)) will match everything... Is this really what we want?
I don't see what makes you think this. I did make the change that makes
":lang()" never match, though (and thus ":not(:lang())" always matches).
I'll attach a new patch that should address bzbarsky's comments.
Assignee | ||
Comment 131•22 years ago
|
||
I didn't bother with the nsAutoString pointer -- I don't think the
RuleProcessorData is constructed enough that we have to worry about a single
nsAutoString, although I could be wrong. Also see my previous comments.
Attachment #94955 -
Attachment is obsolete: true
Comment 132•22 years ago
|
||
Comment on attachment 95149 [details] [diff] [review]
patch updated according to bzbarsky's comments
> + result = attributeSubstring.Equals(aSelectorValue,
> + nsCaseInsensitiveStringComparator());
Odd indent there (one char off).
> I don't see what makes you think this.
I had missed the |+ result = localFalse;| line at the beginning of the
lang-matching code.. ;)
sr=bzbarsky
Attachment #95149 -
Flags: superreview+
Assignee | ||
Comment 133•22 years ago
|
||
Checked in to trunk, 2002-08-14 05:34 PDT. Thanks to all who contributed,
especially Ulrich Drepper, for all the work that's gone into this patch.
(That odd indentation was intentional, to avoid crossing the 80 character boundary.)
Status: NEW → RESOLVED
Closed: 25 years ago → 22 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 134•22 years ago
|
||
This patch goes on top of the previous one -- it moves to bzbarsky's suggestion
in comment 128 of using an |nsAutoString*| in RuleProcessorData for better
performance in the case when there are no :lang selectors.
I just checked this patch in because tinderbox showed a roughly 1.5% page load
performance regression on two separate tinderboxes after this patch landed, and
this is the only thing I can think of that might have caused it.
Assignee | ||
Comment 135•22 years ago
|
||
I also removed the extraneous |SetLength(0)| which peterv pointed out.
Assignee | ||
Comment 136•22 years ago
|
||
Well, either the two cycles where the page load numbers were higher were a
random blip that's a bit higher than most, or that really did make a difference,
because the numbers look like they're back where they were. I really need to
remember never to underestimate the amount of time that nsAutoString's
constructor takes!
Comment 137•22 years ago
|
||
This just adds a "delete mLanguage;" to the destructor of RuleProcessorData
Assignee | ||
Comment 138•22 years ago
|
||
Comment on attachment 95285 [details] [diff] [review]
Deallocate the string too
Yeah, thanks. :-)
rs=dbaron
Attachment #95285 -
Flags: superreview+
Comment on attachment 95285 [details] [diff] [review]
Deallocate the string too
r=me
Attachment #95285 -
Flags: review+
Comment 140•22 years ago
|
||
Woo hoo! Finally resolved after a year. Thanks for the last changes.
Could somebody now at least comment on the patch in bug 24861? It's one of the
few internationalization related bugs which are outstanding. And another step
close to supporting CSS2.
Comment 141•22 years ago
|
||
I'm not sure about this:
<p lang="de">bla<span lang="en">?</span></p>
and the style:
*:lang(de){color:blue}
..should the "?" be blue or black (default)...?
Comment 142•22 years ago
|
||
It should be blue, since color inherits and you've specified no rules on the
lang="en" content to override that color.
Comment 143•22 years ago
|
||
In all the testcases, the xml:lang tests involved inheriting down from another
tag, such as a div being tagged xml:lang whatever and all b tags within it
inherit a certain style.
However, the simple case of styling (for example) all P's of xml:lang whatever
to a given style does not work. Reopening bug, will be attaching testcase to
illustrate.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 144•22 years ago
|
||
In this test, all P's of lang(fr) are red. P's tagged with only the "lang"
attribute get the style. P's tagged with only the "xml:lang" attribute do not.
Since "lang" is deprecated in XHTML 1.1, this becomes a problem.
Comment 145•22 years ago
|
||
if you send that file as text/xml it works.
Assignee | ||
Comment 146•22 years ago
|
||
Comment on attachment 95593 [details]
New xml:lang testcase
xml:lang, and namespaces in general, are not supported in the HTML parser.
Changing mimetype of testcase to "text/xml", which is one of the valid XML MIME
types.
Attachment #95593 -
Attachment mime type: text/html → text/xml
Assignee | ||
Comment 147•22 years ago
|
||
Testcase works fine when changed to text/xml. Marking fixed again.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 148•22 years ago
|
||
verified
Comment 150•22 years ago
|
||
*** Bug 41978 has been marked as a duplicate of this bug. ***
You need to log in
before you can comment on or make changes to this bug.
Description
•