Bug 95890

This was in response to feedback from WebKit people asking for it not to saying that it didn't in WebKit, FWIW...

Ah, I see. There's just a FIXME in the code that we should delete. http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp?rev=127593#L1671

rs=me on removing such. :) we first-implementors are just a confused lot, wandering unawares in the darkness of the unknown.

Based on some quick fiddling around with http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1929, it looks like WebKit is already behaving as the spec claims it should. Eric, did you mean "Remove that FIXME, we should totally inherit editability!" If so, I'm happy to pick it up. If not, I think we can close this bug.

Yes, I think we can remove the FIXME.

Created attachment 176434 [details] Patch

Comment on attachment 176434 [details] Patch 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').cssText; > + 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.

Created attachment 176526 [details] Patch

(In reply to comment #7) > (From update of attachment 176434 [details]) > 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). > > 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. Done, thanks for the tip. > Looks like this is a general seamless bug: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1932. Bleh. :(

Comment on attachment 176526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176526&action=review Thanks! > LayoutTests/fast/frames/seamless/seamless-contenteditable-not-inherited.html:10 > + window.span = document.querySelector('div > iframe').contentDocument.querySelector('span'); > + window.p = document.querySelector('body > iframe').contentDocument.querySelector('p'); Nit: This is fine, but I would have just made them globals (e.g. s/window.span/span/).

(In reply to comment #10) > (From update of attachment 176526 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176526&action=review > > Thanks! Thank you! > > LayoutTests/fast/frames/seamless/seamless-contenteditable-not-inherited.html:10 > > + window.span = document.querySelector('div > iframe').contentDocument.querySelector('span'); > > + window.p = document.querySelector('body > iframe').contentDocument.querySelector('p'); > > Nit: This is fine, but I would have just made them globals (e.g. s/window.span/span/). I did it this way for consistency with the other seamless tests, which generally seem to follow this pattern.

Comment on attachment 176526 [details] Patch Clearing flags on attachment: 176526 Committed r136105: <http://trac.webkit.org/changeset/136105>

All reviewed patches have been landed. Closing bug.