ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.89k stars 13.52k forks source link

bug: ion-segment on android #20466

Closed s3ppo closed 4 years ago

s3ppo commented 4 years ago

Bug Report

Ionic version: [x] 5.x

Current behavior: only 1st ion-segment-button within ion-segment is clickable on android.. on ios it is working fine

Expected behavior: all should be clickable on android

Steps to reproduce: install ionic 5 and run on android

  <ion-toolbar>
    <ion-segment value="general" (ionChange)="segmentChanged($event)">
      <ion-segment-button value="general">
        <ion-label>General</ion-label>
      </ion-segment-button>
      <ion-segment-button value="notifications">
        <ion-label>Notification</ion-label>
      </ion-segment-button>
    </ion-segment>
  </ion-toolbar>

Related code:

insert short code snippets here

Other information:

Ionic info:

Ionic:

   Ionic CLI                     : 5.4.16
   Ionic Framework               : @ionic/angular 5.0.0
   @angular-devkit/build-angular : 0.803.23
   @angular-devkit/schematics    : 8.1.3
   @angular/cli                  : 8.1.3
   @ionic/angular-toolkit        : 2.1.2

Cordova:

   Cordova CLI       : 9.0.0 (cordova-lib@9.0.1)
   Cordova Platforms : android 8.1.0, browser 6.0.0, ios 5.1.1
   Cordova Plugins   : cordova-plugin-ionic-keyboard 2.2.0, cordova-plugin-ionic-webview 4.1.3, (and 15 other plugins)

Utility:

   cordova-res (update available: 0.9.0) : 0.6.0
   native-run (update available: 0.3.0)  : 0.2.4

System:

   Android SDK Tools : 26.1.1 (/Users/haraldwiesinger/Library/Android/sdk)
   ios-deploy        : 1.10.0
   ios-sim           : 8.0.2
   NodeJS            : v10.15.3 (/usr/local/bin/node)
   npm               : 6.13.4
   OS                : macOS Catalina
   Xcode             : Xcode 11.3.1 Build version 11C504
liamdebeasi commented 4 years ago

Thanks for the issue. What devices are you testing this on?

s3ppo commented 4 years ago

i noticed it on the simulator.. i think its a nexus 5

liamdebeasi commented 4 years ago

Can you try this on a physical Android device if possible? Some of the older emulators ship with very outdated versions of the Android webview that cannot be upgraded. As a result, we recommend using physical devices when testing apps on older versions of Android (most physical devices have webviews that auto update).

s3ppo commented 4 years ago

yes of course.. i will try this a little bit later in the evening 👍

gminhaneli commented 4 years ago

+1 , testado em vários dispositivos, dois de exemplos (moto G 7 play e Pixel 2)

gminhaneli commented 4 years ago

Peek 13-02-2020 18-11

s3ppo commented 4 years ago

thx for screenshot @gminhaneli.. thats the same like on my simulator 👍

gminhaneli commented 4 years ago

Aguardando um posicionamento, esse componente sem o clique é dificil pro usuário utilizar

SSuarezBrooktec commented 4 years ago

Same error on Android 5.1 or 6 with Ionic 5, ion-segment do not working due a queryselector error in console on change segment-button. Can't change segment.

Captura de pantalla 2020-02-14 a las 11 58 59

SSuarezBrooktec commented 4 years ago

I try now on physical device (Android 5.1.1, Nexus 4) and ion-segment not working too

liamdebeasi commented 4 years ago

Can someone provide repo with the code required to reproduce this issue? I am unable to reproduce this behavior, and a repo will help us address this issue faster.

SSuarezBrooktec commented 4 years ago

Check this issue https://github.com/ionic-team/ionic/issues/20487

liamdebeasi commented 4 years ago

@SSuarezBrooktec Are these the same issue?

SSuarezBrooktec commented 4 years ago

On my issue in Android 5 do not work any function on ion-segment. I think there are 2 different issues.

liamdebeasi commented 4 years ago

For the issue you linked to, does the repo there reproduce the issue in this thread?

SSuarezBrooktec commented 4 years ago

Its similar then you can check it anyway

gminhaneli commented 4 years ago

Estou acompanhando aqui para ajudar também

gminhaneli commented 4 years ago

have result?

liamdebeasi commented 4 years ago

