Issue 1321943002: Support for CSSOM CSSNamespaceRule interface

Issue 1321943002: Support for CSSOM CSSNamespaceRule interface (Closed)

Created:
5 years, 3 months ago by ramya.v

Modified:
5 years, 3 months ago

Reviewers:
Timothy Loh, esprehn

CC:
blink-reviews, blink-reviews-style_chromium.org, vivekg_samsung, blink-reviews-css, vivekg, dglazkov+blink, apavlov+blink_chromium.org, Inactive, darktears, rwlbuis, pals, philipj_slow

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Support for CSSOM CSSNamespaceRule interface Support for CSSOM CSSNamespaceRule interface Spec:http://drafts.csswg.org/cssom/#the-cssnamespacerule-interface Changes include 1) Exposing CSSNamespaceRule interface 2) Reflecting namespace rules in stylesheet rule list BUG=389549 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201884

Patch Set 1 #

Patch Set 2 : Updating testcases #

Total comments: 14

Patch Set 3 : Updated as per review comments #

Total comments: 6

Patch Set 4 : Updated as per review comments #

Created: 5 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+311 lines, -19 lines)			Patch
M	LayoutTests/css3/supports-cssom.html	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/css3/supports-cssom-expected.txt	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/cssom/insertrule-namespace-mapping.html	View	1	1 chunk	+1 line, -1 line	0 comments	Download
A	LayoutTests/fast/css/css-namespace-rule.html	View	1 2 3	1 chunk	+42 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/css-namespace-rule-expected.txt	View	1 2 3	1 chunk	+22 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/css/namespaces/insert-namespace-with-same-prefix.xhtml	View	1 2 3	1 chunk	+26 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/dom/css-mediarule-functions.html	View		1 chunk	+10 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/dom/css-mediarule-functions-expected.txt	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/platform/android/virtual/stable/webexposed/global-interface-listing-expected.txt	View		2 chunks	+5 lines, -0 lines	0 comments	Download
M	LayoutTests/platform/android/webexposed/global-interface-listing-expected.txt	View		2 chunks	+5 lines, -0 lines	0 comments	Download
M	LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	LayoutTests/webexposed/global-interface-listing-expected.txt	View	1 2 3	2 chunks	+5 lines, -0 lines	0 comments	Download
M	Source/core/core.gypi	View	1 2 3	2 chunks	+3 lines, -0 lines	0 comments	Download
M	Source/core/css/CSSGroupingRule.cpp	View		1 chunk	+6 lines, -3 lines	0 comments	Download
A	Source/core/css/CSSNamespaceRule.h	View		1 chunk	+45 lines, -0 lines	0 comments	Download
A	Source/core/css/CSSNamespaceRule.cpp	View	1 2 3	1 chunk	+53 lines, -0 lines	0 comments	Download
A	Source/core/css/CSSNamespaceRule.idl	View		1 chunk	+12 lines, -0 lines	0 comments	Download
M	Source/core/css/CSSRule.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/css/CSSRule.idl	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	Source/core/css/CSSStyleSheet.cpp	View	1 2	3 chunks	+10 lines, -5 lines	0 comments	Download
M	Source/core/css/StyleRule.cpp	View		2 chunks	+4 lines, -1 line	0 comments	Download
M	Source/core/css/StyleSheetContents.h	View	1 2 3	4 chunks	+4 lines, -1 line	0 comments	Download
M	Source/core/css/StyleSheetContents.cpp	View	1 2 3	7 chunks	+48 lines, -4 lines	0 comments	Download

Messages

Total messages: 13 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Timothy Loh

https://codereview.chromium.org/1321943002/diff/20001/Source/core/css/CSSNamespaceRule.cpp File Source/core/css/CSSNamespaceRule.cpp (right): https://codereview.chromium.org/1321943002/diff/20001/Source/core/css/CSSNamespaceRule.cpp#newcode27 Source/core/css/CSSNamespaceRule.cpp:27: result.append(m_namespaceRule->prefix()); Should call serializeIdentifier and just use prefix() here. ...

