WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
49790
Color changes to option elements in a select multiple aren't drawn immediately
https://bugs.webkit.org/show_bug.cgi?id=49790
Summary
Color changes to option elements in a select multiple aren't drawn immediately
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.
Top of Page
Format For Printing
XML
Clone This Bug