RESOLVED FIXED 55323
REGRESSION (r79762): Items in <select multiple> have focus rings, but shouldn't
https://bugs.webkit.org/show_bug.cgi?id=55323
Summary REGRESSION (r79762): Items in <select multiple> have focus rings, but shouldn't
Adam Roben (:aroben)
Reported 2011-02-27 10:11:39 PST
To reproduce: 1. Go to data:text/html,<select multiple><option>a<option>b 2. Click on "a" A focus ring is drawn around the selected item. This didn't used to happen. It seems extremely likely that r79762 is the cause of this new behavior.
Attachments
fix patch (27.67 KB, patch)
2011-02-28 10:56 PST, Chang Shu
no flags
addtional patch that adds a new test case (25.94 KB, patch)
2011-02-28 13:14 PST, Chang Shu
no flags
fix patch: gtk expected results (29.68 KB, patch)
2011-03-01 10:19 PST, Chang Shu
no flags
Adam Roben (:aroben)
Comment 1 2011-02-27 10:18:37 PST
Chang Shu
Comment 2 2011-02-27 11:16:10 PST
Sorry, I will work on it.
Chang Shu
Comment 3 2011-02-28 10:56:35 PST
Created attachment 84084 [details] fix patch
Antonio Gomes
Comment 4 2011-02-28 12:09:52 PST
Comment on attachment 84084 [details] fix patch View in context: https://bugs.webkit.org/attachment.cgi?id=84084&action=review > LayoutTests/fast/forms/select-listbox-multiple-no-focusring.html:17 > + document.write("To manually test, click on the 'a' option and no focus ring on the selected item should be seen."); I would also had added a test with spatial navigation on, so we would garantee both behaviors.
WebKit Commit Bot
Comment 5 2011-02-28 12:52:02 PST
Comment on attachment 84084 [details] fix patch Clearing flags on attachment: 84084 Committed r79900: <http://trac.webkit.org/changeset/79900>
Chang Shu
Comment 6 2011-02-28 13:14:40 PST
Created attachment 84106 [details] addtional patch that adds a new test case this patch draws a focus ring with snav enabled.
Chang Shu
Comment 7 2011-02-28 13:25:57 PST
Ossy, do you mind generate expected results on Qt as a followup after my patches are landed?
Antonio Gomes
Comment 8 2011-02-28 13:36:44 PST
(In reply to comment #7) > Ossy, do you mind generate expected results on Qt as a followup after my patches are landed? And Martin, could you help with gtk results?
Csaba Osztrogonác
Comment 9 2011-02-28 13:49:40 PST
(In reply to comment #7) > Ossy, do you mind generate expected results on Qt as a followup after my patches are landed? Sure.
Martin Robinson
Comment 10 2011-02-28 13:52:42 PST
Sure, I'll make sure the GTK+ results are added.
WebKit Commit Bot
Comment 11 2011-02-28 15:33:44 PST
Comment on attachment 84106 [details] addtional patch that adds a new test case Clearing flags on attachment: 84106 Committed r79934: <http://trac.webkit.org/changeset/79934>
Ryosuke Niwa
Comment 12 2011-03-01 01:05:00 PST
The test added in this patch has been crashing on GTK. Could you investigate it?
Chang Shu
Comment 13 2011-03-01 04:18:27 PST
(In reply to comment #12) > The test added in this patch has been crashing on GTK. Could you investigate it? What?! Yes, I will take a look right away.
Chang Shu
Comment 14 2011-03-01 06:49:11 PST
I wasn't able to build gtk on my ubuntu. I tried to follow the instruction on this page: https://trac.webkit.org/wiki/BuildingGtk. But the first step failed: [cshu@webkit.gtk] sudo apt-add-repository ppa:martin-james-robinson/webkitgtk Error reading https://launchpad.net/api/1.0/~martin-james-robinson/+archive/webkitgtk: <urlopen error [Errno 110] Connection timed out>
Martin Robinson
Comment 15 2011-03-01 08:30:29 PST
(In reply to comment #14) > I wasn't able to build gtk on my ubuntu. I tried to follow the instruction on this page: https://trac.webkit.org/wiki/BuildingGtk. But the first step failed: > [cshu@webkit.gtk] sudo apt-add-repository ppa:martin-james-robinson/webkitgtk > Error reading https://launchpad.net/api/1.0/~martin-james-robinson/+archive/webkitgtk: <urlopen error [Errno 110] Connection timed out> That's odd. Are you using Maverick? Are you sure the tests crashing on GTK+ are ones added in this patch? We have a test crashing with a very similar name. It look like those failures are tracked by this bug: https://bugs.webkit.org/show_bug.cgi?id=53146
Chang Shu
Comment 16 2011-03-01 10:10:30 PST
> > [cshu@webkit.gtk] sudo apt-add-repository ppa:martin-james-robinson/webkitgtk > > Error reading https://launchpad.net/api/1.0/~martin-james-robinson/+archive/webkitgtk: <urlopen error [Errno 110] Connection timed out> > > That's odd. Are you using Maverick? It was the proxy setting issues. I manually added the repository and it worked. > Are you sure the tests crashing on GTK+ are ones added in this patch? We have a test crashing with a very similar name. It look like those failures are tracked by this bug: https://bugs.webkit.org/show_bug.cgi?id=53146 I just finished building and ran the two tests I added in the two patches in this bug. They all passed successfully. Niwa, did you see the crash on buildbot?
Chang Shu
Comment 17 2011-03-01 10:19:21 PST
Created attachment 84246 [details] fix patch: gtk expected results I attached the gtk results as it comes handy.
Martin Robinson
Comment 18 2011-03-01 11:16:15 PST
Comment on attachment 84246 [details] fix patch: gtk expected results I'm afraid these pixel results aren't quite right. You'll need to run run-webkit-test in Xvfb and with "env -i". See: https://trac.webkit.org/wiki/WebKitGtkLayoutTests Thanks for looking into this!
Martin Robinson
Comment 19 2011-03-01 11:17:29 PST
(In reply to comment #18) > (From update of attachment 84246 [details]) > I'm afraid these pixel results aren't quite right. You'll need to run run-webkit-test in Xvfb and with "env -i". See: https://trac.webkit.org/wiki/WebKitGtkLayoutTests Thanks for looking into this! I'll add them now. Sorry I didn't take care of it yesterday.
Chang Shu
Comment 20 2011-03-01 11:27:09 PST
(In reply to comment #19) > (In reply to comment #18) > > (From update of attachment 84246 [details] [details]) > > I'm afraid these pixel results aren't quite right. You'll need to run run-webkit-test in Xvfb and with "env -i". See: https://trac.webkit.org/wiki/WebKitGtkLayoutTests Thanks for looking into this! > > I'll add them now. Sorry I didn't take care of it yesterday. Sure. thanks for the help!
Chang Shu
Comment 21 2011-03-01 11:30:03 PST
I don't think we have to keep this open anymore. The platform specific results can be landed without review.
Martin Robinson
Comment 22 2011-03-01 11:51:08 PST
Landed results for GTK+ here: http://trac.webkit.org/changeset/80016
Note You need to log in before you can comment on or make changes to this bug.