5 years, 3 months ago (2015-09-02 04:45:14 UTC) #3

ramya.v

Updated as per review comments. PTAL! Thanks https://codereview.chromium.org/1321943002/diff/20001/Source/core/css/CSSNamespaceRule.cpp File Source/core/css/CSSNamespaceRule.cpp (right): https://codereview.chromium.org/1321943002/diff/20001/Source/core/css/CSSNamespaceRule.cpp#newcode27 Source/core/css/CSSNamespaceRule.cpp:27: result.append(m_namespaceRule->prefix()); On ...

5 years, 3 months ago (2015-09-02 09:20:54 UTC) #4

ramya.v

https://codereview.chromium.org/1321943002/diff/20001/Source/core/css/CSSNamespaceRule.cpp File Source/core/css/CSSNamespaceRule.cpp (right): https://codereview.chromium.org/1321943002/diff/20001/Source/core/css/CSSNamespaceRule.cpp#newcode27 Source/core/css/CSSNamespaceRule.cpp:27: result.append(m_namespaceRule->prefix()); On 2015/09/02 09:20:54, ramya.v wrote: > On 2015/09/02 ...

5 years, 3 months ago (2015-09-03 04:03:21 UTC) #5

Timothy Loh

https://codereview.chromium.org/1321943002/diff/20001/Source/core/css/CSSNamespaceRule.cpp File Source/core/css/CSSNamespaceRule.cpp (right): https://codereview.chromium.org/1321943002/diff/20001/Source/core/css/CSSNamespaceRule.cpp#newcode27 Source/core/css/CSSNamespaceRule.cpp:27: result.append(m_namespaceRule->prefix()); On 2015/09/03 04:03:21, ramya.v wrote: > On 2015/09/02 ...

5 years, 3 months ago (2015-09-04 01:35:51 UTC) #6

https://codereview.chromium.org/1321943002/diff/20001/Source/core/css/CSSName...
File Source/core/css/CSSNamespaceRule.cpp (right):

