dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.25k stars 1.58k forks source link

Export "DomTokenList Element.get.classList" #23012

Open DartBot opened 9 years ago

DartBot commented 9 years ago

This issue was originally filed by localvoi...@gmail.com


Can we get access to DomTokenList Element.get.classList?

When working on a library that implements its own abstraction to manipulate DOM, I would prefer to use api that is close to native DOM api as possible. I understand why it was implemented this way long time ago when dart supported browsers that didn't have classList implementation, but now all dart supported browsers have classList implementation.

Simple benchmark using Dart Element.get.classes and Javascript Element.get.classList: http://localvoid.github.io/dart-classlist-benchmark/

DBMonster Benchmark implementations using uix library:

Element.get.classes: http://localvoid.github.io/uix_dbmon/ Element.get.classList (patched dart-sdk with exported classList): http://localvoid.github.io/uix_dbmon/classlist/

rakudrama commented 9 years ago

By coincidence I am looking into this at the moment.

Like you say, classList is widely supported. First I am going to re-implement Element.classes using classList. This should make a huge improvement as it will avoid creating a Set for each operation.

Then I am going to see if it is possible to get myElement.classes.add() to be compiled to myElement.classList.add(). If I succeed at this, do you think there is still a need for making classList available?

Are you using element.classes.add() or querySelectorAll(...).classes.add() ? It is hard to tell from the minified code :-)


Set owner to @rakudrama. Removed Priority-Unassigned label. Added Priority-Medium, Area-Library, Library-Html, Triaged labels.

rakudrama commented 9 years ago

Added this to the 1.10 milestone.

DartBot commented 9 years ago

This comment was originally written by loca...@gmail.com


I am using something like this:

updateClasses(a, b, classes) {
  final changes = diff(a, b); // in real implementation, diff/patch is implemented in one step
  for (final c in changes) {
    if (c.isAdd) classes.add(c.value);
    else classes.remove(c.value);
  }
}

final a = ['a', 'b'];
final b = ['a', 'c'];
final classes = element.classes;

updateClasses(a, b, classes);

I am using it in virtual dom implementation, so I already have list of old classes and list of new classes, I just need to do a diff on unordered lists of strings and apply changes to the real class list. I am already trying to optimize it by using className instead of classList whenever it is possible, but I can't do this for "Composite Components".

If I succeed at this, do you think there is still a need for making classList available?

I would be happy with any solution that will generate efficient access to classList :)

rakudrama commented 9 years ago

There are performance improvements for Element.classes in the upcoming 1.10 release.

In doing the work I discovered that IE11 does not fully support classList so I am leaving this issue open until we can find a uniform solution.

Other browsers define classList for Element, but IE11 defines it for HTMLElement, so classList is not available for SVGElements, even though you can select them with document.querySelectorAll('.class'). (The same is true of className - the only universal way of setting classes (in JavaScript) is element.attributes['class'] = ...) I reviewed several public polyfills for classList and none of them seem to fix the IE11 SVG problem.


Removed this from the 1.10 milestone.

Aetet commented 7 years ago

@kevmoo Is there will be any progress for performance?

kevmoo commented 7 years ago

Not sure. @rakudrama ?

rakudrama commented 7 years ago

Provided you start with a HtmlElement, you should get efficient code.

So change updateClasses to take a HtmlElement argument and inspect the compiled code. It should be using classList.

ranquild commented 7 years ago

@rakudrama

https://github.com/dart-lang/sdk/blob/20a98246053f60b9f88fe87c9000bf5edf695728/tools/dom/src/dart2js_CssClassSet.dart#L144

https://github.com/dart-lang/sdk/blob/20a98246053f60b9f88fe87c9000bf5edf695728/tools/dom/src/dart2js_CssClassSet.dart#L232

https://github.com/dart-lang/angular2_components/blob/master/lib/src/components/material_ripple/material_ripple.dart#L182

Because of dart wrappers logic, even people at Google are forced to use alternatives. I think that Dart wrappers for Web API should as be thin as possible, without any additional logic, but act as pure proxy to native API.

rakudrama commented 7 years ago

We are trying to avoid exposing classList because it is not fully implemented on all browsers we support.

I agree that a thin API is desirable. This is a change in philosophy since dart:html was written.

