Bug 83298 - getMatchedCSSRules() should return null when the second argument is an unrecognized pseudo-element name
Summary: getMatchedCSSRules() should return null when the second argument is an unreco...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 79653
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-05 11:45 PDT by Eric Guzman
Modified: 2012-04-24 20:24 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (5.37 KB, patch)
2012-04-24 11:01 PDT, Kulanthaivel Palanichamy
ojan: review-
ojan: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (7.12 KB, patch)
2012-04-24 15:19 PDT, Kulanthaivel Palanichamy
ojan: review+
Details | Formatted Diff | Diff
Rebased Patch. (7.06 KB, patch)
2012-04-24 16:44 PDT, Kulanthaivel Palanichamy
no flags Details | Formatted Diff | Diff
Rebased again (7.15 KB, patch)
2012-04-24 17:37 PDT, Kulanthaivel Palanichamy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Guzman 2012-04-05 11:45:44 PDT
Method window.getMatchedCSSRules() should return null when the second argument is an unrecognized pseudo-element name. It currently returns the matching rules for the element passed.

Please see Bug 76736 (https://bugs.webkit.org/show_bug.cgi?id=76736), specifically Tab Atkins Jr's second comment.

Thanks!
Comment 1 Tab Atkins 2012-04-05 12:20:49 PDT
Here's a reproduction:

<!DOCTYPE html>
<p>dummy text</p>
<style>
p { color: blue; }
</style>
<script>
function print(text) { 
 document.body.appendChild(document.createTextNode(text));
 document.body.appendChild(document.createElement('br')); 
}
print("For gMCR(el), rules are '" + 
  getMatchedCSSRules(document.querySelector('p'))[0].cssText + "'");
print("For gMCR(el, 'before'), rules are '" + 
  getMatchedCSSRules(document.querySelector('p'), 'before') + "'");
print("For gMCR(el, 'foo'), rules are '" + 
  getMatchedCSSRules(document.querySelector('p'), 'foo')[0].cssText + "'");
</script>
Comment 2 Kulanthaivel Palanichamy 2012-04-24 11:01:17 PDT
Created attachment 138598 [details]
Proposed patch
Comment 3 Ojan Vafai 2012-04-24 11:38:01 PDT
Comment on attachment 138598 [details]
Proposed patch

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

The C++ code looks good. I'm not sure about the idl changes though. Someone more familiar with idl should review that.

With the test, just a few nits. We try to keep as much boilerplate out of the tests as possible. Makes it easier to see what's going on.

Can you provide a link in the ChangeLog description to the appropriate CSS spec or www-style discussion for this behavior?

r- to resolve the above couple issues. Then someone with more idl experience can r+.

> LayoutTests/fast/dom/Window/getMatchedCSSRules-with-invalid-pseudo-elements.html:4
> +<html>
> +<head>
> +<meta charset="utf-8">

You don't need these elements.

> LayoutTests/fast/dom/Window/getMatchedCSSRules-with-invalid-pseudo-elements.html:5
> +<style type="text/css" media="screen">

you don't need the type or media attributes, do you?
Comment 4 Ojan Vafai 2012-04-24 11:38:36 PDT
Adding some people likely to know whether the idl change is correct...
Comment 5 Kentaro Hara 2012-04-24 12:38:23 PDT
Comment on attachment 138598 [details]
Proposed patch

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

> LayoutTests/fast/dom/Window/getMatchedCSSRules-with-invalid-pseudo-elements.html:18
> +shouldBe("getMatchedCSSRules(document.querySelector('p'), 'before')", "null")
> +shouldBe("getMatchedCSSRules(document.querySelector('p'), 'foo')", "null")

Please add test cases for null and undefined in the second argument.

> Source/WebCore/page/DOMWindow.idl:156
> +                                       in [TreatNullAs=NullString, TreatUndefinedAs=NullString,Optional=DefaultIsUndefined] DOMString pseudoElement);

