Bug 55323 – REGRESSION (r79762): Items in <select multiple> have focus rings, but shouldn't

• Component: Forms
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

<rdar://problem/9059911>

[Chang Shu]

Sorry, I will work on it.

[Chang Shu]

Created attachment 84084 [details]
fix patch

[Antonio Gomes]

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 on attachment 84084 [details]
fix patch

Clearing flags on attachment: 84084

Committed r79900: <http://trac.webkit.org/changeset/79900>

[Chang Shu]

Created attachment 84106 [details]
addtional patch that adds a new test case

this patch draws a focus ring with snav enabled.

[Chang Shu]

Ossy, do you mind generate expected results on Qt as a followup after my patches are landed?

[Antonio Gomes]

(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]

(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]

Sure, I'll make sure the GTK+ results are added.

[WebKit Commit Bot]

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]

The test added in this patch has been crashing on GTK.  Could you investigate it?

[Chang Shu]

(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]

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:
[[e-mail]] 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]

(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:
> [[e-mail]] 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]

> > [[e-mail]] 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]

Created attachment 84246 [details]
fix patch: gtk expected results

I attached the gtk results as it comes handy.

[Martin Robinson]

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]

(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]

(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]

I don't think we have to keep this open anymore. The platform specific results can be landed without review.

[Martin Robinson]

Landed results for GTK+ here: http://trac.webkit.org/changeset/80016