We have done work to make this specific issue less of a problem. The comment in material_ripple.dart is wrong. If I change the source to use classes.add like this:

    if (_rippleTemplate == null) {
      final className =
          (supportsAnimationApi) ? '__acx-ripple' : '__acx-ripple fallback';
      _rippleTemplate = new DivElement()..className = className;
    }
--->
    if (_rippleTemplate == null) {
      var e = new DivElement()..classes.add('__acx-ripple');
      if (!supportsAnimationApi) e.classes.add('fallback');
      _rippleTemplate = e;
    }

I get this compiled code:

      if ($._rippleTemplate == null) {
        t1 = document;
        t1 = t1.createElement("div");
        t1.classList.add("__acx-ripple");
        if (!$.$get$supportsAnimationApi())
          t1.classList.add("fallback");
        $._rippleTemplate = t1;
      }

The generated code directly uses classList, like what you would expect if classList was exposed.

The only tricky thing for this optimization to classList.add is that the type of the receiver must be HtmlElement or a subclass, and not Element. The problem with Element is that it includes SvgElement. On IE11, SvgElement does not have classList (http://caniuse.com/#search=classList), so directly using classList would crash. If we exposed classList, we could only expose it in HtmlElement, i.e. with the same.

IE11 had a big market share when we wrote dart:html. We wanted dart to work 'out of the box' so the dart:html library had to contain the polyfill rather than require extra javascript files. Once IE11 is out of the picture, we can make this optimization apply to all Elements, or expose classList without the need for a polyfill in Dart or JavaScript.

@ranquild Do you still support IE11?

Aetet commented 7 years ago

@rakudrama we need fast and fluent api as native as we can. Otherwise we slowdown 2-3 times compare native web-techs. So why we need dart if it is slower than ts or flow by 2 times?

ranquild commented 7 years ago

@rakudrama Tried following code:

import 'dart:html';

void main() {
  HtmlElement element = new DivElement();
  element.classes.add('my-class-name');
}

dart2js compiles it to

main: function() {
      var t1 = document;
      J.get$classes$x(t1.createElement("div")).add$1(0, "my-class-name");
    }
J.get$classes$x = function(receiver) {
    return J.getInterceptor$x(receiver).get$classes(receiver);
  }
get$classes: function(receiver) {
        return new W._ElementCssClassSet(receiver);
      }
_ElementCssClassSet: {
      "^": "CssClassSetImpl;_html$_element",
      readClasses$0: function() {
        var s, t1, t2, _i, trimmed;
        s = P.LinkedHashSet_LinkedHashSet(null, null, null, P.String);
        for (t1 = this._html$_element.className.split(" "), t2 = t1.length, _i = 0; _i < t1.length; t1.length === t2 || (0, H.throwConcurrentModificationError)(t1), ++_i) {
          trimmed = J.trim$0$s(t1[_i]);
          if (trimmed.length !== 0)
            s.add$1(0, trimmed);
        }
        return s;
      },
      writeClasses$1: function(s) {
        this._html$_element.className = s.join$1(0, " ");
      },
      get$length: function(_) {
        return this._html$_element.classList.length;
      },
      add$1: function(_, value) {
        var list, t1;
        list = this._html$_element.classList;
        t1 = list.contains(value);
        list.add(value);
        return !t1;
      }
    },

The important part is this compiled (in the end) to

list = this._html$_element.classList;
        t1 = list.contains(value);
        list.add(value);
        return !t1;

There is extra contains call. But most part part that there is a lot of overhead made by dart2js. Just a side node - if I compiled this code with - m option, code is minified but structure is not changed. In your example your code is compiled without any of those extra calls, how did you do it?

ranquild commented 7 years ago

@rakudrama Sorry for asking you again, but how did you manage to get dart code compiled to

t1 = document;
t1 = t1.createElement("div");
t1.classList.add("__acx-ripple");

without any wrapper functions?

ranquild commented 7 years ago

@rakudrama Used dart2js main.dart --trust-primitives --trust-type-annotations and code is compiled to work you have shown. Issue can be closed now, big thanks!

rakudrama commented 7 years ago

I was investigating the discrepancy and discovered that I was using --trust-type-annotations via a build rule, and was going to reply, but you beat me to it. I decided to make some changes to dart:html so that using --trust-type-annotations will not be necessary to get the effect, at least for new DivElement(). https://codereview.chromium.org/2705213003/