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

(fix) Fix the href not getting removed (#1943) #1965

Closed
wants to merge 21 commits into from
Closed

(fix) Fix the href not getting removed (#1943) #1965

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
bf6abc2
(fix) Fix the href not getting removed (#1943)
cristianbote Oct 2, 2019
419d23c
Switch to undefined check
cristianbote Oct 3, 2019
e5a2f65
Merge branch 'master' into fix/skip-setter-for-href
cristianbote Oct 3, 2019
aca4d2c
Update test case to be more relevant
cristianbote Oct 3, 2019
b996af1
Remove test cases for border
cristianbote Oct 3, 2019
6851acd
Actually keep the test and update the expect value
cristianbote Oct 3, 2019
5711dc9
Revert the if case and added the attribute removal as well
cristianbote Oct 3, 2019
6d48411
Merge branch 'master' into fix/skip-setter-for-href
cristianbote Oct 4, 2019
8b56387
Merge branch 'master' into fix/skip-setter-for-href
cristianbote Oct 7, 2019
686181a
sCU shouldn't block forceUpdate from child (#1984)
marvinhagemeister Oct 7, 2019
1583c4e
(fix) - should reorder memoized children (#1980)
JoviDeCroock Oct 7, 2019
90ad886
Add Songsterr to Real-World Apps section (#1972)
followdarko Oct 8, 2019
d4bf46d
Append portal node to container instead of prepend (#1971)
toraora Oct 8, 2019
10a1d0a
Skipped the removal for value and checked
cristianbote Oct 8, 2019
125410d
Added doc comments
cristianbote Oct 8, 2019
7397b46
Merge branch 'master' into fix/skip-setter-for-href
cristianbote Oct 8, 2019
be852af
Reverted component change
cristianbote Oct 8, 2019
18cb895
Added a better doc line
cristianbote Oct 8, 2019
9434a06
Merge branch 'master' into fix/skip-setter-for-href
cristianbote Oct 15, 2019
c687b41
Update src/diff/props.js
cristianbote Oct 20, 2019
75b2f0d
Merge branch 'master' into fix/skip-setter-for-href
cristianbote Oct 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/diff/props.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function diffProps(dom, newProps, oldProps, isSvg, hydrate) {
let i;

for (i in oldProps) {
if (!(i in newProps)) {
if (newProps[i] === undefined) {
setProperty(dom, i, null, oldProps[i], isSvg);
}
}
Expand Down Expand Up @@ -96,7 +96,19 @@ function setProperty(dom, name, value, oldValue, isSvg) {
&& !isSvg
&& (name in dom)
) {
dom[name] = value==null ? '' : value;
if (value==null) {
// This condition here guards against modifying a unonctrolled input's value
cristianbote marked this conversation as resolved.
Show resolved Hide resolved
if (name!=='value' && name!=='checked') {
Copy link
Member

Choose a reason for hiding this comment

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

ooooh crap I hadn't thought of this implication. There are a bunch more names here that would have to be disabled, I think this might be a sign that we need to try different overall solutions...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, you're right. I'm gonna start looking at it from other angles

// Set the value to empty string first
dom[name] = '';

// And remove the attribute entirely
dom.removeAttribute(name);
}
}
else {
dom[name] = value;
}
}
else if (typeof value!=='function' && name!=='dangerouslySetInnerHTML') {
if (name!==(name = name.replace(/^xlink:?/, ''))) {
Expand Down
11 changes: 9 additions & 2 deletions test/browser/render.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,11 +301,11 @@ describe('render()', () => {

test('3');
test(null);
expect(scratch.innerHTML).to.equal('<div><input><table border=""></table></div>', 'for null');
expect(scratch.innerHTML).to.equal('<div><input><table></table></div>', 'for null');

test('4');
test(undefined);
expect(scratch.innerHTML).to.equal('<div><input><table border=""></table></div>', 'for undefined');
expect(scratch.innerHTML).to.equal('<div><input><table></table></div>', 'for undefined');
});
}

Expand Down Expand Up @@ -999,6 +999,13 @@ describe('render()', () => {
expect(scratch.innerHTML).to.equal('<div>foo</div>');
});

it('should remove old attributes that are undefined', () => {
render(<a href={'/'} />, scratch);
render(<a />, scratch);

expect(scratch.firstElementChild.hasAttribute('href')).to.be.false;
});

// see preact/#1327
it('should not reuse unkeyed components', () => {
class X extends Component {
Expand Down