bwu-dart / bwu_datagrid

A data-grid Polymer element in Dart
MIT License
73 stars 26 forks source link

Random Uncaught Unhandled exception when clicking on column headers #144

Closed tonosama-atlacatl closed 8 years ago

tonosama-atlacatl commented 8 years ago

I can't seem to pin-point the culprit, but this is happening at random... I made sure to use CSS classes for my columns' headers. This is the error I'm seeing in Chromium: screen shot 2016-06-06 at 7 11 50 pm After this error, It takes a few seconds for subsequent clicks to sort correctly.

I'm on: SDK 1.15.0 OS X 10.11.5 from pubspec.lock: bwu_datagrid: description: ref: polymer1 resolved-ref: "627bba819a9db00aa186567b74a58e6b6d7df9d8" url: "https://github.com/bwu-dart/bwu_datagrid.git" source: git version: "0.1.0-dev.1"

zoechi commented 8 years ago

I got a few similar errors when I tested recently with most recent Dart and Polymer version. The ones I encountered should be fixed. I'll try to get a new release out soon. It would be great if you then can test again and report back if this is still an issue.

tonosama-atlacatl commented 8 years ago

I'm still seeing this issue, very intermittent although. I upgraded to: bwu_datagrid: description: ref: polymer1 resolved-ref: "7d2e14e693512d510c1855971528d35b606ec43f" url: "https://github.com/bwu-dart/bwu_datagrid.git" source: git version: "0.1.0-dev.1"

I'm also getting this warning, but I'm not sure if this is related to the theme changes you're making. screen shot 2016-06-07 at 3 52 42 pm

zoechi commented 8 years ago

I'm going to close this one. If you can still reproduce in 1.0.0-dev.1 please add a comment to get it reopened.

tonosama-atlacatl commented 8 years ago

I'm still seeing this after upgrading to: bwu_datagrid: description: ref: polymer1 resolved-ref: c17b849c0aba06d9aa24718481d4119bce9f483e url: "https://github.com/bwu-dart/bwu_datagrid.git" source: git version: "0.1.0-dev.1"

zoechi commented 8 years ago

In which browsers did you try it?

zoechi commented 8 years ago

If it's about this message, I don't think there is something I can do about it. This is generated by Polymer. image

You still get this exceptions? image

tonosama-atlacatl commented 8 years ago

Ok, thanks for checking those warnings. Yup, I'm affraid I'm still getting those exceptions..randomly.. I'm also getting them on Chromium..will update with others soon...

Here is what I'm seeing in Safari: screen shot 2016-06-08 at 1 16 46 pm

FF: screen shot 2016-06-08 at 1 17 38 pm

Chrome: screen shot 2016-06-08 at 1 22 14 pm

pub_upgrade_output.txt pubspec.yaml.txt

tonosama-atlacatl commented 8 years ago

Ok, I think I figured out how to successfully reproduce this issue...this only happens when I click on the column header's text.... then I get this error. If I click anywhere else on the header it works fine and the column sorts correctly...

And just to make sure, I specified some values for ".bwu-datagrid-column-name".

gtaylor20 commented 8 years ago

I think the "Unhandled exception for the 'StyleElementImpl' " error is coming from your utils.closest function, where you do a querySelectorAll. In Polymer 1.0, now, the style element also has the .bwu-datagrid-header-column class. I did not do a detailed analysis of your code, but I think this is the culprit. I hope this helps.

zoechi commented 8 years ago

@gtaylor20 thanks a lot for investigating and your feedback. I'll check it.

gtaylor20 commented 8 years ago

I meant to say the "style" element also has the .bwu-datagrid-header-column class.

gtaylor20 commented 8 years ago

I was able to resolve this particular error by doing the following -- In utils.closest, I appended ":not(.style-scope)" to the selector being passed. This will ignore the style elements that were causing the problem.
Of course, you'll have to ensure that this works for all of your uses of this function, but it does seem to fix this particular one. I hope this helps.

zoechi commented 8 years ago

Sure this helps. I hope to find time tomorrow to check it and hopefully publish an update.

zoechi commented 8 years ago

Is this with a custom application or also with one of the examples? I couldn't reproduce yet.

gtaylor20 commented 8 years ago

This is a custom application. I've tried running your examples, like #13, and it doesn't have the problem. Remember we are using the polymer1 branch. So, I opened "dev tools" to inspect the elements. Here is what I see for a column header in our application:

<bwu-datagrid-header-column class="ui-state-default bwu-datagrid-header-column bwu-datagrid-header-sortable"
                            id="6714f569-14f5-48df-b64c-3fca981d38d5" title=""
                            ismovable="false" style="width: 112px;">
  <style is="custom-style" bwu-datagrid-theme="bwu-datagrid-default-theme"
         include="bwu-datagrid-default-theme-column-header"
         class="style-scope bwu-datagrid-header-column"></style>
  <span class="hidden style-scope bwu-datagrid-header-column" id="theme-placeholder"></span>
  <span class="bwu-datagrid-column-name">Bandwidth</span>
  <span class="bwu-datagrid-sort-indicator"></span>
  <div class="bwu-datagrid-resizable-handle" nonsortable="true" bwu-draggable="true"></div>
