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.

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

Use removeAttribute to forcefully remove properties from the DOM #1510

Merged
merged 1 commit into from
Jan 29, 2016
Merged

Use removeAttribute to forcefully remove properties from the DOM #1510

merged 1 commit into from
Jan 29, 2016

Conversation

syranide
Copy link
Contributor

Proper implementation of #1448, because now there's something factual to discuss.

The DOM now statelessly reflects the props provided to DOM components. No more getDefaultValueForProperty. "", null and undefined no longer behave differently depending on the value being an attribute or property, with "" being an actual value and distinct from null and undefined. I would dare say that this is the correct fix for #1431.

As is mentioned and benchmarked in the #1448, there should be a theoretically measurable impact of this (in some browsers). But I dare anyone to suggest a reasonable real-life use-case where it is actually at all measurable (unless my benchmark is flawed).


If we want to push the performance of edge-browsers to the limit, we could conditionally make the default type be attribute (instead of property) as FF and Chrome perform significantly better, but they're basically an order of magnitude faster already making it quite pointless (and fragile) IMHO.

Basic tests added, removed an existing test (that has been half-broken) as it no longer makes sense.

   raw     gz Compared to master @ 32b84a4c5ea32835b93a857cf00f0db86d6c755a
     =      = build/JSXTransformer-previous.js
     =      = build/JSXTransformer.js
     =      = build/react-previous.min.js
     =      = build/react-test.js
  -580   -172 build/react-with-addons.js
  -151    -60 build/react-with-addons.min.js
  -580   -169 build/react.js
  -150    -53 build/react.min.js

// Boolean properties set to false removes the attribute from the DOM.
// Additionally, using removeAttribute to clear boolean properties can
// apparently prevent related side-effects from occuring, example:
// selectNode.removeAttribute('multiple') might not update value
Copy link
Collaborator

Choose a reason for hiding this comment

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

might not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

selectNode.removeAttribute('multiple') is apparently not guaranteed to correctly update "option.selected"

Makes more sense?

I found this while running the tests, removing the attribute for some reason does not update selected for the affected options, causing multiple to still be selected (even though it should not be allowed at that point).

Copy link
Collaborator

Choose a reason for hiding this comment

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

That wording makes sense but it seems quite strange to me.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@zpao
Copy link
Member

zpao commented Sep 2, 2014

@syranide, can you rebase? Based on @sebmarkbage adding that tag, I guess he's ready for this.

@syranide
Copy link
Contributor Author

syranide commented Sep 2, 2014

@zpao Will do, tomorrow.

@syranide
Copy link
Contributor Author

syranide commented Sep 3, 2014

@zpao Rebased. However!

scrollTop and scrollLeft does not have associated attributes #2140 (are there other property-only names we support?), I'm not 100% sure why they're exposed. I could add a HAS_NO_ATTRIBUTE flag to take care of it, but what is the expected behavior? Set them to null or leave them as-is?

@sophiebits
Copy link
Collaborator

Sorry, what's wrong with DOMAttributeNames as it is now?

@syranide
Copy link
Contributor Author

syranide commented Sep 4, 2014

@spicyj Nothing, I had missed that it's automatically filled with lower-cased names. :)

@syranide
Copy link
Contributor Author

"Kind of waiting" for #2202

@syranide
Copy link
Contributor Author

