Bug 95890 – seamless iframes should not inherit editability

• Component: Frames
• Status: RESOLVED FIXED

[Ojan Vafai]

http://html5.org/tools/web-apps-tracker?from=7318&to=7319

[Ian 'Hixie' Hickson]

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

[Ojan Vafai]

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

[Eric Seidel (no email)]

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

[Mike West]

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.

[Ojan Vafai]

Yes, I think we can remove the FIXME.

[Mike West]

Created attachment 176434 [details]
Patch

[Ojan Vafai]

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.

[Mike West]

Created attachment 176526 [details]
Patch

[Mike West]

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

[Ojan Vafai]

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

[Mike West]

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

[WebKit Review Bot]

Comment on attachment 176526 [details]
Patch

Clearing flags on attachment: 176526

Committed r136105: <http://trac.webkit.org/changeset/136105>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.