Bug 49790 - Color changes to option elements in a select multiple aren't drawn immediately
Summary: Color changes to option elements in a select multiple aren't drawn immediately
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 525.x (Safari 3.2)
Hardware: Macintosh Intel OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-19 02:12 PST by Robert Sinton
Modified: 2011-06-18 11:37 PDT (History)
12 users (show)

See Also:


Attachments
Reduced example (739 bytes, text/html)
2010-11-19 02:12 PST, Robert Sinton
no flags Details
Patch (25.53 KB, patch)
2010-11-20 04:10 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (37.06 KB, patch)
2010-11-20 07:55 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (37.19 KB, patch)
2011-01-17 12:43 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (43.25 KB, patch)
2011-01-22 12:40 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (15.90 KB, patch)
2011-01-26 12:20 PST, Rob Buis
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Sinton 2010-11-19 02:12:57 PST
Created attachment 74363 [details]
Reduced example

See attached reduction. Observed and reduced in Safari 5.0.2 / wk 533.18.1, but first observed some time ago inside FileMaker web viewers.

When applying color changes via JavaScript to an <option> element inside a <select multiple="multiple">, the element doesn't change appearance until something else forces a refresh. Examples are bringing another window or app to the front, zooming the window, or changing the size of an element.

Text color and background color changes are both affected. Reduction shows the same color changes working as expected for <ul> <li> elements for comparison.

