Description: When the user selects an expand/collapse button, a dotted outline appears around the target area of the button. Repro Steps: 1) Update a Flame to 20150212010213. 2) Select any expand/collapse button on the homescreen. Actual: Outline appears around the target area on the button. Expected: No outline appears around the target area. Environmental Variables: Device: Flame Master (319mb, full flash) Build ID: 20150212010213 Gaia: d5a71cedb37dd45f439f672489db3994b349ac43 Gecko: 3094601af679 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 38.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0 Repro frequency: 5/5 See attached: screenshot logcat

This issue does NOT reproduce on Flame 2.2. Result: No outline appears when tapping expand/collapse button. Device: Flame 2.2 (319mb, full flash) Build ID: 20150212002504 Gaia: 791e53728cd8018f1d7cf7efe06bbeb1179f0370 Gecko: 5dec207fcbeb Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 37.0a2 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Chris, any idea where this is coming from?

This outline appears to be a focus indicator - I've seen it in various other places too, and as far as I'm aware, we either shouldn't be displaying focus states (except perhaps for accessibility?) or we shouldn't be marking non-editable elements as focused on a touch screen. Asking for a regression window, but also n?jet to see if there's someone on layout/rendering that can weigh in here - my basic question is "Why are we displaying focus rings on a touch-screen device with no keyboard and with accessibility disabled?" (and the follow-up question is "How do we fix that?" :)) I guess a short-term fix is to disable the style in CSS, but I'm wary of doing that and breaking accessibility (perhaps my concern is unwarranted? n?eitan for that)

This is not an issue of accessibility alone, but of keyboard support in general. Do we ever expect a user to tab around the interface with a keyboard? If the answer is no, then we could get rid of the focus indicator. We have our own cursor indicator in the screen reader that is separate. So there is no immediate effect here.

