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.
Created attachment 74478 [details] Patch
(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.
Created attachment 74480 [details] Patch
(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 on attachment 74480 [details] Patch looks ok
Created attachment 79206 [details] Patch
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 on attachment 79206 [details] Patch ok
(In reply to comment #8) > (From update of attachment 79206 [details]) > ok Landed in http://trac.webkit.org/changeset/76002
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.
Reverted r76002 for reason: breaks a bunch of fast/forms tests on mac and GTK Committed r76006: <http://trac.webkit.org/changeset/76006>
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.
It should be reopened, because it was rolled out: See Comment #11
(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.
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.
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.
(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
setTimeout(foo, 0) is completly valid in layout tests. As long as you use layoutTestController.waitUntilDone() before calling setTimeout() and layoutTestController.notifyDone() afterwards.
(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
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.
(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.
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.
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.
Created attachment 79853 [details] Patch
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'?
Created attachment 80222 [details] Patch
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 on attachment 80222 [details] Patch ok
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
The new test appear to need Windows-specific results: https://bugs.webkit.org/show_bug.cgi?id=53327