Real-world case was 'graying out' options by toggling CSS class on the options, but was reduced to direct color changes on the elements.
Comment 1 Rob Buis 2010-11-20 04:10:43 PST
Created attachment 74478 [details]
Patch
Comment 2 Rob Buis 2010-11-20 04:14:03 PST
(In reply to comment #1)
> Created an attachment (id=74478) [details]
> Patch

This patch is the best I could come up with. I would rather avoid the pixel test
but I dont think it is possible, since the bug is about the question whether the
repaint is done, not whether the style is set in the first place.
Cheers,

Rob.
Comment 3 Rob Buis 2010-11-20 07:55:29 PST
Created attachment 74480 [details]
Patch
Comment 4 Rob Buis 2010-11-20 07:58:02 PST
(In reply to comment #3)
> Created an attachment (id=74480) [details]
> Patch

I figured layouting is not really needed, since RenderListBox only cares about color,
backgroundColor and visibility properties of the options. I think it is safe to do the repaint in setRenderStyle since it means we are in recalcStyle of the <option> element and something in the style changed. For visibility property I added a new test.
Cheers,

Rob.
Comment 5 Kent Tamura 2011-01-13 04:10:12 PST
Comment on attachment 74480 [details]
Patch

looks ok
Comment 6 Rob Buis 2011-01-17 12:43:53 PST
Created attachment 79206 [details]
Patch
Comment 7 Rob Buis 2011-01-17 12:46:07 PST
Unfortunately just before landing I discovered the testcases did not test the problem at all. I am not sure whether that was also the case back when I created the testcases, but anyway the tests are fixed now
and do test the problem now. But I think I need another positive review before landing, so I uploaded the new patch....
Cheers,

Rob.
Comment 8 Kent Tamura 2011-01-17 17:49:15 PST
Comment on attachment 79206 [details]
Patch

ok
Comment 9 Csaba Osztrogonác 2011-01-18 01:07:55 PST
(In reply to comment #8)
> (From update of attachment 79206 [details])
> ok

Landed in http://trac.webkit.org/changeset/76002
Comment 10 Csaba Osztrogonác 2011-01-18 01:14:01 PST
I tested the new tests introduced in r76002, but unfortunately 
they fail in Qt-DRT, but pass with QtTestBrowser. 

-<body onload="setTimeout(test, 0)">
+<body onload="test()">

If I change setTimeout(test, 0) to test() all tests pass. Why do we 
need this timeout construction? We usually call the test() directly.
Comment 11 Philippe Normand 2011-01-18 01:25:12 PST
Reverted r76002 for reason:

breaks a bunch of fast/forms tests on mac and GTK

Committed r76006: <http://trac.webkit.org/changeset/76006>
Comment 12 Rob Buis 2011-01-18 01:30:33 PST
Hi,

(In reply to comment #10)
> I tested the new tests introduced in r76002, but unfortunately 
> they fail in Qt-DRT, but pass with QtTestBrowser. 
> 
> -<body onload="setTimeout(test, 0)">
> +<body onload="test()">
> 
> If I change setTimeout(test, 0) to test() all tests pass. Why do we 
> need this timeout construction? We usually call the test() directly.

The reduced example is actually one where pressing the link fails to show new
rendering of the option. So I tried to emulate this using the testcase, maybe just using the onload to calltest directly is too early and the option rendering
is part of the overall document rendering, it should really be done afterwards. So I had hoped the setTimeout would solve that, but maybe it doesn't work in all cases. I see two possible fixes, increase the value in setTimeout, or introduce a link that we can click on from javascript. I won't have time for this until late tonight, so if you want to experiment with that, I wouldn't mind :)
Cheers,

Rob.
Comment 13 Csaba Osztrogonác 2011-01-18 01:34:19 PST
It should be reopened, because it was rolled out: See Comment #11
Comment 14 Dirk Schulze 2011-01-18 01:35:30 PST
(In reply to comment #10)
> I tested the new tests introduced in r76002, but unfortunately 
> they fail in Qt-DRT, but pass with QtTestBrowser. 
> 
> -<body onload="setTimeout(test, 0)">
> +<body onload="test()">
> 
> If I change setTimeout(test, 0) to test() all tests pass. Why do we 
> need this timeout construction? We usually call the test() directly.

Ossy is right, no setTimeout's in LayoutTests.
Comment 15 Rob Buis 2011-01-18 01:39:35 PST
Hi Dirk,

(In reply to comment #14)
> (In reply to comment #10)
> > I tested the new tests introduced in r76002, but unfortunately 
> > they fail in Qt-DRT, but pass with QtTestBrowser. 
> > 
> > -<body onload="setTimeout(test, 0)">
> > +<body onload="test()">
> > 
> > If I change setTimeout(test, 0) to test() all tests pass. Why do we 
> > need this timeout construction? We usually call the test() directly.
> 
> Ossy is right, no setTimeout's in LayoutTests.

You learn something new every day :)
Sorry for the mess, all! Will have a look tonight, right now I can only think of the simulated link pressing as a fix, but maybe the other fast/forms tests will show another way.
Cheers,

Rob.
Comment 16 Philippe Normand 2011-01-18 01:45:38 PST
Sorry it seems I rolled out this patch based on a wrong analysis of the failure on GTK. I will investigate the issue some more.
Comment 17 Philippe Normand 2011-01-18 03:00:02 PST
(In reply to comment #16)
> Sorry it seems I rolled out this patch based on a wrong analysis of the failure on GTK. I will investigate the issue some more.

Turns out the massive GTK tests failure was due to Hajime's RoundedIntRect patch (r76004) which he fixed now (r76011).

The patch I rolled out could probably be landed again. Sorry for the trouble
Comment 18 Nikolas Zimmermann 2011-01-18 04:36:14 PST
setTimeout(foo, 0) is completly valid in layout tests. As long as you use
layoutTestController.waitUntilDone() before calling setTimeout() and layoutTestController.notifyDone() afterwards.
Comment 19 Nikolas Zimmermann 2011-01-18 04:36:32 PST
(In reply to comment #18)
> setTimeout(foo, 0) is completly valid in layout tests. As long as you use
> layoutTestController.waitUntilDone() before calling setTimeout() and layoutTestController.notifyDone() afterwards.

To clarify, afterwards == in the foo() function
Comment 20 Rob Buis 2011-01-19 12:50:22 PST
Hello Ossy,

(In reply to comment #10)
> I tested the new tests introduced in r76002, but unfortunately 
> they fail in Qt-DRT, but pass with QtTestBrowser. 
> 
> -<body onload="setTimeout(test, 0)">
> +<body onload="test()">
> 
> If I change setTimeout(test, 0) to test() all tests pass. Why do we 
> need this timeout construction? We usually call the test() directly.

Did you try increasing the timeout value? I tried both simulating pressing a link and
directly calling test(), but both fail to reproduce the problem on OS X.
Cheers,

Rob.
Comment 21 Csaba Osztrogonác 2011-01-19 13:10:42 PST
(In reply to comment #20)
> Did you try increasing the timeout value? I tried both simulating pressing a link and
> directly calling test(), but both fail to reproduce the problem on OS X.
> Cheers,

I tried increasing the timeout, but unfortunately it didn't help.
Comment 22 mitz 2011-01-19 13:42:32 PST
Seems like you could use layoutTestController.display() using the framework used by repaint tests. See any test in fast/repaint that uses fast/repaint/resources/repaint.js.
Comment 23 Rob Buis 2011-01-19 13:44:32 PST
Hello Mitz,

(In reply to comment #22)
> Seems like you could use layoutTestController.display() using the framework used by repaint tests. See any test in fast/repaint that uses fast/repaint/resources/repaint.js.

Thanks a lot! I was kind of stuck... I'll try that approach tomorrow.
Cheers,

Rob.
Comment 24 Rob Buis 2011-01-22 12:40:43 PST
Created attachment 79853 [details]
Patch
Comment 25 Kent Tamura 2011-01-26 05:56:31 PST
Comment on attachment 79853 [details]
Patch

This looks ok.

However,
 - We don't need three tests.
 - The test target <select> should have two or more <option>s
 - What happens if .style.display = 'none'?
Comment 26 Rob Buis 2011-01-26 12:20:15 PST
Created attachment 80222 [details]
Patch
Comment 27 Rob Buis 2011-01-26 12:26:40 PST
Hi Kent,

Maybe I am too cautious, but I thought I'd post the new patch firdt, it addresses your
points below:

(In reply to comment #25)
> (From update of attachment 79853 [details])
> This looks ok.
> 
> However,
>  - We don't need three tests.

Right, I chose one.

>  - The test target <select> should have two or more <option>s

I decided to remove the multiple part, we don't need more than one option element for
testing this bug, it was just there because the reduced testcase had it basically.

>  - What happens if .style.display = 'none'?

It doesn't update at all! In FF it removes the option element from the list, which sounds
reasonable to me. However IMHO this is a different bug, and there may already be a bug
on it, I didn't check that.
Cheers,

Rob.
Comment 28 Kent Tamura 2011-01-26 14:10:44 PST
Comment on attachment 80222 [details]
Patch

ok
Comment 29 WebKit Review Bot 2011-01-27 12:36:07 PST
http://trac.webkit.org/changeset/76826 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
accessibility/crashing-a-tag-in-map.html
css1/box_properties/clear.html
css1/box_properties/float.html
css1/box_properties/height.html
css1/box_properties/width.html
css1/formatting_model/height_of_lines.html
css1/formatting_model/replaced_elements.html
css1/text_properties/vertical_align.html
css2.1/t0803-c5501-imrgn-t-00-b-ag.html
css2.1/t0803-c5502-mrgn-r-00-c-ag.html
css2.1/t0803-c5503-imrgn-b-00-b-a.html
css2.1/t0803-c5504-mrgn-l-00-c-ag.html
css2.1/t0803-c5505-mrgn-00-b-ag.html
css2.1/t0803-c5505-mrgn-03-c-ag.html
css2.1/t0804-c5509-padn-l-03-f-g.html
css2.1/t0804-c5510-padn-00-b-ag.html
css2.1/t0905-c414-flt-01-d-g.html
css2.1/t0905-c414-flt-wrap-01-d-g.html
css2.1/t0905-c5525-fltclr-00-c-ag.html
css2.1/t0905-c5525-fltmrgn-00-c-ag.html
Comment 30 Jessie Berlin 2011-01-28 13:01:36 PST
The new test appear to need Windows-specific results:
https://bugs.webkit.org/show_bug.cgi?id=53327