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.

Sign up for GitHub

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

Jump to bottom

New test harness for attributes test #564

Closed
wants to merge 7 commits into from
Closed

New test harness for attributes test #564

wants to merge 7 commits into from

Conversation

Serabe
Copy link
Member

@Serabe Serabe commented Jul 3, 2017

No description provided.

@Serabe Serabe requested a review from chadhietala July 3, 2017 22:28
@Serabe
Copy link
Member Author

Serabe commented Jul 3, 2017

@chadhietala I would appreciate a first input on this. It is mostly the harness and the renderTemplate implementation was done mostly by imitation, so any input would be more than welcomed.

@Serabe
Copy link
Member Author

Serabe commented Jul 4, 2017

#561

chadhietala
chadhietala reviewed Jul 7, 2017
View reviewed changes
packages/@glimmer/runtime/test/attributes-test.ts Outdated
constructor(env = new TestEnvironment()) {
super(env);
this.element = env.getDOM().createElement('div') as HTMLDivElement;
//this.element.setAttribute('debug-root', 'true');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove ✂️

packages/@glimmer/runtime/test/attributes-test.ts Outdated
equalTokens(root, '<input disabled />');

rerender({ enabled: null });
abstract class RenderTest extends BaseRenderTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged #560 so I'm not sure if you need this any more.

packages/@glimmer/runtime/test/attributes-test.ts Outdated

equalTokens(root, '<a href="http://foo.bar"></a>');
});
@test "a[href] marks javascript: protocol as unsafe on updates"() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be covered at L190

@Serabe Serabe force-pushed the tests/attributes-helper branch from a3461ae to 0bbbcad Compare July 9, 2017 13:40
@Serabe Serabe changed the title [WIP] New test harness for attributes test New test harness for attributes test Jul 9, 2017
@Serabe
Copy link
Member Author

Serabe commented Jul 9, 2017

@chadhietala this is ready for review. The tests related to sauce labs are failing (the four of them). Shall I just restart job like in Ember or is there anything I am doing wrong here?

OTOH, is there any way to remove the TS2300 and related errors?

Thank you!

@Serabe Serabe mentioned this pull request Jul 9, 2017
Closed
9 tasks
@chadhietala
Copy link
Member

@Serabe I think there might be legit errors here with IE. I think you might need to write some type of condition for IE like I did here looks like textarea doesn't behave the same. What is the TS2300 error?

@chadhietala
Copy link
Member

@Serabe this looks great otherwise.

@Serabe
Copy link
Member Author

Serabe commented Jul 10, 2017

TS2300 and related errors are warnings from TypeScript when it finds a duplicated definition. It seems to be coming from the type declaration of es2015 and such. It raises so many that Travis log is hard to use.

I've added the select quirk and I'm looking for the one for textarea.

Thank you!

@Serabe Serabe force-pushed the tests/attributes-helper branch from b106536 to 6ec1b27 Compare July 10, 2017 23:06
@Serabe
Copy link
Member Author

Serabe commented Jul 11, 2017

0003screenshot

Seems that nodes are not stable in "handles empty string textarea values" after setting the value to Alex

@Serabe
Copy link
Member Author

Serabe commented Jul 11, 2017

2017-07-11 at 16 59

The errors I mentioned were these. There are over 8.5K lines of this bloating travis log :$

@Serabe
Copy link
Member Author

Serabe commented Jul 23, 2017

I've been a few hours digging into this in IE11 and cannot find anything :(

Any help available figuring this out?

@chadhietala
Copy link
Member

chadhietala commented Jul 24, 2017 •
edited
Loading

@krisselden I checked on a VM of IE and it appeared IE and Edge do different things with CDATA in a textarea c/d?

@Serabe
Copy link
Member Author

Serabe commented Sep 1, 2017

@chadhietala anything I can do to help fix this?

@Serabe
Copy link
Member Author

Serabe commented Oct 4, 2017

@chadhietala @krisselden ping?

@chadhietala
Copy link
Member

@Serabe we removed IE9/IE10/Pantom from master. Can you try to rebase and see if the tests just pass?

@Serabe Serabe force-pushed the tests/attributes-helper branch from f9e0d57 to b098cd3 Compare October 6, 2017 11:20
@Serabe
Copy link
Member Author

Serabe commented Oct 6, 2017

Done. Still failing IE11 and Edge.

@locks
Copy link
Contributor

locks commented Oct 9, 2017

@Serabe You've Got Conflicts™.

@Serabe
Copy link
Member Author

Serabe commented Oct 9, 2017

@locks already in Berlin, not sure when I'll be able to rebase again, but the underlying issue is still there.

@chadhietala
Copy link
Member

Ok I have an answer. Firstly value is not a valid html attribute for textarea. Yet this still works if you set the prop, which is what we do today. That being said it appears that browsers differ how they represent the text. In Chrome you don't see a text node with the value instead you just see the following:

screen shot 2017-10-09 at 5 05 04 pm

Notice it does not have a firstChild. Now with IE and Edge if we start off with an empty string or null we have a similar behavior.

screen shot 2017-10-09 at 5 18 52 pm

However when we do update the textarea we have the following:

screen shot 2017-10-09 at 5 05 42 pm

So IE and Edge inject a TextNode when value is set.

Bold What Shall We Do???
Well because the result is the same between these browsers but the implementation details are slightly different we should just not assertStableNodes as they are not going to be stable. Instead we should just keep the assertion that the value updated. I can rebased and fix this if you don't have the time.

@chadhietala chadhietala mentioned this pull request Oct 10, 2017
Merged
@chadhietala
Copy link
Member

Merged as part of #698. Sorry this took so long @Serabe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chadhietala chadhietala chadhietala left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Serabe @chadhietala @locks