Bug 49790

Summary:

Color changes to option elements in a select multiple aren't drawn immediately

Product:

WebKit

Reporter:

Robert Sinton <robert-wkbugz>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, darin, eric, jberlin, krit, mitz, ossy, pnormand, rwlbuis, tkent, webkit.review.bot, zimmermann

Priority:

Version:

525.x (Safari 3.2)

Hardware:

Mac (Intel)

OS:

OS X 10.6

Attachments:

Description	Flags
Reduced example	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	tkent: review+

Robert Sinton

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

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Rob Buis

Comment 1 2010-11-20 04:10:43 PST

Created attachment 74478 [details] Patch

Rob Buis

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

Rob Buis

Comment 3 2010-11-20 07:55:29 PST

Created attachment 74480 [details] Patch

Rob Buis

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

Kent Tamura

Comment 5 2011-01-13 04:10:12 PST

Comment on attachment 74480 [details] Patch looks ok

Rob Buis

Comment 6 2011-01-17 12:43:53 PST

Created attachment 79206 [details] Patch

Rob Buis

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

Kent Tamura

Comment 8 2011-01-17 17:49:15 PST

Comment on attachment 79206 [details] Patch ok

Csaba Osztrogonác

Comment 9 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

Csaba Osztrogonác

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

Philippe Normand

Comment 11 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>

Rob Buis

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

Csaba Osztrogonác

Comment 13 2011-01-18 01:34:19 PST

It should be reopened, because it was rolled out: See Comment #11

Dirk Schulze

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

Rob Buis

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

Philippe Normand

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

Philippe Normand

Comment 17 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

Nikolas Zimmermann

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

Nikolas Zimmermann

Comment 19 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

Rob Buis

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

Csaba Osztrogonác

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

mitz

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

Rob Buis

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

Rob Buis

Comment 24 2011-01-22 12:40:43 PST

Created attachment 79853 [details] Patch

Kent Tamura

Comment 25 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'?

Rob Buis

Comment 26 2011-01-26 12:20:15 PST

Created attachment 80222 [details] Patch

Rob Buis

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

Kent Tamura

Comment 28 2011-01-26 14:10:44 PST

Comment on attachment 80222 [details] Patch ok

WebKit Review Bot

Comment 29 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

Jessie Berlin

Comment 30 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

Note You need to log in before you can comment on or make changes to this bug.