@zpao Apart from breaking (the already broken) scrollLeft and scrollTop (#2202) and triggering broken select-behavior (#2241) this PR should be good to go. But we probably want to wait for #2241 as a test otherwise fails.

document.body.innerHTML = '<form enctype="multipart/form-data">';
document.body.firstChild.removeAttribute('enctype');
console.log(document.body.firstChild.enctype);
> "application/x-www-form-urlencoded" 

Correctly restores the property value in all browsers I have my hands on, as you would expect.

@syranide
Copy link
Contributor Author

#2202 merged. Waiting for #2241 (or similar) to prevent test error, unless you want to go ahead without that test (it's testing that selected values remain for an uncontrolled select when toggling multiple, which is half-broken right now anyway but passing the test).

@sebmarkbage
Copy link
Collaborator

Accepted but only once the dependency is resolved.

@syranide
Copy link
Contributor Author

syranide commented Nov 7, 2014

Dependency landed, I'll rebase and retest this PR.

@zpao
Copy link
Member

zpao commented Jan 21, 2015

We're the worst at merging… This is still passing all tests in a real browser and Phantom, but it's failing with jest, which is a bit of a bummer. It's actually an issue with the outdated jsdom we have in jest (which I'm going to fix... jestjs/jest#221). In the mean time we might have to xit those tests and add TODO comments.

@syranide
Copy link
Contributor Author

@zpao This is on me actually, I got the go-ahead for merging my accepted ones some time (too long) ago after rebasing and testing... it's just been one thing after another on my end. But I'm trying to get some time over to go through my accepted PRs and get them rebased/tested soon.

@syranide
Copy link
Contributor Author

@sebmarkbage @zpao I've verified that there aren't any other properties that should have MUST_DELETE_PROPERTY with a script that iterates through all elements supported by React and all configured properties (found #3000).

I've changed it to always set boolean properties to false instead of removeAttribute, this also neatly avoids the select.multiple-bug. However, I have left MUST_DELETE_PROPERTY on the affected properties even though it does nothing right now (but may in the future, like MUST_USE_PROPERTY).

There's an unsolvable edge-case for now; button.value will never lose the attribute due to sharing configuration with input.value, but it will still work as intended. But this is a minor nitpick considering we currently have significantly worse flaws without this PR.

So unless there's any red flags for you guys, this should be OK to merge from my POV.

EDIT: (jest) JSDOM was failing my new prop/attr tests, I commented out the affected rows in hopes that JSDOM will correct it in the future (or if we drop JSDOM). Ok? Or should I just remove the affected tests, they have some value they way they are now at least...

@syranide
Copy link
Contributor Author

ping (no rush)

PS. I'm not quite sure why travis is failing this PR because of existing lint errors but not others?

} else if (DOMProperty.mustDeleteProperty[name]) {
// Some properties are separate from their attribute, `removeAttribute`
// does not have the intended effect for these.
var propName = DOMProperty.getPropertyName[name];
Copy link
Collaborator

Choose a reason for hiding this comment

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

@syranide This is the lint error making Travis fail:

src/browser/ui/dom/DOMPropertyOperations.js
  174:12  error  propName is already defined  no-redeclare

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks.

@syranide
Copy link
Contributor Author

Ping. I fixed the lint issue and I feel confident that everything is in order and that only the above mentioned caveats apply (which are far better than the current caveats), also note the issue with JSDOM tests.

@jimfb
Copy link
Contributor

jimfb commented Dec 15, 2015

Ok, I wrote a jsfiddle to help identify attributes that might warrant manual investigation. I ran it on chrome, next step is to run it on other browsers. Fiddle is located here: http://jsfiddle.net/gLd3hfrn/

On chrome, it flagged: radiogroup mediaGroup httpEquiv icon. The most interesting of which is httpEquiv (it didn't appear to work using attributes, though I'm not sure if chrome would honor the change if it were set using properties, so maybe it doesn't matter). Anyway, fwiw.

cc @syranide

@sophiebits
Copy link
Collaborator

Looks like http-equiv works fine if you spell it as http-equiv not html-equiv.

@jimfb
Copy link
Contributor

jimfb commented Dec 16, 2015

Ah, ok, that's good :). http comes right after html in the list of attributes, plays tricks on your mind.

@syranide
Copy link
Contributor Author

Again really sorry for dragging my feet on this. Personally I'm mostly concerned about input controls and the various mutations of them (e.g. toggling multiple on select doesn't update value in some browsers when specifically removing the attribute (I think), etc) and them visually responding as they should (video/audio attributes/properties?). Also, this PR was initially intended to fix progress which previously wouldn't work as it required the absence of the attribute to correctly reset.

…p MUST_USE_ATTRIBUTE and manage all regular properties as attributes instead
@syranide
Copy link
Contributor Author

syranide commented Jan 9, 2016

Simple test: http://jsfiddle.net/670u7ure/8/

However as you can see <progress /> still doesn't work properly because it should set value as an attribute, but it conflicts with <input> which requires it to be set as a property. We can hack around it by ignoring the property-flag if the node isn't <input>, <select>, etc, but that's really really ugly. A better alternative that's short of per-element-whitelist is to have a per-element-override, so value would be attribute by default but would be overridden to be a property for <input>, not sure about the perf implications of this. Anyway, this bug is already present so it's not a show-stopper, but it's worth figuring out.

@jimfb Do you feel like the transition from properties to attributes is safe from your tests?

Unrelated; Also, I noticed that React has acquired some weird behavior (in master) where it doesn't set value, checked, etc as attributes on first creation when rendering client-side, but if you render to a string and render on-top of that it behaves as "expected".

Unrelated 2; https://github.com/facebook/react/blob/master/src/renderers/dom/shared/DOMPropertyOperations.js#L176 that seems broken, NS-attributes don't obey by the property flags correctly.

@facebook-github-bot
Copy link

@syranide updated the pull request.

@syranide
Copy link
Contributor Author

syranide commented Jan 9, 2016

PS. Another thing to consider, we could get rid of MUST_USE_PROPERTY too and leave it up to the wrappers to manage these properties instead. It would simplify the shared logic in DOMPropertyOperations.

@sophiebits
Copy link
Collaborator

Yes, we can move the .value logic to the wrappers. I wanted to make it use attributes at the low level anyway as part of a fix for #4618.

@syranide
Copy link
Contributor Author

@spicyj Make that change first, then remove MUST_USE_PROPERTY from this PR and merge? Or how do we proceed?

@syranide
Copy link
Contributor Author

Ping

@sophiebits
Copy link
Collaborator

I don't think they need to be intertwined. Will leave merging this to @jimfb since he's already done most of the testing.

@jimfb
Copy link
Contributor

jimfb commented Jan 29, 2016

Ok, I ran a whole suite of tests on a wide variety of browsers (firefox, chrome, ie, edge, safari, etc). I also cherrypicked this over to my devserver and ran through the whole react test flow. I think we've done our due diligence here. Worst case, we revert/fix, but I think the risk is low. Let's do this.

jimfb added a commit that referenced this pull request Jan 29, 2016
Use removeAttribute to forcefully remove properties from the DOM
@jimfb jimfb merged commit 67e1291 into facebook:master Jan 29, 2016
@syranide syranide deleted the propattr branch February 25, 2016 12:04
@syranide
Copy link
Contributor Author

Totally missed that this landed, that's awesome @jimfb 👍

@everdimension
Copy link
Contributor

This PR introduces this bug: #6219

I discussed my solution in that thread a little bit and offered a small fix: #6228

The fix is aimed to preserve current behavior as much as possible without breaking anything.

We could leave it at that, but it might be better to reconsider current implementation in general. Current else-if chain responsible for setting/removing attributes/properties doesn't look straightforward.

@syranide
Copy link
Contributor Author

@everdimension Just by looking at it I'm not entirely sure why that code is to blame or why yours fixes it, or why it worked before but not now (if that is the actual source of the problem)...

But the actual problem I assume should be related to the one described in #6119. Namely that value is considered a special-case now because of inputs but the same logic incorrectly applies to the value attribute for all tags. The solution is to un-special-case value either by moving the special logic to the input-wrapper or alternatively some form of per-element config. EDIT: But it still doesn't make sense to me... it should work anyway.

@syranide
Copy link
Contributor Author

Oh now it makes sense to me, yeah, #6221 is the correct way to fix this. But I'm not sure why it worked before but not now still, that logic is the same, unless there are unrelated changes elsewhere that are the cause of this (I know selects/options have been updated).

@chicoxyzzy
Copy link
Contributor

@syranide did you meant #6228 ?

@syranide
Copy link
Contributor Author

Oh my bad, #6119 is the one I meant I think. It removes the concept of these special properties so it no longer affects DOMPropertyOperations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants