This repository was archived by the owner on Apr 12, 2024. It is now read-only.
test: fix ms edge failures #13686
Closed
test: fix ms edge failures #13686
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| expect(box.width === 0 && box.height === 0).toBe(false); | ||
| var hasWidthOrHeight = (box.width !== 0) || (box.height !== 0); | ||
|
|
||
| if (possiblyUnsizedParent && !hasWidthOrHeight) { |
There was a problem hiding this comment.
this change defeats the porpoise of the test itself
There was a problem hiding this comment.
I was not sure what the purpose of the test was (not to mention it has a comment that says it might be redundant), so I am open to alternative suggestions.
TBH, I am not too familiar with SVG, so I don't really know why MS Edge behaves differently than other browsers (although it theoretically supports foreignObject). It isn't Angular-related, btw, because the same behavior can be reproduced with "raw" HTML.
There was a problem hiding this comment.
I think it is better to put an if (isEdge) condition on the test and do not run the test there at all than this change. I understand that this is an issue with Edge, but we should not try to test something else because of a browser
There was a problem hiding this comment.
I think I am with @lgalfaso on this unless we can explain better why it is happening this way.
There was a problem hiding this comment.
I wrapped the test in if (!isEdge) in gkalpak@f7f4417.
|
I know that this touches several tests, but can you please squash this into one commit? |
|
@lgalfaso, thx for the review. I prefer leaving it as separate commits for now (so it's easier to review related changes in the proper context) and squash upon merging. |
|
Good job! LGTM. |
For whatever reason, MS Edge throws during testing, when trying to set the view value to `9999-12-31T23.59.59`. Other values I tried (including later dates) didn't throw. I also couldn't reproduce it outside of the test.
gkalpak
force-pushed
the
test-fix-ms-edge-failures
branch
from
c7b29e5 to
f7f4417
Compare
|
LGTM - @lgalfaso any further comments? If there are no more comments then let's land this before RC1 |
|
LGTM |
|
Please merge |
Although the detected `vendorPrefix` for MS Edge is "ms", it doesn't seem to recognize some vendor-prefixed CSS rules (e.g. `-ms-animation-*`). Other browsers (currently) recognize either vendor-prefixed rules only or both. This commit fixes this issue by adding and retrieving styles using both prefixed and un-prefixed names.
For unknown reasons, an `<svg>` element inside a `<foreignObject>` element on MS Edge seems to have no size, causing the included `<circle>` element to have no size as well and thus fails an assertion (relying on the element having a non-zero size). This seems to be an MS Edge issue; i.e. it is also reproducible without Angular. (Tested with MS Edge version 25.10586.0.0 on Windows 10.)
gkalpak
force-pushed
the
test-fix-ms-edge-failures
branch
from
f7f4417 to
ff44c1c
Compare
gkalpak
added a commit
to gkalpak/angular.js
that referenced
this pull request
Jan 11, 2016
Includes the following fixes (per component):
* `$sniffer`: Properly determine the expected `vendorPrefix` for MS Edge
* `input`: MS Edge does not support dates with years with more than 4 digits.
Trying to set the value of an `input[datetime-local]` to `9999-12-31T23.59.59.999` throws an
error (probably related to converting the date to one with a year with more than 4 digits,
due to timezone offset).
* `$sanitize`: Fix failing tests on MS Edge
* `$animateCss`: Although the detected `vendorPrefix` for MS Edge is "ms", it doesn't seem to
recognize some vendor-prefixed CSS rules (e.g. `-ms-animation-*`). Other browsers (currently)
recognize either vendor-prefixed rules only or both.
Fixed by adding and retrieving styles using both prefixed and un-prefixed names.
* `$compile`: Skip failing `foreignObject` test on MS Edge.
For unknown reasons, an `<svg>` element inside a `<foreignObject>` element on MS Edge has no
size, causing the included `<circle>` element to also have no size and thus fails an
assertion (relying on the element having a non-zero size).
This seems to be an MS Edge issue; i.e. it is also reproducible without Angular.
(Tested with MS Edge version 25.10586.0.0 on Windows 10.)
Closes angular#13686
gkalpak
added a commit
to gkalpak/angular.js
that referenced
this pull request
Jan 11, 2016
Includes the following fixes (per component):
* `$sniffer`: Properly determine the expected `vendorPrefix` for MS Edge
* `input`: MS Edge does not support dates with years with more than 4 digits.
Trying to set the value of an `input[datetime-local]` to `9999-12-31T23.59.59.999` throws an
error (probably related to converting the date to one with a year with more than 4 digits,
due to timezone offset).
* `$animateCss`: Although the detected `vendorPrefix` for MS Edge is "ms", it doesn't seem to
recognize some vendor-prefixed CSS rules (e.g. `-ms-animation-*`). Other browsers (currently)
recognize either vendor-prefixed rules only or both.
Fixed by adding and retrieving styles using both prefixed and un-prefixed names.
* `$compile`: Skip failing `foreignObject` test on MS Edge.
For unknown reasons, an `<svg>` element inside a `<foreignObject>` element on MS Edge has no
size, causing the included `<circle>` element to also have no size and thus fails an
assertion (relying on the element having a non-zero size).
This seems to be an MS Edge issue; i.e. it is also reproducible without Angular.
(Tested with MS Edge version 25.10586.0.0 on Windows 10.)
Closes angular#13686
gkalpak
added a commit
that referenced
this pull request
Jan 11, 2016
Includes the following fixes (per component):
* `$sniffer`: Properly determine the expected `vendorPrefix` for MS Edge
* `input`: MS Edge does not support dates with years with more than 4 digits.
Trying to set the value of an `input[datetime-local]` to `9999-12-31T23.59.59.999` throws an
error (probably related to converting the date to one with a year with more than 4 digits,
due to timezone offset).
* `$animateCss`: Although the detected `vendorPrefix` for MS Edge is "ms", it doesn't seem to
recognize some vendor-prefixed CSS rules (e.g. `-ms-animation-*`). Other browsers (currently)
recognize either vendor-prefixed rules only or both.
Fixed by adding and retrieving styles using both prefixed and un-prefixed names.
* `$compile`: Skip failing `foreignObject` test on MS Edge.
For unknown reasons, an `<svg>` element inside a `<foreignObject>` element on MS Edge has no
size, causing the included `<circle>` element to also have no size and thus fails an
assertion (relying on the element having a non-zero size).
This seems to be an MS Edge issue; i.e. it is also reproducible without Angular.
(Tested with MS Edge version 25.10586.0.0 on Windows 10.)
Closes #13686
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.