Closed Bug 902153 Opened 6 years ago Closed 6 years ago

Input sliders are broken

Categories

(Firefox OS Graveyard :: General, defect, major)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gerard-majax, Assigned: janjongboom)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Code for event fluffing (bug 789358) has landed a couple of hours ago, and it seems to break on some elements.

For example, I'm unable to configure the brightness level in Settings, on several devices (Inari and HTC Desire Z).
Summary: Event fluffing breaks manual brightness in settings → [Settings] Manual brightness broken
Actually, since this event fluffing is only in b2g-inbound, it can't be the root of this issue :(
This is funny, it's only on brightness, other input type range seems okay.
Summary: [Settings] Manual brightness broken → Input sliders are broken
Actually, after checking, not only manual brightness brokes, but also volumes sliders as reported in bug 899487.

When tapping the element, logcat shows a bunch of:
E/GeckoConsole( 1826): [JavaScript Error: "element is null" {file: "chrome://browser/content/forms.js" line: 824}]
E/GeckoConsole( 1826): [JavaScript Error: "element is null" {file: "chrome://browser/content/forms.js" line: 824}]
E/GeckoConsole( 1826): [JavaScript Error: "element is null" {file: "chrome://browser/content/forms.js" line: 824}]
Assignee: nobody → janjongboom
Hmm, in the simulator forms.js:getSelectionRange has `element.selectionStart` that throws...
Also we show a keyboard here, so we're not listening properly to a preventDefault()
Yuan, can you shine some light on this?
Flags: needinfo?(xyuan)
Alright, strange case here... in forms.js put a dump right after case "focus" and a dump right after case "input". Then you get this in your logcat:

```
input!
input!
focus!
```

So an input event fires before we even received focus...
Attached patch Proposed workaround (obsolete) — Splinter Review
Hack around this, two problems:

1. input event comes in before focus, add some null checking around it
2. selectionStart property cannot be read from element, throws in gecko. Catch this and prevent sending Forms:Input event so the keyboard won't be shown in those cases...
Attachment #788149 - Flags: review?(xyuan)
(In reply to Jan Jongboom [:janjongboom] from comment #8)
> Created attachment 788149 [details] [diff] [review]
> Proposed workaround
> 
> Hack around this, two problems:
> 
> 1. input event comes in before focus, add some null checking around it
> 2. selectionStart property cannot be read from element, throws in gecko.
> Catch this and prevent sending Forms:Input event so the keyboard won't be
> shown in those cases...

This workarounds fixes the issue for the brighness slider (after adding step="0.1" to it), I have yet to check for the volume case because for now it does not fix anything.
Comment on attachment 788149 [details] [diff] [review]
Proposed workaround

Review of attachment 788149 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Jan, thanks for the patch. I believe you found the root cause and the patch can resolve it, but an improvement to it will be better.

::: b2g/chrome/content/forms.js
@@ +617,5 @@
>      }
>  
> +    let json = getJSON(element, this._focusCounter);
> +    if (json.selectionStart === -1) {
> +      // element we cannot control with keyboard

We should ingored these input fields earlier. You can add these input filed that keyboard doesn't handle to FormAssistant.ignoredInputTypes.

@@ +846,5 @@
> +      start = element.selectionStart;
> +      end = element.selectionEnd;
> +    }
> +    catch (ex) {
> +      // f.e. input type="range" wont let you read these and throw

Is there any input field other than "range" throws here? If "range" input type won't be handle, do we still need to catch the error here?
Attachment #788149 - Flags: review?(xyuan) → review-
Flags: needinfo?(xyuan)
Attached patch 902153_v2.diff (obsolete) — Splinter Review
Yes, at the moment f.e. the keyboard pops up on type="date" and type="time", even though there is alternative UI present (you see the hide animation after choosing date/time). We can fix this later on in keyboard_manager.js when we implement new APIs. Ergo: for date/time selection we have our own IME that will then pop up, so we keep that pluggable, and we don't show active text IME. Should be easy to implemt and is nicer than hack exceptions for that in gecko.
Attachment #788149 - Attachment is obsolete: true
Attachment #788853 - Flags: review?(xyuan)
Comment on attachment 788853 [details] [diff] [review]
902153_v2.diff

Review of attachment 788853 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #788853 - Flags: review?(xyuan) → review+
Ready to check-in:-)
Keywords: checkin-needed
Missing commit information (again). Please config Hg to generate it by default.
Keywords: checkin-needed
I'm sorry that I forgot to check it.

Jan, please help to generate proper patch.

Thanks.
Status: NEW → ASSIGNED
Flags: needinfo?(janjongboom)
Yeah, I wanted to regenerate the patch before setting checkin-needed.
Flags: needinfo?(janjongboom)
Attached patch 902153_v2.diffSplinter Review
Attachment #788853 - Attachment is obsolete: true
Attachment #788929 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/92c940888425
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.