https://codereview.chromium.org/1321943002/diff/20001/Source/core/css/CSSName...
Source/core/css/CSSNamespaceRule.cpp:27:
result.append(m_namespaceRule->prefix());
On 2015/09/03 04:03:21, ramya.v wrote:
> On 2015/09/02 09:20:54, ramya.v wrote:
> > On 2015/09/02 04:45:14, Timothy Loh wrote:
> > > Should call serializeIdentifier and just use prefix() here.
> > > 
> > > Also we should check if there is a prefix first as per spec.
> > 
> > As per the spec https://drafts.csswg.org/cssom/#dom-cssrule-csstext
> > cssText attribute must return a serialization of the CSS rule.
> > 
> > For the below test case
> > <!DOCTYPE html>
> > <style>
> >  @namespace html-namespace url(http://www.w3.org/1999/fdfs);
> > </style>
> > <script>
> > alert(document.styleSheets[0].cssRules[0].cssText);
> > </script>
> > 
> > Mozilla's output is @namespace html-namespace
> > url("http://www.w3.org/1999/fdfs");
> > Hence included prefix. 
> 
> 
> 
> In the spec as well it was mentioned as below (prefix included)
> 
> The literal string "@namespace", followed by a single SPACE (U+0020), followed
> by the identifier escaped value of the prefix attribute (if any), followed by
a
> single SPACE (U+0020) if there is a prefix, followed by the URL escaped value
of
> the namespaceURI attribute, followed the character ";" (U+003B).

For the default namespace (no prefix value), this will print an extra space.

https://codereview.chromium.org/1321943002/diff/40001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/1321943002/diff/40001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:161: // import/namespace/charset rules
not sure you want to write namespace here

https://codereview.chromium.org/1321943002/diff/40001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:254:
parserAddNamespace(namespaceRule->prefix(), namespaceRule->uri());
parserAddNamespace won't do what you want here. If a stylesheet specifies a
namespace prefix multiple times, the url is given by the last valid declaration.
If a namespace rule is inserted in the middle of a bunch of namespace rules, we
might end up using that rule instead of a later one with the same prefix.

https://codereview.chromium.org/1321943002/diff/40001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:284: if (m_childRules.size() > 0)
!m_childRules.isEmpty() probably reads better

ramya.v

https://codereview.chromium.org/1321943002/diff/40001/Source/core/css/StyleSheetContents.cpp File Source/core/css/StyleSheetContents.cpp (right): https://codereview.chromium.org/1321943002/diff/40001/Source/core/css/StyleSheetContents.cpp#newcode254 Source/core/css/StyleSheetContents.cpp:254: parserAddNamespace(namespaceRule->prefix(), namespaceRule->uri()); On 2015/09/04 01:35:50, Timothy Loh wrote: > ...

5 years, 3 months ago (2015-09-07 05:28:23 UTC) #7

https://codereview.chromium.org/1321943002/diff/40001/Source/core/css/StyleSh...
File Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/1321943002/diff/40001/Source/core/css/StyleSh...
Source/core/css/StyleSheetContents.cpp:254:
parserAddNamespace(namespaceRule->prefix(), namespaceRule->uri());
On 2015/09/04 01:35:50, Timothy Loh wrote:
> parserAddNamespace won't do what you want here. If a stylesheet specifies a
> namespace prefix multiple times, the url is given by the last valid
declaration.
> If a namespace rule is inserted in the middle of a bunch of namespace rules,
we
> might end up using that rule instead of a later one with the same prefix.

Hi Tim just one doubt.
As per spec it was given
If a namespace prefix or default namespace is declared more than once only the
last declaration shall be used. Declaring a namespace prefix or default
namespace more than once is nonconforming.

As per your comments what I understood is if there are bunch of namespace rules
with same prefix, the rule which comes last in css rule list should be used.

However firefox and IE are using the rule which is declared later in time frame
even if it is added an index previous to existing namespace rules.

Ex: In the below example as per our understanding div3 with mango should have
background color red, however in Firefox and IE div2 with apple is having
background color red.

<html xmlns="http://www.w3.org/1999/xhtml">

    <style>
    @namespace abc url(banana);
     </style>

     <div id="div1" xmlns="banana"> div1</div>
	 <div id="div3" xmlns="apple">div3</div>
	 <div id="div2" xmlns="mango">div2</div>
    <script>
	document.styleSheets[0].insertRule('@namespace abc url(mango);',
document.styleSheets[0].cssRules.length);
	document.styleSheets[0].insertRule('@namespace abc url(apple);', 0);
	document.styleSheets[0].insertRule('div { color: red;}',
document.styleSheets[0].cssRules.length);
	document.styleSheets[0].insertRule('abc|div { color: green;}',
document.styleSheets[0].cssRules.length);
	</script>
</html>

Can you please let me know if this compatibility difference is fine?
Do I have to confirm the behavior with spec editors?

Timothy Loh

On 2015/09/07 05:28:23, ramya.v wrote: > Hi Tim just one doubt. > As per spec ...

5 years, 3 months ago (2015-09-07 05:32:44 UTC) #8

ramya.v

Updated as per review comments. PTAL! Thanks https://codereview.chromium.org/1321943002/diff/40001/Source/core/css/StyleSheetContents.cpp File Source/core/css/StyleSheetContents.cpp (right): https://codereview.chromium.org/1321943002/diff/40001/Source/core/css/StyleSheetContents.cpp#newcode161 Source/core/css/StyleSheetContents.cpp:161: // import/namespace/charset ...

5 years, 3 months ago (2015-09-07 09:06:28 UTC) #9

Timothy Loh

On 2015/09/07 09:06:28, ramya.v wrote: > Updated as per review comments. > PTAL! > > ...

5 years, 3 months ago (2015-09-08 00:33:39 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1321943002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1321943002/60001

5 years, 3 months ago (2015-09-08 01:19:36 UTC) #12

commit-bot: I haz the power

5 years, 3 months ago (2015-09-08 02:31:11 UTC) #13

Message was sent while issue was closed.

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201884

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages