Bug 83298

Summary: getMatchedCSSRules() should return null when the second argument is an unrecognized pseudo-element name
Product: WebKit Reporter: Eric Guzman <epiceric24>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, haraken, kulanthaivel, ojan, tabatkins, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 79653    
Bug Blocks:    
Attachments:
Description Flags
Proposed patch
ojan: review-, ojan: commit-queue-
Proposed patch
ojan: review+
Rebased Patch.
none
Rebased again none

Eric Guzman
Reported 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!
Attachments
Proposed patch (5.37 KB, patch)
2012-04-24 11:01 PDT, Kulanthaivel Palanichamy 		ojan: review-
ojan: commit-queue-
DetailsFormatted DiffDiff
Proposed patch (7.12 KB, patch)
2012-04-24 15:19 PDT, Kulanthaivel Palanichamy 		ojan: review+
DetailsFormatted DiffDiff
Rebased Patch. (7.06 KB, patch)
2012-04-24 16:44 PDT, Kulanthaivel Palanichamy 		no flags
DetailsFormatted DiffDiff
Rebased again (7.15 KB, patch)
2012-04-24 17:37 PDT, Kulanthaivel Palanichamy 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Tab Atkins
Comment 1 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>
Kulanthaivel Palanichamy
Comment 2 2012-04-24 11:01:17 PDT
Created attachment 138598 [details] Proposed patch
Ojan Vafai
Comment 3 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?
Ojan Vafai
Comment 4 2012-04-24 11:38:36 PDT
Adding some people likely to know whether the idl change is correct...
Kentaro Hara
Comment 5 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.
Kulanthaivel Palanichamy
Comment 6 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.
Kulanthaivel Palanichamy
Comment 7 2012-04-24 15:19:27 PDT
Created attachment 138664 [details] Proposed patch
Kentaro Hara
Comment 8 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]
Ojan Vafai
Comment 9 2012-04-24 16:03:21 PDT
Looks like we're planning on killing this API entirely. See bug 79653.
Ojan Vafai
Comment 10 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.
Kulanthaivel Palanichamy
Comment 11 2012-04-24 16:44:19 PDT
Created attachment 138687 [details] Rebased Patch.
Kulanthaivel Palanichamy
Comment 12 2012-04-24 17:37:23 PDT
Created attachment 138702 [details] Rebased again
WebKit Review Bot
Comment 13 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>
WebKit Review Bot
Comment 14 2012-04-24 20:24:36 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.