915461 - [B2G] [Buri] [1.2] [Music] Tapping x in music search bar does not clear text in search bar

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[Chris Kreinbring [:CKreinbring]]

Description:
Tapping the x at the end of the music search bar will not erase any text that is already in the search bar.

Repro Steps:
1) Update Buri to Build ID: 20130911040201
2) Launch the Music app.
3) Scroll the screen upwards to reveal the search bar.
4) Enter a short text string in the textbox.
5) Tap the x at the right end of the textbox and observe the textbox's reaction.

Actual:
The keyboard disappears, but the text in the search textbox remains.

Expected:
The text in the search textbox is erased.

Environmental Variables
Occurs on Buri 1.2 mozilla RIL
Build ID: 20130911040201
Gecko: http://hg.mozilla.org/mozilla-central/rev/f9e8e8ce552c
Gaia: ebbb325958c66c3e68e1f190472d5f6e9076d83a
Platform Version: 26.0a1

Notes:
Repro frequency: 100%
Test Suite Name: Music
UCID: music, search
Link to failed test case: https://moztrap.mozilla.org/manage/cases/?filter-id=6955
See attached screenshot, nothing appeared on the logcat logs.

Comment 1

[Chris Kreinbring [:CKreinbring]]

Attachment: Screenshot of search bar

Comment 2

[sarsenyev]

Regression Window!

The bug doesn't reproduce on Buri:
Build ID: 20130818040219
Gecko: http://hg.mozilla.org/mozilla-central/rev/a71cedddadd1
Gaia: 2675cc8d2d4fb18c6071b396c182fb8fd0e56fa3
Platform Version: 26.0a1

The bug reproduces on Buri:
Build ID: 20130819040203
Gecko: http://hg.mozilla.org/mozilla-central/rev/c8c9bd74cc40
Gaia: f6de05c135913f2cb790759335875bb1b3c4f9bb
Platform Version: 26.0a1

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: Pull request to fix clear search on tap of x

Hi Dominic

As part of Bug 896471, shared/js/mouse_event_shim.js was removed from apps/music/index.html with the possible fall out of clear search reset button stopped working. 

I am attaching pull request with the fix of clear search on tap of x. Please review.

Thanks

Comment 4

[Punam Dahiya [:pdahiya]]

Hi Dominique,
While testing it on hamachi, I realized the submitted PR is not enough to fix the issue. I am still struggling with identifying the right fix. I do see it working when i make this additional change on top of PR by making the z-index of the reset button higher than others like this:

form p input + button[type="reset"] {
  z-index: 2000;
}

Its not ideal, but at this point, am not sure how to handle it any better.

Any inputs?

Comment 5

[Dominic Kuo [:dkuo]]

Comment on attachment 805717 [details] [review]
Pull request to fix clear search on tap of x

Punam,

Thanks for looking into this, as comment 3 and after I took a quick look on your patch, I found the suitable and probably the simplest fix should be listening to "touchend" instead of "click" on the views, I am not sure why the click event didn't fire to the search elements but the touchend event(might be the z-index), also changing from click to touchend seems harmless to the existing features(but this just my quick test), more, if it's possible, I feel like increasing the "clear" area is also a worthy change with this patch, so let's go for the new approach and after you revised it, feel free to re-assign review to me, thanks!

Comment 6

[Punam Dahiya [:pdahiya]]

