1511154 - Hotels.com date picker closes before user has a chance to pick a date

Status: RESOLVED FIXED

(GeckoView :: General, defect, P2)

Description

[Chris Peterson [:cpeterson]]

Steps to reproduce

1. open hotels.com
2. tap on the check-in or check-out field to enter a date

Expected behavior

-user should be able to select a date before the date picker closes

Actual behavior

-date picker closes before user gets a chance to select a date (I was able to do it on the 3rd try) or picks a random date and closes (I think depending on where the user tapped to open the date picker.

This bug was reported as part of the GeckoView/WebView perceived performance study and reproduced in the latest nighty.
Device information

    Device: Pixel
    Android device: 9
    Focus version: 8.0.1 (Build #323311210 GV 64.0.20181122182000)

Vesta filed a Focus issue:
https://github.com/mozilla-mobile/focus-android/issues/3984

I wonder if this bug is related to 2016 Fennec bug 1308952.

Comment 1

[Chris Peterson [:cpeterson]]

Is this date picker problem also reproducible in Fennec?

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

This works for me using Focus Nightly, Firefox 64 and Firefox 66. Using a Pixel 3.

Comment 3

[vzare]

I can still reproduce this error using my original Pixel. Sometimes it works if user picks a date really quickly before the picker closes.

Comment 4

[Chris Peterson [:cpeterson]]

Vesta, does this date picker work correctly for you in Chrome?

Agi says the date picker has the same behavior for him in GV and Chrome.

Comment 5

[Chris Peterson [:cpeterson]]

Resetting priority to bounce this bug back to triage.

Comment 6

[vzare]

Hi Chris and Agi, yes the date picker works correctly for me in Chrome. Let me take a video recording.

Comment 7

[Chris Peterson [:cpeterson]]

P1 for Fenix. Vesta sent me videos demonstrating the different behavior in Focus+GV and Chrome.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: 20190103 hotels.com (Focus).mp4

In Focus+GV, the date picker works correctly the first time, but subsequent attempts to change the date are thwarted by the date picker closing too soon.

I can reproduce this bug in Focus+GV, but not in Fennec or Chrome.

This date picker is super janky in both Fennec and GV. I'll file a separate perf bug for it.

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

Ha yeah I can reproduce this on the latest focus on my phone (but curiously not on the X86 emulator). I'll see if I can figure this out (or at least a minimum repro case).

Comment 10

[Chris Peterson [:cpeterson]]

Makoto, do you mind taking a look at this date picker bug? I don't know if this is an input focus issue

Bug 1517635 is a related bug about this Hotels.com date picker's animation being super janky and skipping frames in GV.

Comment 11

[Makoto Kato [:m_kato]]

OK, I look this. But we need investigate content script since I cannot sometimes select date even if Chrome/Android.

Comment 12

[Makoto Kato [:m_kato]]

Hmm, maybe, this depends on touch event handling. When tapping date field, date picker by hotels.com seems to recognize same touch point for leaving touch....

Comment 13

[Makoto Kato [:m_kato]]

I guess that root cause is that LoadURIDelegate doesn't use sync message manager. LoadURIDelegate.load has event loop, some event message is handled on touch event unfortunately.

touch event handler in hotels.com modifies location.hash, then nsIDocShell.LoadURI is called. So LoadURIDelegate.load is called.

Comment 14

[Chris Peterson [:cpeterson]]

[geckoview:fenix:p2] because this is important but technically not a release blocker. We currently ship this bug in Fennec so we could ship it in Fenix MVP.

Comment 15

[vzare]

FYI I just tested again and I can reproduce it on the original pixel but not on my pixel 3.

Comment 17

[Chris Peterson [:cpeterson]]

Makoto says this bug depends on onLoadRequest ordering bug 1441059.

Comment 18

[Chris Peterson [:cpeterson]]

I'm editing a bunch of GeckoView bugs. If you'd like to filter all this bugmail, search and destroy emails containing this UUID:

e88a5094-0fc0-4b7c-b7c5-aef00a11dbc9

Comment 19

[Makoto Kato [:m_kato]]

Rethinking this issue. Why do we call delegating network API when changing hash? Firefox 69 still call it, but Firefox 70 won't call it. This might be related to bug 1563587.

Comment 20

[Makoto Kato [:m_kato]]

Dylan, I think that location.hash setter shouldn't call onLoadRequest of NavigationDelegate since it doesn't create new network request. How do you think? If agree, I will add a test since we already change this behaviour by bug 1563587.

Also, even if not calling onLoadRequest, we still call onLocationChange since location is changed by location.hash setter.

Comment 21

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea8b86a0cce0d1c9a329a6e67648e9fbac7009b2

Comment 22

[Dylan Roeh (:droeh) (he/him)]

(In reply to Makoto Kato [:m_kato] from comment #20)

Dylan, I think that location.hash setter shouldn't call onLoadRequest of NavigationDelegate since it doesn't create new network request. How do you think? If agree, I will add a test since we already change this behaviour by bug 1563587.

Also, even if not calling onLoadRequest, we still call onLocationChange since location is changed by location.hash setter.

I think that sounds right; bouncing the NI to snorp just in case I'm overlooking something.

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah, onLoadRequest is only called for page loads.

Comment 24

[Makoto Kato [:m_kato]]

Thank you, I will add junit test.

Comment 25

[Makoto Kato [:m_kato]]

Attachment: Bug 1511154 - Add junit test for location.hash setter. r?

By bug 1563538, we don't call onLoadRequest when setting location.hash. So we should add this test.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08669f191b51
Add junit test for location.hash setter. r=geckoview-reviewers,droeh

Comment 27

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/08669f191b51

Comment 28

[Chris Peterson [:cpeterson]]

firefox-esr68=wontfix and firefox69=wontfix because we don't need to uplift this fix to Fennec ESR 68 or GV 69 Beta. This fix can ride the trains when GV 70 Nightly rides to Beta next week.