83298 – getMatchedCSSRules() should return null when the second argument is an unrecognized pseudo-element name

RESOLVED FIXED Bug 83298

getMatchedCSSRules() should return null when the second argument is an unrecognized pseudo-element name

https://bugs.webkit.org/show_bug.cgi?id=83298

Summary getMatchedCSSRules() should return null when the second argument is an unreco...

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-	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
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Nobody

Reported

2012-04-05 11:45 PDT

Modified

2012-04-24 20:24 PDT History

CC List

7 users Show

URL

Keywords

Depends on

79653

Blocks

Dependancies

tree graph