Yes, I was able to reproduce this issue. We are investigating a fix.

gminhaneli commented 4 years ago

@liamdebeasi quando subirá essa atualização?

liamdebeasi commented 4 years ago

Can you try the following dev build and let me know if it resolves the issue?

Angular npm i @ionic/angular@5.1.0-dev.202002182107.b0ce4ab

React npm i @ionic/react@5.1.0-dev.202002182107.b0ce4ab

SSuarezBrooktec commented 4 years ago

ok will check now

gminhaneli commented 4 years ago

@liamdebeasi o ERRO ainda persiste.

Peek 19-02-2020 08-39

SSuarezBrooktec commented 4 years ago

Kapture 2020-02-19 at 12 46 56

Not working on Android 5.1

liamdebeasi commented 4 years ago

Can everyone provide a full code reproduction of the issue happening? I checked the reproduction in https://github.com/ionic-team/ionic/issues/20487 and the fix works fine.

SSuarezBrooktec commented 4 years ago

I tried with my repo and compile for Android 5.1 but is not working

Kapture 2020-02-19 at 15 36 39

gminhaneli commented 4 years ago

@liamdebeasi consegui replicar o problema, é o uso do bloqueio do touch no ngOnInit ou ionViewDidEnter

document.addEventListener("touchstart", () => { this.slides.lockSwipes(true); }, false); document.addEventListener("touchend", () => { this.slides.lockSwipes(true); }, false); document.addEventListener("touchcancel", () => { this.slides.lockSwipes(true); }, false); document.addEventListener("touchleave", () => { this.slides.lockSwipes(true); }, false); document.addEventListener("touchmove", () => { this.slides.lockSwipes(true); }, false);

liamdebeasi commented 4 years ago

@SSuarezBrooktec Can you test this on a physical device? We do not recommend testing on emulators are they ship with very outdated, non upgradable versions of the webview.

I am testing your app with my dev build on Android 5.1, and I am seeing your app work properly now.

gminhaneli commented 4 years ago

O problema real agora encontrei, é ion-segment esta com z-index css menor que o filho. coloquei z-index: 11; funcionou.

simonhaenisch commented 4 years ago

@liamdebeasi I've encountered this issue as well and it only happened on one page for us where the structure is

<ion-header />
<ion-segment />
<ion-content /> <!-- this is switched out based on the value of the segment -->

In other pages where the segment was inside the ion-content it was working. I was able to reproduce it with an ionic-pwa stencil starter a couple days ago. I'll try to give you a reproduction. It was happening when switching to the device simulator in Chrome devtools, but only in md mode. On desktop the ripple effect was always on the segment-button that was previously selected instead of the clicked one. In ios mode the segments were working es expected.

simonhaenisch commented 4 years ago

@liamdebeasi here you go: https://codepen.io/simonhaenisch/full/poJEEgv

Open this in Chrome dev tools, toggle the device toolbar, then switch to an Android simulator, e. g. Galaxy S5. Clicking the second tab will trigger the ripple effect on the first one but it won't switch to the second tab.

If you open it normally (without a device simulator), you will see that the ripple effect is applied to the wrong segment-button (always the one that was active prior to the click).

If you switch to an iPhone simulator, the segments will work as expected.

yarivgdidi commented 4 years ago

Hi Liam as promised, I've update my repo to reproduce this bug git clone https://github.com/yarivgdidi/ion-segments-20381 npm i

tab 1 has the issue with android, tabs 2 and 3 don't the difference are: tab 1, ion-segment is not surrounded by ion-toolbar (not working in android) tab 2, ion-segment surrounded by ion-toolbar (working) tab 3, ion-segment not not surrounded by ion-toolbar and ion-header is not present (working)

Thanks Yariv @liamdebeasi

aspoonemore commented 4 years ago

@liamdebeasi Here's a link to a repo you can use to reproduce the problem

https://github.com/aspoonemore/ionic-v5-android-segment

Thanks, Dale Spoonemore

yarivgdidi commented 4 years ago

@aspoonemore, @liamdebeasi, @s3ppo
Just verified on Dales repo, surrounding ion-segments with ion-toolbar solve this

`

Test1 Test2

` Yariv

SSuarezBrooktec commented 4 years ago

Errors on console in my project using ion-segment

