Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Android only: Broken GridLayout with angular 17.3.0. With 17.2.1 thing were working #129

Open
cjohn001 opened this issue Mar 17, 2024 · 13 comments

Comments

@cjohn001
Copy link

cjohn001 commented Mar 17, 2024

OS: macOS 14.4
CPU: (10) arm64 Apple M1 Pro
Shell: /bin/zsh
node: 20.11.1
npm: 10.2.4
nativescript: 8.6.5

# android
java: 11.0.22
ndk: Not Found
apis: Not Found
build_tools: Not Found
system_images: Not Found

# ios
xcode: 15.3/15E204a
cocoapods: 1.15.2
python: 3.12.2
python3: 3.12.2
ruby: 2.7.8
platforms: 
  - DriverKit 23.4
  - iOS 17.4
  - macOS 14.4
  - tvOS 17.4
  - visionOS 1.1
  - watchOS 10.4

Dependencies

"dependencies": {
  "@angular/animations": "17.3.0",
  "@angular/common": "17.3.0",
  "@angular/compiler": "17.3.0",
  "@angular/core": "17.3.0",
  "@angular/forms": "17.3.0",
  "@angular/platform-browser": "17.3.0",
  "@angular/platform-browser-dynamic": "17.3.0",
  "@angular/router": "17.3.0",
  "@apollo/client": "3.9.7",
  "@mnd/external-web-view": "file:../app-plugins/dist/packages/external-web-view/mnd-external-web-view-1.0.1.tgz",
  "@nativescript/angular": "17.0.0",
  "@nativescript/core": "8.6.2",
  "@nativescript/iqkeyboardmanager": "2.1.1",
  "@nativescript/localize": "5.2.0",
  "@nativescript/mlkit-barcode-scanning": "2.1.0",
  "@nativescript/mlkit-core": "2.1.0",
  "@nativescript/secure-storage": "3.0.3",
  "@nativescript/theme": "3.0.2",
  "@nativescript/ui-charts": "0.2.4",
  "apollo-angular": "6.0.0",
  "apollo3-cache-persist": "0.14.1",
  "d3-ease": "3.0.1",
  "graphql": "16.8.1",
  "graphql-tag": "2.12.6",
  "intl": "1.2.5",
  "moment": "2.30.1",
  "nativescript-health-data": "file:../app-plugins-customized/nativescript-health-data/publish/package/nativescript-health-data-2.0.0.tgz",
  "nativescript-oauth2-ext": "file:../app-plugins-customized/nativescript-oauth2-ext/publish/package/nativescript-oauth2-ext-3.0.3.tgz",
  "nativescript-sqlite": "2.8.6",
  "nativescript-sqlite-commercial": "file:../app-plugins-customized/commercial-sqlite/nativescript-sqlite-commercial-1.8.0.tgz",
  "nativescript-sqlite-encrypted": "file:../app-plugins-customized/commercial-sqlite/nativescript-sqlite-encrypted-1.6.0.tgz",
  "nativescript-ui-calendar": "15.2.3",
  "nativescript-ui-gauge": "15.2.3",
  "qs": "npm:querystring@0.2.1",
  "rxjs": "7.8.1",
  "util": "0.12.5",
  "uuidjs": "5.0.1",
  "zone.js": "0.14.4"
},
"devDependencies": {
  "@angular-devkit/build-angular": "17.3.0",
  "@angular/compiler-cli": "17.3.0",
  "@graphql-codegen/cli": "5.0.2",
  "@graphql-codegen/fragment-matcher": "5.0.2",
  "@graphql-codegen/introspection": "4.0.3",
  "@graphql-codegen/typescript": "4.0.6",
  "@graphql-codegen/typescript-apollo-angular": "4.0.0",
  "@graphql-codegen/typescript-operations": "4.2.0",
  "@nativescript/android": "8.6.2",
  "@nativescript/ios": "8.6.4",
  "@nativescript/types": "8.6.1",
  "@nativescript/webpack": "5.0.18",
  "@ngtools/webpack": "17.3.0",
  "@types/d3-ease": "3.0.2",
  "@types/intl": "1.2.2",
  "@types/lodash": "4.17.0",
  "@types/node": "20.11.28",
  "keycloak-request-token": "0.1.0",
  "rimraf": "5.0.5",
  "sass": "1.72.0",
  "ts-node": "10.9.2",
  "typescript": "5.2.2"
}

