Created attachment 42869 [details] testcase test case, open it in QVGLauncher to see the problem

Created attachment 42870 [details] Patch, hardcode the color in the constructor

Doesn't seem right. It seems more right that the method doing the painting would take care of that. No color was defined, thus the default color is invalid right?

Hi David, do you have any comments on this?

To copy here what we talked on irc, it all depend if the invalid flag in the background is intended to mean "don't draw" or not. This flag don't seem to be verified in different GraphicContext implementations. Currently this cause problems since we don't check this and Qt has an undefined behavior when painting with a brush that has an invalid color (the default invalid color is black, and it gets painted anyway).

Created attachment 42879 [details] Patch, use a RenderStyle static init function This patch uses an initialization function instead of hardcoding the value in the constructor. This seems to be done in both way and I don't know which style is prefered. Pick your favorite if the behavior is satisfying.

Comment on attachment 42879 [details] Patch, use a RenderStyle static init function Seems we should set m_outline to the initial outline as well. This patch looks correct though. If you want it auto-committed you should set commit-queue=? as well.

Comment on attachment 42870 [details] Patch, hardcode the color in the constructor The other patch is correct.

Comment on attachment 42879 [details] Patch, use a RenderStyle static init function Clearing flags on attachment: 42879 Committed r50796: <http://trac.webkit.org/changeset/50796>

All reviewed patches have been landed. Closing bug.

(In reply to comment #7) > Seems we should set m_outline to the initial outline as well. This patch looks I had a look at m_outline initialization. This members object take care of more than one style property and their initialization seems to be scattered around. Nothing is broken so I will let it as it is.

<http://trac.webkit.org/changeset/50796> caused a regression affecting WebKit clients on Mac OS X (because now an NSAttributedString created from a DOM range has the background color attribute (set to transparent), whereas before it didn’t). I would like to revert r50796, but not break Qt. One way to do so would be to also revert <http://trac.webkit.org/changeset/49242>, but its change log gives no motivation. Can someone explain why r49242 was needed? Thanks!

(In reply to comment #12) > <http://trac.webkit.org/changeset/50796> caused a regression affecting WebKit clients on Mac OS X (because now an NSAttributedString created from a DOM range has the background color attribute (set to transparent), whereas before it didn’t). I would like to revert r50796, but not break Qt. One way to do so would be to also revert <http://trac.webkit.org/changeset/49242>, but its change log gives no motivation. Can someone explain why r49242 was needed? Thanks! I do not remember why that change was needed. Unfortunately that was long ago. But on one hand it seems strange that a color has a m_valid, and uses the color even if m_valid is false.

(In reply to comment #13) > (In reply to comment #12) > > <http://trac.webkit.org/changeset/50796> caused a regression affecting WebKit clients on Mac OS X (because now an NSAttributedString created from a DOM range has the background color attribute (set to transparent), whereas before it didn’t). I would like to revert r50796, but not break Qt. One way to do so would be to also revert <http://trac.webkit.org/changeset/49242>, but its change log gives no motivation. Can someone explain why r49242 was needed? Thanks! > > I do not remember why that change was needed. Unfortunately that was long ago. > > But on one hand it seems strange that a color has a m_valid, and uses the color even if m_valid is false. The invalid color is conveniently transparent, so in many contexts you can just use it without checking for validity.

> The invalid color is conveniently transparent, so in many contexts you can just use it without checking for validity. Then I suggest that we remove my original patch and leave this comment instead.

I believe https://trac.webkit.org/changeset/58949 makes the earlier changes obsolete and safe to back out. Tor Arne, what do you think?

Thanks for your help, Kenneth and Simon! After thinking about this some more and talking to Darin, I’ve decided to address the issue for Mac OS X clients by making a change in WebKit/mac rather than reverting the RenderStyle changes.

Created attachment 58775 [details] revert r49242 Patch for reverting the previous changes if needed.

(In reply to comment #17) > Thanks for your help, Kenneth and Simon! > > After thinking about this some more and talking to Darin, I’ve decided to address the issue for Mac OS X clients by making a change in WebKit/mac rather than reverting the RenderStyle changes. I've attached a patch as Kenneth suggested, but maybe it's too late, right?! :) I didn't mark it for review as it might not be needed anymore.

r58949 should make it safe to pass invalid colors down to the Qt graphics context, as we won't draw anything in that case, so 50796 can be reverted, but we still have to keep 49242 since the QColor(r,g,b,a) constructor will create a valid QColor.

Comment on attachment 58775 [details] revert r49242 > + // The invalid WebCore::Color is conveniently transparent, so in many contexts you can just use it without checking for validity. > + return QColor(red(), green(), blue(), alpha()); This is wrong, in the Qt context is is not transparent, so we cant use it without checking for validity, we have to keep the check to make the guards from r58949 catch it.

(In reply to comment #17) > Thanks for your help, Kenneth and Simon! > > After thinking about this some more and talking to Darin, I’ve decided to address the issue for Mac OS X clients by making a change in WebKit/mac rather than reverting the RenderStyle changes. Feel free to go the original way of reverting r50796

(In reply to comment #22) > (In reply to comment #17) > > Thanks for your help, Kenneth and Simon! > > > > After thinking about this some more and talking to Darin, I’ve decided to address the issue for Mac OS X clients by making a change in WebKit/mac rather than reverting the RenderStyle changes. > > Feel free to go the original way of reverting r50796 No need for that after <http://trac.webkit.org/changeset/61189>. Thanks again for your help!

Closing again after http://trac.webkit.org/changeset/61189