What is the expected behavior for null or undefined in the second argument? Look at here: https://trac.webkit.org/wiki/WebKitIDL#TreatNullAs

We do not specify [TreatNullAs=NullString, TreatUndefinedAs=NullString], unless the spec requires it.
Comment 6 Kulanthaivel Palanichamy 2012-04-24 15:18:21 PDT
(In reply to comment #5)
> (From update of attachment 138598 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138598&action=review
> 
> > LayoutTests/fast/dom/Window/getMatchedCSSRules-with-invalid-pseudo-elements.html:18
> > +shouldBe("getMatchedCSSRules(document.querySelector('p'), 'before')", "null")
> > +shouldBe("getMatchedCSSRules(document.querySelector('p'), 'foo')", "null")
> 
> Please add test cases for null and undefined in the second argument.
> 
> > Source/WebCore/page/DOMWindow.idl:156
> > +                                       in [TreatNullAs=NullString, TreatUndefinedAs=NullString,Optional=DefaultIsUndefined] DOMString pseudoElement);
> 
> What is the expected behavior for null or undefined in the second argument? Look at here: https://trac.webkit.org/wiki/WebKitIDL#TreatNullAs
> 
> We do not specify [TreatNullAs=NullString, TreatUndefinedAs=NullString], unless the spec requires it.
I couldn't find any W3C spec for this API (any pointers?). I think this was originally added for WebInspector, but web developers started using it in the wild lately. In any case it shouldn't be different from window.getComputedStyle() as far as the parameters are concerned.

In the mean time I found that getMatchedCSSRules is not supporting ':' and '::' prefixed pseudo-elements like how getComputedStyle does. My follow-up patch has fixed this issue as well.
Let me know if you suggest separating that into a different patch.
Comment 7 Kulanthaivel Palanichamy 2012-04-24 15:19:27 PDT
Created attachment 138664 [details]
Proposed patch
Comment 8 Kentaro Hara 2012-04-24 15:31:27 PDT
(In reply to comment #6)
> (In reply to comment #5)
> I couldn't find any W3C spec for this API (any pointers?). I think this was originally added for WebInspector, but web developers started using it in the wild lately. In any case it shouldn't be different from window.getComputedStyle() as far as the parameters are concerned.
> 
> In the mean time I found that getMatchedCSSRules is not supporting ':' and '::' prefixed pseudo-elements like how getComputedStyle does. My follow-up patch has fixed this issue as well.
> Let me know if you suggest separating that into a different patch.

r+ for the IDL part, given that

- getMatchedCSSRules() should match getComputedStyle()
- getComputedStyle() already uses [TreatNullAs=NullString, TreatUndefinedAs=NullString]
Comment 9 Ojan Vafai 2012-04-24 16:03:21 PDT
Looks like we're planning on killing this API entirely. See bug 79653.
Comment 10 Ojan Vafai 2012-04-24 16:05:35 PDT
Comment on attachment 138664 [details]
Proposed patch

Not sure we should commit more patches for an API we're planning to kill. That said, this code is all isolated in code that will naturally go away when we kill the API. So, in the meantime, may as well fix this on the off-chance that we can't kill it.

It looks like this patch doesn't applies cleanly to DOMWindow.cpp. Please sync and upload a new patch. Make sure it passes on the EWS bots before committing.
Comment 11 Kulanthaivel Palanichamy 2012-04-24 16:44:19 PDT
Created attachment 138687 [details]
Rebased Patch.
Comment 12 Kulanthaivel Palanichamy 2012-04-24 17:37:23 PDT
Created attachment 138702 [details]
Rebased again
Comment 13 WebKit Review Bot 2012-04-24 20:24:31 PDT
Comment on attachment 138702 [details]
Rebased again

Clearing flags on attachment: 138702

Committed r115164: <http://trac.webkit.org/changeset/115164>
Comment 14 WebKit Review Bot 2012-04-24 20:24:36 PDT
All reviewed patches have been landed.  Closing bug.