Describe the bug
I am having a grid layout showing tiles see images attached, those are set up using the following code snipped

<GridLayout #idBaseGridLayout class="mnd" iosOverflowSafeArea="true" [paddingBottom]="BOTTOM_BAR_HEIGHT" [marginLeft]="_isTablet ? '60' : '0' ">
  <GridLayout #idInnerGridLayout col="0" row="0" ios:columns="*,10,*" ios:rows="*,10,*,10,*"
    android:columns="{{(_tinyScreen || _smallScreen) ? '*,5,*' : '*,10,*'}}" android:rows="{{(_tinyScreen || _smallScreen) ? '*,5,*,5,*' : '*,10,*,10,*' }}"
    class="{{(_tinyScreen || _smallScreen) ? 'tiles-padding-small-screen' : 'tiles-padding' }}" iosOverflowSafeArea="false">

When moving from angular 17.2.1 to 17.3.0 this layout breaks because of the following line

android:columns="{{(_tinyScreen || _smallScreen) ? '*,5,*' : '*,10,*'}}" android:rows="{{(_tinyScreen || _smallScreen) ? '*,5,*,5,*' : '*,10,*,10,*' }}

_tinyScreen and _smallScreen are booleans both set to false. When I set columns and rows to a fixed value things are working as expected.

I also tried to change the property binding to the following line, but this also does not work:

[android:columns]="(_tinyScreen || _smallScreen) ? '*,5,*' : '*,10,*'" [android:rows]="(_tinyScreen || _smallScreen) ? '*,5,*,5,*' : '*,10,*,10,*' "

Screenshot_1710698377

Screenshot_1710699405

To Reproduce

Expected behavior

Sample project

Additional context

@cjohn001 cjohn001 changed the title Android only: Broken GridLayout with angular 17.3.0 on 17.2.1 thing were still working Android only: Broken GridLayout with angular 17.3.0. With 17.2.1 thing were working Mar 17, 2024
@edusperoni
Copy link
Collaborator

Hello!

Can you please enable the NativeScript Angular logs (set Trace.enable() in your polyfills.ts, and enable the NativeScriptDebug.rendererTraceCategory).

You should be seeing this log on the screen:

NativeScriptDebug.rendererLog(`NativeScriptRenderer.setAttribute ${namespace ? namespace + ':' : ''}${el}.${name} = ${value}`);

I suspect this issue comes from the angular side, not our side, so most likely that log will be setting the string value itself instead of evaluating the expression. The NativeScript angular integration doesn't evaluate any expressions, we just apply what angular provides to us via the Renderer2 API

@cjohn001
Copy link
Author

cjohn001 commented Mar 17, 2024

Hello @edusperoni , not sure how I have to do it. could you please explain what I have to set in polyfills.ts.
So far I only used Trace by putting the following code into app.module.ts

import { Trace } from '@nativescript/core';
Trace.addCategories(Trace.categories.Style)
Trace.enable();

However, there seems to be no category NativeScriptDebug.rendererTraceCategory

@cjohn001
Copy link
Author

cjohn001 commented Mar 17, 2024

@edusperoni, I tried to set
Trace.addCategories(Trace.categories.All)
as I was not able to import NativeScriptDebug

import { NativeScriptDebug } from '@nativescript/angular/lib/trace';
Trace.addCategories(NativeScriptDebug.rendererTraceCategory);
Trace.enable();

and I also tried to set it as string Trace.addCategories('NativeScriptDebug.rendererTraceCategory').

The later does not work and the first one does not result in any output of NativeScriptDebug.rendererLog.

