Do not merge attributes that are the same #7463
Conversation
|
Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
|
I just noticed that the Travis build failed. When I run the protractor tests (as well as all other tests) locally it passes. Is there a way I can re-run Travis without doing another push? |
|
don't worry about it, that's a clear flake, and restarting it now will probably flake again (saucelabs tends to have these sort of bumpy periods of badness). It looks like it's okay |
|
I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS. Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match. If you signed the CLA as a corporation, please let us know the company's name. Thanks a bunch! PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR. |
|
I have signed the CLA. Let me know if you need anything else, Ms. Poppins. |
…tives If a directives specifies `replace:true` and the template of the directive contains a root element with an attribute which already exists at the place where the directive is used with the same value, don't duplicate the value. Closes #7463
|
@tbosch, please forgive my ignorance, but why was this closed? |
|
@jeffwhelpley it was closed from the commit 1ab6e90) which means it was merged. See https://help.github.com/articles/closing-issues-via-commit-messages for more info. |
…tives If a directives specifies `replace:true` and the template of the directive contains a root element with an attribute which already exists at the place where the directive is used with the same value, don't duplicate the value. Closes angular#7463
|
Ah, awesome! Thanks. |
Request Type: bug
How to reproduce: 1. have a replace directive with 'id' attribute in the main wrapping element
2. have an element that references the directive and has an 'id' attribute with a value that exactly matches the 'id' attribute from the main wrapping element in the directive
3. when you view this html, you will see that the id attribute was duplicated (i.e. id="myid" becomes id="myid myid")
Component(s): $compile
Impact: small
Complexity: small
This issue is related to:
Detailed Description:
The is a very simple issue. The logic for merging attributes should not concatenate if the attribute values are the same. I would imagine that not a lot of people run into this because
The use case where this starts to be a problem is when you are trying to have a nicer initial page load experience. Basically, for various situations, I will have the server side render default html that is displayed by the browser before angular is fully downloaded and bootstrapped. So, this type of issue would only affect people concerned with server side rendering (either for SEO purposes or for UX so user sees something very quickly before Angular bootstraps). It only affects the initial page load. For people like me that are concerned about these types of things, it is a significant issue and this very minor change will help tremendously.
Other Comments:
The logic for merging attributes will essentially concatenate values if an attribute is in the DOM element and is in the partial that will be replacing the DOM element. This doesn't make sense in some use cases, but the most clear one is where there is something like an ID which is the exact same in both. The current logic will concatenate
<div id="myid" my-directive>and<div id="myid">(for the directive template) into<div id="myid myid">.This pull request is to add one very simple condition that will just ensure that when the values are exactly the same, they are not concatenated.