(In reply to Dominic Kuo [:dkuo] from comment #5)
> Comment on attachment 805717 [details] [review]
> Pull request to fix clear search on tap of x
> 
> Punam,
> 
> Thanks for looking into this, as comment 3 and after I took a quick look on
> your patch, I found the suitable and probably the simplest fix should be
> listening to "touchend" instead of "click" on the views, I am not sure why
> the click event didn't fire to the search elements but the touchend
> event(might be the z-index), also changing from click to touchend seems
> harmless to the existing features(but this just my quick test), more, if
> it's possible, I feel like increasing the "clear" area is also a worthy
> change with this patch, so let's go for the new approach and after you
> revised it, feel free to re-assign review to me, thanks!

Thanks Dominic for the feedback. I have updated PR by using 'touchend' event listener on views. I have increased reset button width by 1rem. Please review

Comment 7

[Dominic Kuo [:dkuo]]

Punam, I think I was wrong on the touchend event, I pulled your branch and found if I scroll the list, it will enter one of the album/artist unexpectedly, which means the touchend event was fired because of the scrolling, so maybe we have to find the root cause that why the clear button cannot receive the click event, or we need to listen to touchstart additionally to judge that it's a tap/click from the users. Do you have any idea why the clear button cannot receive the click event?

Comment 8

[Punam Dahiya [:pdahiya]]

(In reply to Dominic Kuo [:dkuo] from comment #7)
> Punam, I think I was wrong on the touchend event, I pulled your branch and
> found if I scroll the list, it will enter one of the album/artist
> unexpectedly, which means the touchend event was fired because of the
> scrolling, so maybe we have to find the root cause that why the clear button
> cannot receive the click event, or we need to listen to touchstart
> additionally to judge that it's a tap/click from the users. Do you have any
> idea why the clear button cannot receive the click event?

My findings are it's setting of pointer-events to none in input_areas.css thats blocking the click event.
https://github.com/punamdahiya/gaia/blob/master/shared/style/input_areas.css#L72

Its unset on foucs of search input box
https://github.com/punamdahiya/gaia/blob/master/shared/style/input_areas.css#L99

which for some reason is not working after removing mouse_event_shim.js.

If i explicitly add below css in music.css file it starts throwing click event on clear button

form p input + button[type="reset"] {
  pointer-events: auto !important;
  opacity: 1;
}

Comment 9

[Dominic Kuo [:dkuo]]

Great, then I start feeling this is an issue in the building block.

According to: https://developer.mozilla.org/en-US/docs/Web/CSS/pointer-events
The value "all" of the pointer-events should applied to SVG only, and since the clear button is a button with a png background image, we should use { pointer-events: auto } just like you mentioned. We should modify input_areas.css instead of adding override styles in music.css so that we can also avoid this issue happens in some other apps that use the shared input_areas.css as well.

So if I didn't miss anything this time, the patch will become:
1. Modify input_areas.css in shared/style/
2. When click fires, add clearing the search input and re-focusing to the search input

Let's try these and feel free to re-assign review to me after you modify it, thanks!

Comment 10

[Punam Dahiya [:pdahiya]]

Agreed, fixing this issue in building blocks is ideal. There is a bug https://bugzilla.mozilla.org/show_bug.cgi?id=830127 open with that and some additional requirements. 

For music app, i tried to take a stab at fixing this issue similar to email search fix using email/js/input_areas.js. 

The idea behind is to not take focus from the input fields, which causes pointer-events on clear button to set to none.

I am handling touch start on reset button and calling preventDefault on that so it doesn't change the focus. I have updated PR with the fix.

In the above implemented fix for some reason, few user taps on x goes to parent form element views-tiles-search instead of views-tiles-search-clear. Any ideas why parent becomes target for some of the taps. Thanks

Comment 11

[Punam Dahiya [:pdahiya]]

Hi Dominic

I have updated attached PR with the fix. To handle parent form elements taps, i have added handler on the form and checking for the originalTarget. 

In some cases user might have to tap multiple times on x to clear, these are inherent issue with BB and seen in contacts app. I have opened bug https://bugzilla.mozilla.org/show_bug.cgi?id=920770
for these issues.

Please review the fix.

Thanks

Comment 12

[Dominic Kuo [:dkuo]]

Pynam, I am confused now, in comment 8, adding { pointer-events: auto } to override the style of the clear can let it starts throwing click event, so doesn't it fix the problem we encountered here?

And I saw the new patch you listen to touchstart and treat it as click event, which I think probably not a suitable replacement for click event. If we only listen to touchstart, then once the user touch the element it will immediately trigger what it should does for click, but click should be a touchstart + touchend so I think we better also listen to touchend if you want to fix this issue like that.

I prefer the way in comment 8 if it's workable, I haven't try it deeply myself so if I am wrong, please correct me, thanks.

Comment 13

[Punam Dahiya [:pdahiya]]

(In reply to Dominic Kuo [:dkuo] from comment #12)
> Pynam, I am confused now, in comment 8, adding { pointer-events: auto } to
> override the style of the clear can let it starts throwing click event, so
> doesn't it fix the problem we encountered here?

That's true, I tried settings pointer-events to auto in input_areas.css
https://github.com/punamdahiya/gaia/blob/master/shared/style/input_areas.css#L99
In line 99, updating pointer events to auto doesn't make tap on clear button throw click events

https://github.com/punamdahiya/gaia/blob/master/shared/style/input_areas.css#L72
In line 72, setting pointer-events to auto, will have clear button starts throwing click events but than it makes clear button active all the time even when it's invisible. 

Andrew's comment, https://bugzilla.mozilla.org/show_bug.cgi?id=830127#c7 explains it very well.

> 
> And I saw the new patch you listen to touchstart and treat it as click
> event, which I think probably not a suitable replacement for click event. If
> we only listen to touchstart, then once the user touch the element it will
> immediately trigger what it should does for click, but click should be a
> touchstart + touchend so I think we better also listen to touchend if you
> want to fix this issue like that.
> 

Agreed, will update PR to use touchend event on clear button to clear the input textbox.
> I prefer the way in comment 8 if it's workable, I haven't try it deeply
> myself so if I am wrong, please correct me, thanks.

Comment 14

[Punam Dahiya [:pdahiya]]

Attached PR updated to handle touchend event on tap of clear button. Please review. Thanks

Comment 15

[Hema Koka [:hema]]

Dominic,

please take a look at the updated PR from Punam - hopefully we can close this before the summit.

Thanks
Hema

Comment 16

[Dominic Kuo [:dkuo]]

Comment on attachment 805717 [details] [review]
Pull request to fix clear search on tap of x

Punam, the updated patch looks good to me, only some minor issues and just address them before you land it. I still feel this is a bug in the building blocks because we have to workaround in both email and music, but since this is a regression and koi+, we should land it first and remove the workaround after we have come out a correct approach to modify the input areas, thanks.

Comment 17

[Punam Dahiya [:pdahiya]]

Thanks Dominic for the review. I have addressed all the issues except https://github.com/mozilla-b2g/gaia/pull/12257#discussion_r6679778, which i have explained in PR.

I don't have permissions to land PR in master and will appreciate if you can help merge it in master.

Comment 18

[Dominic Kuo [:dkuo]]

Thanks for working on this, Punam!

Landed on master: 90e78f2744412abf3a57d4d4fe981d662203d93a

Comment 19

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 90e78f2744412abf3a57d4d4fe981d662203d93a to:
v1.2: 0a89310b3a72ea5887420dfaefe544bf4f07facd

Comment 20

[Shilpa Suresh]

The issue still reproduces on Buri v1.2 aurora Build(10/10)
Tapping x in music search bar does not clear text in search bar

Environmental Variables
Device: Buri v 1.2.0 aurora Mozilla RIL)
Build ID: 20131010004001
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/3f16dc100b1f
Gaia: 51f3c79ea93bb91d3b12e50b49d203a049a94a9b
Platform Version: 26.0a2
Firmware Version: US_20130912_QC211

Can we reopen this Bug or Log a new Bug for the same issue?

Comment 21

[Shilpa Suresh]

This issue is not reproducing 100% though. Tapping on lower part of x in music search bar sometimes clears text.

Comment 22

[Marcia Knous [:marcia]]

Verified fixed on both branches using Buri device:

Gaia   cb981e2f47bc644b4d178d54378c3676c946facf
SourceStamp eec4da1b27eb
BuildID 20131103004003
Version 26.0

Gaia   1aee772a384f1ed1148f08c6c7df45d2fe35506e
SourceStamp b4143e04bea1
BuildID 20131104044747
Version 28.0a1

I don't see any of the issues noted in Comment 20.