Bug 31295 – [Qt] Rendering of html select elements is broken

• Component: New Bugs
• Status: RESOLVED FIXED

[Simon Hausmann]

The background is drawn in black.

[Antonio Gomes]

Created attachment 42869 [details]
testcase

test case, open it in QVGLauncher to see the problem

[Jocelyn Turcotte]

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

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

Hi David, do you have any comments on this?

[Jocelyn Turcotte]

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

[Jocelyn Turcotte]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

The other patch is correct.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jocelyn Turcotte]

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

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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

[Simon Hausmann]

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.

[Jesus Sanchez-Palencia]

Created attachment 58775 [details]
revert r49242

Patch for reverting the previous changes if needed.

[Jesus Sanchez-Palencia]

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

[Tor Arne Vestbø]

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.

[Tor Arne Vestbø]

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.

[Tor Arne Vestbø]

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

[Simon Hausmann]

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