(In reply to Eitan Isaacson [:eeejay] from comment #5) > This is not an issue of accessibility alone, but of keyboard support in > general. Do we ever expect a user to tab around the interface with a > keyboard? If the answer is no, then we could get rid of the focus indicator. > > We have our own cursor indicator in the screen reader that is separate. So > there is no immediate effect here. In that case, I assume we could fix this by altering the b2g widget style (which is something I assume we have?). I'll have a look at this tomorrow. I do think it would be better to detect and deal with this at the platform level, however (who says that all b2g devices won't have some kind of keyboard/pad input?)

b2g-inbound regression window: Last Working Environmental Variables: Device: Flame BuildID: 20150211092847 Gaia: e280a660955bbdab265d50f8d9e009de34082332 Gecko: c1ac604684b4 Version: 38.0a1 (3.0 Master) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0 First Broken Environmental Variables: Device: Flame BuildID: 20150211094730 Gaia: db36424ccef9d601ece913a804f40a01bbb81bef Gecko: 80a04d108cc9 Version: 38.0a1 (3.0 Master) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0 Last Working Gaia & First Broken Gecko - issue does NOT repro Gaia: e280a660955bbdab265d50f8d9e009de34082332 Gecko: 80a04d108cc9 Last Working Gecko & First Broken Gaia - issue DOES repro Gaia: db36424ccef9d601ece913a804f40a01bbb81bef Gecko: c1ac604684b4 Gaia pushlog: https://github.com/mozilla-b2g/gaia/compare/e280a660955bbdab265d50f8d9e009de34082332...db36424ccef9d601ece913a804f40a01bbb81bef Caused by the patch for Bug 1123023.

(In reply to Chris Lord [:cwiiis] from comment #4) > Asking for a regression window, but also n?jet to see if there's someone on > layout/rendering that can weigh in here - my basic question is "Why are we > displaying focus rings on a touch-screen device with no keyboard and with > accessibility disabled?" (and the follow-up question is "How do we fix > that?" :)) (In reply to Pi Wei Cheng [:piwei] from comment #7) > Caused by the patch for Bug 1123023. Seems the patch in the bug just wanted labels and not necessarily focus indicators. Modifying the CSS to split those styles should do the trick. Drop me a line if we need Gecko Layout changes after all.

(In reply to Jet Villegas (:jet) from comment #8) > (In reply to Chris Lord [:cwiiis] from comment #4) > > > Asking for a regression window, but also n?jet to see if there's someone on > > layout/rendering that can weigh in here - my basic question is "Why are we > > displaying focus rings on a touch-screen device with no keyboard and with > > accessibility disabled?" (and the follow-up question is "How do we fix > > that?" :)) > > (In reply to Pi Wei Cheng [:piwei] from comment #7) > > Caused by the patch for Bug 1123023. > > Seems the patch in the bug just wanted labels and not necessarily focus > indicators. Modifying the CSS to split those styles should do the trick. > Drop me a line if we need Gecko Layout changes after all. So it looks like this patch caused it because it switched the element from a span to a button, and we're picking up default button styling, which gives us a focus ring - I still think this is a platform issue that we're applying the focus style at all when there's no keyboard (or other button-based input device), but I guess we should be able to fix this by tweaking button styling. We have other bugs about buttons getting 'outlines' after they've been pressed (like bug 1092354), so would be good to fix this properly rather than on an app-by-app basis.

Still looking at this, from what I can tell, the dotted outline doesn't seem to come from style...? At least, just commenting out all the focus styling in b2g/chrome/content/content.css does nothing and inspecting the style and computed style of the element doesn't reflect its outline...

Well, that turned out simpler than expected :) Downside: If we have b2g devices with key navigation, they'll no longer get default focus rings (but they can easily pref them back on).

Comment on attachment 8565999 [details] [diff] [review] Don't draw focus rings on b2g Review of attachment 8565999 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/app/b2g.js @@ +40,4 @@ > /* disable text selection */ > pref("browser.ignoreNativeFrameTextSelection", true); > > +// Bug 1132728: Disable focus rings nit: local style is /* disable focus rings */

I moved the comment up to group with the other prefs that use the // style - there are actually more lines with that then there are using C-style comments and I don't think it hurts to link to the providence of the decision. Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a29c4ad0d52b

The blame is enough to get back to the root of the change, but whatever.

sorry had to back this out for test failure like https://treeherder.mozilla.org/logviewer.html#?job_id=6768545&repo=mozilla-inbound

Ends up there are a couple of tests that presume things about form element rendering (such as the default 1-pixel border for the focus ring) - just changing the reftest manifest to explicitly set the pref for those tests that fail. Will flag for review after try comes back green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=396c6b9b0f52

Comment on attachment 8566479 [details] [diff] [review] Don't draw focus rings on b2g v2 Do you think this is ok? These tests are already fuzzed because Android/b2g have different form styling to the default (I presume, perhaps incorrectly), so just setting the focus-ring-width pref seems a reasonable thing to do to me.

I prefer setting |mShowFocusRings| to true because the focus ring will be visible without any works once we get to support keyboard. https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#1103

(In reply to Takeshi Kurosawa from comment #19) > I prefer setting |mShowFocusRings| to true because the focus ring will be > visible without any works once we get to support keyboard. > https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow. > cpp#1103 I'm not sure what you mean here? We could set this to false to get the same effect I suppose, but I'd prefer a pref change to a code change - it's more easily discoverable and we can tweak it with per-device prefs if we want. Could you elaborate a bit on why you'd prefer to manipulate this variable and how it's easier than manipulating the pref?

I misunderstood what this variable means. Please ignore my previous comment.

Comment on attachment 8566479 [details] [diff] [review] Don't draw focus rings on b2g v2 In case bz is less busy than fabrice, r? to see if what I'm doing on the failing tests is reasonable.

Comment on attachment 8566479 [details] [diff] [review] Don't draw focus rings on b2g v2 Makes sense. r=me

Comment on attachment 8566479 [details] [diff] [review] Don't draw focus rings on b2g v2 Cool, r=bz for test changes, r=fabrice carried over from prior patch for pref change.

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb071ecfdeb3

Comment on attachment 8566479 [details] [diff] [review] Don't draw focus rings on b2g v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Ugly dotted outline on some elements after tapping them Testing completed: Manual testing completed, platform tests still pass Risk to taking this patch (and alternatives if risky): Low String or UUID changes made by this patch: None

This issue is verified fixed on the latest Nightly Flame 3.0 and 2.2 builds. Actual results: Outlines do not appear around objects when tapping them. Environmental Variables: Device: Flame 3.0 KK (Full Flash) (319 MB) BuildID: 20150306010207 Gaia: 7a91c16bfa348be8b25e09719178efa051512988 Gecko: 0189941a3fd5 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 39.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0 Environmental Variables: Device: Flame 2.2 KK (Full Flash) (319 MB) BuildID: 20150306002519 Gaia: eb86137e247224e86d17ed1a0a133b2a318dce3c Gecko: a04034e239fb Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 37.0 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0