</bwu-datagrid-header-column>

Here is what I see in your #13 example:

<bwu-datagrid-header-column class="ui-state-default bwu-datagrid-header-column bwu-datagrid-header-sortable"
                            id="c1" title="" ismovable="true" style="width: 231px;">
  <span class="bwu-datagrid-column-name">Sort 1</span>
  <span class="bwu-datagrid-sort-indicator"></span>
  <div class="bwu-datagrid-resizable-handle" nonsortable="true" bwu-draggable="true"></div>
</bwu-datagrid-header-column>

I don't know what causes the difference. Hopefully, you do. 😃

zoechi commented 8 years ago

Did you use the online example? These are still from the Polymer 0.16 version. You would need to run one from the example_/web directory for the source and then check if you can reproduce or if the headers are looking differently. (run pub get before that in example_ and comment out the path dependencies in dev_dependencies (bwu_webdriver, bwu_docker, webdriver)

gtaylor20 commented 8 years ago

No, I ran the polymer1 branch version from my downloaded folder. Yes, I did a pub get and commented out the devdependencies in order to get it to work. The HTML from #13 above was from the example/web folder.

One odd thing, though (and I'm using IntelliJ WebStorm) -- when I run index.html in your example_/web directory, it opens the page in Chrome instead of Chromium as it normally does. I'm not sure why that is.

gtaylor20 commented 8 years ago

Aha! I found the difference in our 2 tests. Your index.html has the following line:

I don't have that line. When I comment it out from your index.html, and click on the header text, I get the same error. Looking at the internal HTML, it now looks like mine, too.

It seems like you need to take into account both ways of running Polymer 1.0.

zoechi commented 8 years ago

There are issues with the way I use themes with shady DOM (default). Therefore the grid requires shadow DOM to be enables until either they fix the webcomponents polyfills or the browsers support shadow DOM natively (whatever comes first). Polymer doesn't provide any support for dynamically generated content. CSS variables and mixins are not enough :-/ I'm pretty sure I mentioned this somewhere (README or so). I'll check an try to make this more prominent.

gtaylor20 commented 8 years ago

Hmmm... I'm not sure what the effects are going to be on our very large application if we start running with shadow DOM. We're going to have to look into it. Thanks.

Are themes the only problem that you've seen with shady DOM? Our simple theme seemed to be okay, at least with what we're trying to do today.

BTW, I just did a very quick look and didn't see any mention of "shadow" in the README, etc. But, I didn't look everywhere.

Thanks for all the help.

gtaylor20 commented 8 years ago

Another quick question -- if bwu_datagrid requires shadow DOM, and our overall application requires shady DOM, is there a way to create a wrapper element around bwu_datagrid that uses shadow DOM, while our overall application still uses shady DOM? Did this question make sense? Thanks.

zoechi commented 8 years ago

As far as I know this setting is only available globally. Applications that work with shady DOM should work the same with shadow DOM (the other way around this is not the case).

The main disadvantage of enabling shadow DOM is that the polyfills are quite slow on Safari mobile, maybe a bit slower on IE and Firefox as well but not so significant as on Safari.

I could check again. Maybe they fixed the issue in the shady DOM polyfills in the meantime. The issue I reported wasn't updated or closed yet, this is why I didn't check again.

zoechi commented 8 years ago

With shady DOM (shadow DOM not enabled) I can reproduce the exception

image

gtaylor20 commented 8 years ago

I'm glad you can recreate the problem that we've seen.

What was the theme-related issue that you reported for shady DOM? Can you provide a link?

If themes are the only issue with shady DOM, and we're not seeing any problems, perhaps we can run with shady DOM by using our "fix" to utils.closest. Would it be practical to put in an optional mode of running in shady DOM, but with some caveats on themes?

zoechi commented 8 years ago

I tested a bit with shady DOM and it doesn't look too bad. I'm considering supporting shady DOM. I guess Chrome won't have much problems with shady DOM anyway. The one exception is solveable. I need to test more on Firefox and IE.

This is the issue I mentioned https://github.com/Polymer/polymer/issues/2681

gtaylor20 commented 8 years ago

That would be awesome, thanks!

zoechi commented 8 years ago

I looked a bit into it and it looks like an ugly rathole I don't want go down. This would require me to use the Polymer DOM API everywhere to ensure everything works properly and that is just too cumbersome. Shady DOM is just a temporary hack anyway until all browsers provide native support for shadow DOM. Firefox and Webkit status are "in development" http://caniuse.com/#search=shadow%20dom

zoechi commented 8 years ago

Not planned because it would require to support shady DOM.