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

Export "DomTokenList Element.get.classList" #23012

Open
DartBot opened this issue Mar 27, 2015 · 14 comments

Comments

Projects
None yet
5 participants
@DartBot
Copy link

commented Mar 27, 2015

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

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

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

This comment has been minimized.

Copy link
Member

commented Mar 27, 2015

Added this to the 1.10 milestone.

@DartBot

This comment has been minimized.

Copy link
Author

commented Mar 27, 2015

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

This comment has been minimized.

Copy link
Member

commented Apr 11, 2015

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

This comment has been minimized.

Copy link

commented Feb 17, 2017

@kevmoo Is there will be any progress for performance?

@kevmoo

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

Not sure. @rakudrama ?

@rakudrama

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

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

This comment has been minimized.

Copy link

commented Feb 19, 2017

@rakudrama

static bool _add(Element _element, String value) {

static bool _classListContainsBeforeAddOrRemove(

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

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

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

This comment has been minimized.

Copy link

commented Feb 21, 2017

@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

This comment has been minimized.

Copy link

commented Feb 21, 2017

@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

This comment has been minimized.

Copy link

commented Feb 24, 2017

@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

This comment has been minimized.

Copy link

commented Feb 24, 2017

@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

This comment has been minimized.

Copy link
Member

commented Feb 24, 2017

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/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.