Bug 31295 - [Qt] Rendering of html select elements is broken
Summary: [Qt] Rendering of html select elements is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P3 Normal
Assignee: Jocelyn Turcotte
URL:
Keywords: Qt
Depends on:
Blocks: Qt46
  Show dependency treegraph
 
Reported: 2009-11-10 08:08 PST by Simon Hausmann
Modified: 2010-06-16 06:17 PDT (History)
10 users (show)

See Also:


Attachments
testcase (366 bytes, text/html)
2009-11-10 09:29 PST, Antonio Gomes
no flags Details
Patch, hardcode the color in the constructor (1.12 KB, patch)
2009-11-10 09:37 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch, use a RenderStyle static init function (1.96 KB, patch)
2009-11-10 11:05 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
revert r49242 (1.98 KB, patch)
2010-06-15 07:35 PDT, Jesus Sanchez-Palencia
vestbo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2009-11-10 08:08:01 PST
The background is drawn in black.
Comment 1 Antonio Gomes 2009-11-10 09:29:41 PST
Created attachment 42869 [details]
testcase

test case, open it in QVGLauncher to see the problem
Comment 2 Jocelyn Turcotte 2009-11-10 09:37:37 PST
Created attachment 42870 [details]
Patch, hardcode the color in the constructor
Comment 3 Kenneth Rohde Christiansen 2009-11-10 10:30:53 PST
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?
Comment 4 Kenneth Rohde Christiansen 2009-11-10 10:33:22 PST
Hi David, do you have any comments on this?
Comment 5 Jocelyn Turcotte 2009-11-10 10:58:16 PST
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).
Comment 6 Jocelyn Turcotte 2009-11-10 11:05:20 PST
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 7 Eric Seidel (no email) 2009-11-10 12:47:40 PST
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 8 Eric Seidel (no email) 2009-11-10 12:48:03 PST
Comment on attachment 42870 [details]
Patch, hardcode the color in the constructor

The other patch is correct.
Comment 9 WebKit Commit Bot 2009-11-11 01:38:46 PST
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>
Comment 10 WebKit Commit Bot 2009-11-11 01:38:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Jocelyn Turcotte 2009-11-11 01:57:59 PST
(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.
Comment 12 mitz 2010-06-13 19:57:05 PDT
<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!
Comment 13 Kenneth Rohde Christiansen 2010-06-14 14:36:53 PDT
(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.
Comment 14 mitz 2010-06-14 14:41:42 PDT
(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.
Comment 15 Kenneth Rohde Christiansen 2010-06-14 14:48:21 PDT
> 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.
Comment 16 Simon Hausmann 2010-06-15 06:56:26 PDT
I believe https://trac.webkit.org/changeset/58949 makes the earlier changes obsolete and safe to back out. Tor Arne, what do you think?
Comment 17 mitz 2010-06-15 07:16:25 PDT
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.
Comment 18 Jesus Sanchez-Palencia 2010-06-15 07:35:55 PDT
Created attachment 58775 [details]
revert r49242

Patch for reverting the previous changes if needed.
Comment 19 Jesus Sanchez-Palencia 2010-06-15 07:37:31 PDT
(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.
Comment 20 Tor Arne Vestbø 2010-06-16 02:03:42 PDT
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 21 Tor Arne Vestbø 2010-06-16 02:05:11 PDT
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.
Comment 22 Tor Arne Vestbø 2010-06-16 02:08:09 PDT
(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
Comment 23 mitz 2010-06-16 02:17:00 PDT
(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!
Comment 24 Simon Hausmann 2010-06-16 06:17:03 PDT
Closing again after http://trac.webkit.org/changeset/61189