I finally also tried to set
import { Trace } from '@nativescript/core';
Trace.enable();

in src/polyfill.ts, but I do not get any NativeScriptRenderer.setAttribute output either.

Please provide a more detailed description how I can enable it.

@edusperoni
Copy link
Collaborator

The NativeScriptDebug is exported as ɵNativeScriptAngularDebug. In any case, you can enable the category 'ns-renderer' instead

@cjohn001
Copy link
Author

I now did try both variants in app.module.ts, but I do not see any output

import { Trace } from '@nativescript/core';

import { ɵNativeScriptAngularDebug } from '@nativescript/angular';
Trace.addCategories(ɵNativeScriptAngularDebug.rendererTraceCategory);
Trace.enable();

and Trace.addCategories('ns-renderer');
Trace.enable();

@cjohn001
Copy link
Author

cjohn001 commented Mar 17, 2024

ok, got it. Had to go into polyfills.ts

It looks like the expression is evaluated, see attached file line 1404 and 1405

ns-renderer: NativeScriptRenderer.setProperty GridLayout(107).:android:columns = *,10,*
ns-renderer: NativeScriptRenderer.setProperty GridLayout(107).:android:rows = *,10,*,10,*

This looks correct?

It looks awkward to me ":android:"

out.txt

Update: I just compared it against the old NS Angular version 17.2.1 there it looks different

 ns-renderer: NativeScriptRenderer.setProperty GridLayout(107).columns = *,10,*
 ns-renderer: NativeScriptRenderer.setProperty GridLayout(107).rows = *,10,*,10,*

Seems like the android prefix is broken? Should be removed?

@edusperoni
Copy link
Collaborator

@cjohn001 is there any calls to setAttribute on the old one? Seems like the new version isn't calling setAttribute with the namespace.

Seems like angular might have changed the way it stets these kinds of properties. I'll take a look later if this is something it makes sense for us to support or not.

In the meantime I highly recommend using a function/getter instead and bind that to the colums/rows, as it'll avoid multiple evaluation calls too (right now your code will process row/columns for both platforms, but only select one of them). Something like:

get rows() {
  if(isAndroid) {
     if(this._tinyScreen....) { ... }
     return '....';
   }
   return '....';
}

@cjohn001
Copy link
Author

cjohn001 commented Mar 17, 2024

@edusperoni you mean if I am calling setAttribute? No.

In regards to the android: ios: you do not want to support it anymore? In that case I would have to check my entire code for it. Use it for multiple attributes..

One more question to this regarding efficiency. I think to remember to have red that I also should not call functions from templates when possible as those are to be evaluated for each redraw. I am therefore wondering if the approach you provided is more efficient. Or should I maybe use a signal for it? Seems like those somtimes work and sometimes do not. I observed them not to refresh. However, here refresh is not required as the platform never changes

@edusperoni
Copy link
Collaborator

it's not that we don't want to support it anymore, it's that angular is changing how it sets those, so we'd need to work around it. Ever since I wrote the tests for it, dynamic values didn't work properly (while static values like you tried, work, because properties don't have namespaces, while attributes do). I personally don't use that feature for dynamic values because of that, and most of the time it's better to use a getter or function instead.

@cjohn001
Copy link
Author

Ok, than I better change my code. Would be good to skip this feature than with a new major release. How about using a signal instead. I think you had answered before I finished updating my question :)

@cjohn001
Copy link
Author

And maybe a last question to the topic. I am wondering why this stuff works on ios and not on android. I thought the same angular library is used on both platforms. Do you have an idea why this could be?

@edusperoni
Copy link
Collaborator

it seems like your code on iOS is static, while android has dynamic content. So most likely angular itself is using setAttribute (correctly) on iOS and setProperty on Android (which has no namespace support)

@paul-staskiewicz
Copy link

It also affects platform-specific css (https://docs.nativescript.org/guide/styling#platform-specific-css) when used with dynamic values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants