angulardart-community / angular_components

High-quality Material Design components for AngularDart.
https://gallery.angulardart.xyz
20 stars 15 forks source link

Potential issues with Popups #72

Closed GZGavinZhao closed 2 years ago

GZGavinZhao commented 2 years ago

This only applies to the v2.0.0-dev.X series alpha release (I.e. the dev and new-dev branches). Please try using these branches in your projects and tell us whether this is indeed an issue!

In my opinion, the current null safety implementation of ZIndexer has a caveat:

@JS()
library angular_components.css.acux.zindexer;

import 'package:angular/angular.dart';
import 'package:js/js.dart';

@JS('acxZIndex')
external int get _currentZIndex;

@JS('acxZIndex')
external set _currentZIndex(int value);

/// The layout tools will monotonically increment the zIndex for hoverable
/// elements.
const int hoverableAutoIncrement = 1000;

/// This allows a monotonically increasing z-index for hoverable elements. This
/// works around the problem where setting a static z-index for newly shown
/// elements will sometimes show up underneath existing elements.
@Injectable()
class ZIndexer {
  static ZIndexer? _currentInstance;

  static void _initZIndex() {
    if (_currentZIndex == null) {
      _currentZIndex = hoverableAutoIncrement;
    }
  }

  factory ZIndexer() {
    return _currentInstance ??= ZIndexer._();
  }

  ZIndexer._() {
    _initZIndex();
  }

  /// Increment and get the current z-index.
  int pop() => ++_currentZIndex;

  /// Peek at the current z-index without changing it.
  int peek() => _currentZIndex;
}

In the function _initZIndex(), _currentZIndex can never be null. However, according to testing with the non-null-safety version (the default template project created by ngdart is enough; after entering an item, hover above the checkbox), when the first time ZIndexer is used, _currentZIndex is definitely null. Therefore, _currentZIndex is never set to hoverableAutoIncrement and just seems to be... a random number? However, I don't see any errors or weird behaviors when I test the current implementation.

Just putting this problem out here for now. I will do some further investigation tomorrow.

Affected components:

GZGavinZhao commented 2 years ago

Fixed by 73bea40aaf88656f563fa6a7eba07e5488cfd5f8. Through my testing, _currentZIndex is never null as long as ZIndexer has been used (through the Angular DI). The solution is to add another variable to check whether ZIndexer is used and set _currentZIndex accordingly.