[webkit-reviews] review denied: [Bug 95890] seamless iframes should not inherit editability : [Attachment 176434] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Mike West <mkwst at chromium.org>'s
request for review:
Bug 95890: seamless iframes should not inherit editability
https://bugs.webkit.org/show_bug.cgi?id=95890

Attachment 176434: Patch
https://bugs.webkit.org/attachment.cgi?id=176434&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176434&action=review


Removing the FIXME is still correct since this only affects the document, but
please update the test case to use js-test-pre and to fail in the second case
(it's ok to check in tests that say FAIL if they are exposing a bug).

>
LayoutTests/fast/frames/seamless/seamless-contenteditable-not-inherited.html:22

> +	   if (window.testRunner)
> +	       testRunner.dumpAsText();
> +
> +	   function test() {
> +	       var span = document.querySelector('div >
iframe').contentWindow.document.querySelector('span');
> +	       var spanEditable =
window.getComputedStyle(span).getPropertyCSSValue('-webkit-user-modify').cssTex
t;
> +	       if (spanEditable == 'read-only')
> +		   console.log("PASS: An element inside a seamless iframe
inside a contenteditable element does not inherit editability.");
> +	       else
> +		   console.log("FAIL: An element inside a seamless iframe
inside a contenteditable element should not inherit editability.");
> +
> +	       var p = document.querySelector('body >
iframe').contentWindow.document.querySelector('p');
> +	       var pEditable =
window.getComputedStyle(p).getPropertyCSSValue('-webkit-user-modify').cssText;
> +	       if (pEditable == 'read-only')
> +		   console.log("PASS: An element inside a seamless iframe does
not inherit editability via CSS cascade.");
> +	       else
> +		   console.log("FAIL: An element inside a seamless iframe
should not inherit editability via CSS cascade.");
> +	   }

Please use js-test-pre.js for this. It makes for more developer friendly error
messages when there are failures and would be less code.

>
LayoutTests/fast/frames/seamless/seamless-contenteditable-not-inherited.html:36

> +    <iframe seamless srcdoc="<p>This paragraph is not
editable.</p>"></iframe>

Actually, I think this paragraph should be editable! The spec just says
contentEditable doesn't inherit. It seems like all CSS styling should. In fact,
it some cases it seems like it does. I think this is exposing a bug in the
seamless implementation.

Looks like this is a general seamless bug:
http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1932.