Closed Bug 1132728 Opened 5 years ago Closed 5 years ago

Various elements (mainly buttons) get dotted outlines after tapping them on b2g

Categories

(Core Graveyard :: Widget: Gonk, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
mozilla39
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 --- fixed
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: ychung, Assigned: cwiiis)

References

Details

(Keywords: polish, regression, Whiteboard: [systemsfe][3.0-Daily-Testing])

Attachments

(3 files, 1 obsolete file)

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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Attached image Outline.png
Chris, any idea where this is coming from?
Flags: needinfo?(chrislord.net)
Whiteboard: [systemsfe]
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Whiteboard: [systemsfe] → [systemsfe][3.0-Daily-Testing]
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)
Flags: needinfo?(eitan)
Flags: needinfo?(chrislord.net)
Flags: needinfo?(bugs)
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.
Flags: needinfo?(eitan)
QA Whiteboard: [QAnalyst-Triage+]
QA Contact: pcheng
(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?)
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
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.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
(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.
Flags: needinfo?(bugs)
(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...
Attached patch Don't draw focus rings on b2g (obsolete) — Splinter Review
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).
Attachment #8565999 - Flags: review?(fabrice)
Duplicate of this bug: 1073622
Component: Gaia::Homescreen → Widget: Gonk
Keywords: polish
Product: Firefox OS → Core
Summary: (app-grouping) Outline appears when selecting expand/collapse button → Various elements (mainly buttons) get dotted outlines after tapping them on b2g
Target Milestone: --- → mozilla38
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 */
Attachment #8565999 - Flags: review?(fabrice) → review+
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.
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
Attachment #8565999 - Attachment is obsolete: true
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.
Attachment #8566479 - Flags: review?(fabrice)
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?
Flags: needinfo?(taken.spc)
I misunderstood what this variable means. Please ignore my previous comment.
Flags: needinfo?(taken.spc)
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.
Attachment #8566479 - Flags: review?(bzbarsky)
Comment on attachment 8566479 [details] [diff] [review]
Don't draw focus rings on b2g v2

Makes sense.  r=me
Attachment #8566479 - Flags: review?(bzbarsky) → review+
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.
Attachment #8566479 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/fb071ecfdeb3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla38 → mozilla39
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
Attachment #8566479 - Flags: approval-mozilla-b2g37?
Duplicate of this bug: 1134773
Attachment #8566479 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Keywords: verifyme
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.