Captura de pantalla 2020-02-20 a las 13 48 36

liamdebeasi commented 4 years ago

Thanks for all the follow ups. I have made a few changes and created a new dev build. I have verified with all of the provided reproductions that this dev build fixes the issue.

Here is the dev build if people would like to test in their other apps:

Angular npm i @ionic/angular@5.1.0-dev.202002201606.ac98652

React npm i @ionic/react@5.1.0-dev.202002201606.ac98652

s3ppo commented 4 years ago

@liamdebeasi i tested it with the devbuild.. works like charm :) thank you very much

s3ppo commented 4 years ago

is it already included in yesterdays release of 5.0.1 or do we have to wait for 5.1.0?

liamdebeasi commented 4 years ago

It is not in the 5.0.1 release, but will be in an upcoming patch release.

aspoonemore commented 4 years ago

Works for me too. Great job, Ionic team!

Dale

SSuarezBrooktec commented 4 years ago

For me in physichal mobile: Huawei P3 Lite Android 5.0.1 is not working. Because webview is v39 (if i install the last webview version, didn't work). If you want you can check my repo here https://github.com/SSuarezBrooktec/Ionic5_ion_segment_error

Error is this:

Captura de pantalla 2020-02-21 a las 9 55 50

Segment & tabs not working (both)

bodinaren commented 4 years ago

I tried out the dev build and I still experience the same issue as detailed in https://github.com/ionic-team/ionic/issues/20466#issuecomment-588478532.

I attempted one fix which seems to work at least for this case, on this line... https://github.com/ionic-team/ionic/blob/master/core/src/components/segment/segment.tsx#L305

What happens is when it tries to find nextEl on the next line it gets the ion-header element instead. Instead of using the top point of the previously selected button we use the vertical center of the element like this: const previousY = rect.y + (rect.height / 2);

yarivgdidi commented 4 years ago

I confirm the fix work, great job @liamdebeasi and team @bodinaren is your segment surrounded with ion-toolbar , e.g.

<ion-toolbar>
    <ion-segment (ionChange)="segmentChanged($event)" >

        <ion-segment-button >
          <ion-label>
            one
          </ion-label>
        </ion-segment-button>

        <ion-segment-button >
          <ion-label>
            two
          </ion-label>
        </ion-segment-button>

    </ion-segment>
</ion-toolbar>

for me it solved the issue even before the fix I've noticed it because some of my segments worked while other didn't

bodinaren commented 4 years ago

@yarivgdidi No, they weren't, it does appear fix it though so thanks for that. Still need to try in my real code, just tried on codepen.

That's quite the well concealed piece of requirement though, perhaps the docs can be improved regarding this?

simonhaenisch commented 4 years ago

I don't think it's supposed to be a requirement, is it?

liamdebeasi commented 4 years ago

I tried out the dev build and I still experience the same issue as detailed in #20466 (comment).

I attempted one fix which seems to work at least for this case, on this line... https://github.com/ionic-team/ionic/blob/master/core/src/components/segment/segment.tsx#L305

What happens is when it tries to find nextEl on the next line it gets the ion-header element instead. Instead of using the top point of the previously selected button we use the vertical center of the element like this: const previousY = rect.y + (rect.height / 2);

@bodinaren The code you suggested is what we already do in the dev build (https://github.com/ionic-team/ionic/pull/20554/files#diff-eb82c3be26eed17c0d922817c1dcb05dR308). Are you trying the latest dev build 5.1.0-dev.202002201606.ac98652?

liamdebeasi commented 4 years ago

I don't think it's supposed to be a requirement, is it?

Using ion-segment inside of ion-toolbar is not a requirement.

liamdebeasi commented 4 years ago

Thanks for the issue! As per https://github.com/ionic-team/ionic/issues/20466#issuecomment-589173907 it sounds like this issue has been fixed by the dev build. As a result I have merged my changes in and will be closing out this issue. If you are still experiencing issues, please ensure that you are running dev build 5.1.0-dev.202002201606.ac98652. For any additional bugs, please open a new issue. Thanks!

bodinaren commented 4 years ago

I must have indeed done something wrong when I tested it because with dev build 5.1.0-dev.202002201606.ac98652 it does indeed work. Thanks!

ionitron-bot[bot] commented 4 years ago

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.