Make IFRAME_SEAMLESS child documents inherit styles from their parent iframe element

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Make IFRAME_SEAMLESS child documents inherit styles from their parent iframe element

[Eric Seidel (no email)]

Created attachment 140842 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 140842 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140842&action=review

> Source/WebCore/dom/Document.cpp:1649
> +    // FIXME: In the seamless case, we should likely schedule a style recalc
> +    // on our parent and instead return early here.

Turns out this code is needed for another test case once I land the layout changes.  This change is just about style inheritance.  I'm open to suggestions for further testing of this chagne.

[Ojan Vafai]

Comment on attachment 140842 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140842&action=review

> Source/WebCore/css/StyleResolver.cpp:1542
> +    // FIXME: It's not clear that we want to override all of these values in the seamless case!

In fact, I think almost all of these are wrong for seamless. designMode is an interesting case. Seems like it should inherit editability and we should certainly inherit rtl, locale, zoom, etc.

When the content's of a srcdoc seamless iframe change, does the srcdoc also change? That somewhat effects whether editability should inherit.

In either case, this is probably worth bring up on whatwg@ to get other browser vendors' opinions.

> Source/WebCore/dom/Document.cpp:1734
> +    if ((change >= Detach) || (shouldDisplaySeamlesslyWithParent() && (change >= Inherit))) {

This looks to me like it changes behavior for non-seamless-documents as well. Can you create a test case for that?

[Eric Seidel (no email)]

(In reply to comment #3)
> (From update of attachment 140842 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140842&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:1542
> > +    // FIXME: It's not clear that we want to override all of these values in the seamless case!
> 
> In fact, I think almost all of these are wrong for seamless. designMode is an interesting case. Seems like it should inherit editability and we should certainly inherit rtl, locale, zoom, etc.
> 
> When the content's of a srcdoc seamless iframe change, does the srcdoc also change? That somewhat effects whether editability should inherit.
> 
> In either case, this is probably worth bring up on whatwg@ to get other browser vendors' opinions.

I'd rather test our behavior and someone else can bring it up on the list. :)
 
> > Source/WebCore/dom/Document.cpp:1734
> > +    if ((change >= Detach) || (shouldDisplaySeamlesslyWithParent() && (change >= Inherit))) {
> 
> This looks to me like it changes behavior for non-seamless-documents as well. Can you create a test case for that?

I did.  I can remove that part if you like, but I do not believe it to be observable.  It could be made observable if style recalcs were made to propgate across iframe boundaries in the detach case.

[Eric Seidel (no email)]

(In reply to comment #3)
> (From update of attachment 140842 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140842&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:1542
> > +    // FIXME: It's not clear that we want to override all of these values in the seamless case!
> 
> In fact, I think almost all of these are wrong for seamless. designMode is an interesting case. Seems like it should inherit editability and we should certainly inherit rtl, locale, zoom, etc.

If you have specific suggestions on how you'd like these to behave, I'm happy to implement them.  Otherwise my default would be to leave them as a FIXME for now.

> When the content's of a srcdoc seamless iframe change, does the srcdoc also change? That somewhat effects whether editability should inherit.

No.  srcdoc doesn't change any more than src=data: would. :)

[Eric Seidel (no email)]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 140842 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140842&action=review
> > 
> > > Source/WebCore/css/StyleResolver.cpp:1542
> > > +    // FIXME: It's not clear that we want to override all of these values in the seamless case!
> > 
> > In fact, I think almost all of these are wrong for seamless. designMode is an interesting case. Seems like it should inherit editability and we should certainly inherit rtl, locale, zoom, etc.
> 
> If you have specific suggestions on how you'd like these to behave, I'm happy to implement them.  Otherwise my default would be to leave them as a FIXME for now.

(My reasoning there is that I'm trying not to change iframe behavior unless specified to by the seamless spec.)

[Eric Seidel (no email)]

Created attachment 141028 [details]
Remove == Force -> >= Detach change per Ojan/James' suggestion

[Ojan Vafai]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > (From update of attachment 140842 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=140842&action=review
> > > 
> > > > Source/WebCore/css/StyleResolver.cpp:1542
> > > > +    // FIXME: It's not clear that we want to override all of these values in the seamless case!
> > > 
> > > In fact, I think almost all of these are wrong for seamless. designMode is an interesting case. Seems like it should inherit editability and we should certainly inherit rtl, locale, zoom, etc.
> > 
> > If you have specific suggestions on how you'd like these to behave, I'm happy to implement them.  Otherwise my default would be to leave them as a FIXME for now.
> 
> (My reasoning there is that I'm trying not to change iframe behavior unless specified to by the seamless spec.)

This depends on your interpretation of the spec I suppose, but rtl, userModify (i.e. designMode), zoom, etc are all inherited CSS properties and thus, the spec  says they should inherit, no?

[Eric Seidel (no email)]

(In reply to comment #8)
> > (My reasoning there is that I'm trying not to change iframe behavior unless specified to by the seamless spec.)
> 
> This depends on your interpretation of the spec I suppose, but rtl, userModify (i.e. designMode), zoom, etc are all inherited CSS properties and thus, the spec  says they should inherit, no?

I'm happy to change this method to early-return if that's your recommendation.

http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L1531

As you can see, it sets a bunch of junk on the Document's style.  It's presumably safe/correct to inherit those from the parent document.

I think either approach is viable, but I don't feel I have enough data to make the call which way (so I went with what I believe to be the smaller change).

If you'd like me to early return (thus not overriding these), I'll add a test to make sure we inherit userModify from the parent iframe, as well as are able to set it on the child via document.designMode (like i already have a test for).

Please make sure you've read StyleResolver::styleForDocument.  But I'm happy to implement either direction.

[Eric Seidel (no email)]

I should clarify: the only aspect of "rtl-ness" which is not inherited in the current implementation is -webkit-rtl-ordering:
http://css-infos.net/property/-webkit-rtl-ordering

[Eric Seidel (no email)]

I've filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=17015

[Julien Chaffraix]

Comment on attachment 141028 [details]
Remove == Force -> >= Detach change per Ojan/James' suggestion

View in context: https://bugs.webkit.org/attachment.cgi?id=141028&action=review

> Source/WebCore/css/StyleResolver.cpp:1540
> +    bool seamlessWithParent = document->shouldDisplaySeamlesslyWithParent();
> +    if (seamlessWithParent) {
> +        RenderStyle* iframeStyle = document->seamlessParentIFrame()->renderStyle();
> +        if (iframeStyle)
> +            documentStyle->inheritFrom(iframeStyle);
> +    }

I think I agree with Ojan here that the properties set on the iframe's element should be inherited. What I don't think we have addressed is the issue of those not specified on the iframe's element. Those used to be set below but if we add an early return, we will never set them.

There is likely a good way of handling that involving some UA style sheet here so that we can still get them overriden and match the spec.

This should definitely be tested for documentation's sake.

[Eric Seidel (no email)]

I'm not sure we're making progress here, 24 hours into review. :)

The CSS properties in question are approximately:
-webkit-rtl-ordering
zoom
(page-scale-transform -- not a real CSS property)
-webkit-user-modify
-webkit-locale
writing-mode
direction
-webkit-column-axis
-webkit-column-gap
font

I'm happy to add the early-return and test that these values are inherited from the iframe.

Doing so will break usage of iframe.contentDocument.designMode, as well as writing-mode setting from the documentElement and may change printing for better or worse. :)

My current belief is that a FIXME with no early return is safer, but possibly less correct with the intent of seamless.

Again, happy to do either approach, assuming someone is willing to r+ a patch with either! :)

Please Ojan, Jullien, or (some other rendering reviewer) make a recommendation as to what suits your feelings and I will implement immediately. :)

[Eric Seidel (no email)]

(The patch after this one adds the size-negotation between the content document and the parent document, after that <seamless> will be basically "complete" and ready for our first users to play around with it.  I expect we'll learn a lot from those users.)

[Eric Seidel (no email)]

Re-reading the current prevailing winds, I plan to upload a new patch which early-returns from styleForDocument.  That will (intentionally) break setting of iframe.contentDocument.designMode, but will make seamless content documents inherit all styles from their parent iframe.  I will include a test which verifies that the above-mentioned CSS properites are inherited correctly.

Please squeak now if you disagree with that approach. :)  I remain ambivalent. :)

[Eric Seidel (no email)]

Created attachment 141220 [details]
Updated as discussed

[Ojan Vafai]

Comment on attachment 141220 [details]
Updated as discussed

View in context: https://bugs.webkit.org/attachment.cgi?id=141220&action=review

> Source/WebCore/css/StyleResolver.cpp:1540
> +            // Early-return to avoid overriding inheritted styles.
> +            return documentStyle.release();

This still needs a FIXME. I had said I was OK with the previous patch + FIXME mainly because early returning is also not fully correct. If you set designMode on the iframe's document or writing mode on the documentElement, then we do the wrong thing now.

Making this code do the right thing isn't actually hard. It just requires carefully looking at the code below and applying the right bits. 

I'm fine with the code either way for now as long as there is a clear FIXME.

[Eric Seidel (no email)]

(In reply to comment #17)
> (From update of attachment 141220 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141220&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:1540
> > +            // Early-return to avoid overriding inheritted styles.
> > +            return documentStyle.release();
> 
> This still needs a FIXME. I had said I was OK with the previous patch + FIXME mainly because early returning is also not fully correct. If you set designMode on the iframe's document or writing mode on the documentElement, then we do the wrong thing now.
> 
> Making this code do the right thing isn't actually hard. It just requires carefully looking at the code below and applying the right bits. 
> 
> I'm fine with the code either way for now as long as there is a clear FIXME.

Then I would prefer to land my previous patch. :)

[Eric Seidel (no email)]

Created attachment 141233 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 141233 [details]
Patch for landing

Clearing flags on attachment: 141233

Committed r116694: <http://trac.webkit.org/changeset/116694>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.