Bug 55323 - REGRESSION (r79762): Items in <select multiple> have focus rings, but shouldn't
Summary: REGRESSION (r79762): Items in <select multiple> have focus rings, but shouldn't
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Chang Shu
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2011-02-27 10:11 PST by Adam Roben (:aroben)
Modified: 2011-03-01 11:51 PST (History)
6 users (show)

See Also:


Attachments
fix patch (27.67 KB, patch)
2011-02-28 10:56 PST, Chang Shu
no flags Details | Formatted Diff | Diff
addtional patch that adds a new test case (25.94 KB, patch)
2011-02-28 13:14 PST, Chang Shu
no flags Details | Formatted Diff | Diff
fix patch: gtk expected results (29.68 KB, patch)
2011-03-01 10:19 PST, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 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.
Comment 1 Adam Roben (:aroben) 2011-02-27 10:18:37 PST
<rdar://problem/9059911>
Comment 2 Chang Shu 2011-02-27 11:16:10 PST
Sorry, I will work on it.
Comment 3 Chang Shu 2011-02-28 10:56:35 PST
Created attachment 84084 [details]
fix patch
Comment 4 Antonio Gomes 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.
Comment 5 WebKit Commit Bot 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>
Comment 6 Chang Shu 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.
Comment 7 Chang Shu 2011-02-28 13:25:57 PST
Ossy, do you mind generate expected results on Qt as a followup after my patches are landed?
Comment 8 Antonio Gomes 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?
Comment 9 Csaba Osztrogonác 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.
Comment 10 Martin Robinson 2011-02-28 13:52:42 PST
Sure, I'll make sure the GTK+ results are added.
Comment 11 WebKit Commit Bot 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>
Comment 12 Ryosuke Niwa 2011-03-01 01:05:00 PST
The test added in this patch has been crashing on GTK.  Could you investigate it?
Comment 13 Chang Shu 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.
Comment 14 Chang Shu 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>
Comment 15 Martin Robinson 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
Comment 16 Chang Shu 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?
Comment 17 Chang Shu 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.
Comment 18 Martin Robinson 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!
Comment 19 Martin Robinson 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.
Comment 20 Chang Shu 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!
Comment 21 Chang Shu 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.
Comment 22 Martin Robinson 2011-03-01 11:51:08 PST
Landed results for GTK+ here: http://trac.webkit.org/changeset/80016