facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.59k stars 46.43k forks source link

Bug: React converts boolean data-attributes to `"true"` but leaves boolean attributes as `""` #24812

Closed franciscop closed 5 months ago

franciscop commented 2 years ago

React converts boolean data-attributes to "true", while leaving boolean attributes as "" (empty string). This is inconsistent with the way Javascript works and also it's inconsistent from the way React treats attributes.

React version: 18.0.0

Steps To Reproduce

  1. Run this CodeSandbox
import { useEffect } from "react";

export default function App() {
  useEffect(() => {
    // The React way:
    const input = document.querySelector("input");
    console.log("=== React ===");
    console.log("1", input.getAttribute("disabled"));  // ""
    console.log("2", input.getAttribute("data-disabled"));  // "true"

    // The Javascript way:
    document.querySelector(".base").innerHTML +=
      '<input class="injected" disabled data-disabled />';
    const injected = document.querySelector(".injected");
    console.log("=== Javascript ===");
    console.log("3", injected.getAttribute("disabled"));  // ""
    console.log("4", injected.getAttribute("data-disabled"));  // ""

    return () => injected.remove();
  }, []);
  return (
    <div className="base">
      <input disabled data-disabled />
    </div>
  );
}
  1. Look at the console. This is what it returns:
=== React === 
1  "" 
2  true 
=== Javascript === 
3  "" 
4  "" 

This is what is expected:

=== React === 
1  "" 
2  ""    // <= different
=== Javascript === 
3  "" 
4  "" 

Link to code example:

https://codesandbox.io/s/fragrant-thunder-3dsi62?file=/src/App.js

The current behavior

The data-X attribute, when it doesn't have a value, is coerced to "true", a behavior that is not observed in plain Javascript (stays as "") and is also not observed with the way React treats plain boolean attributes (also stays as "").

The expected behavior

The boolean attributes, both for plain attributes or data-attributes, behave in the same way. I don't know which one is "the right way", but surely treating them vastly different seems to be the wrong way.

Alternative: maybe this is the expected behavior? But I couldn't find any mention to anything related to this in the official documentation, please let me know if I missed it somehow or if this needs to be documented otherwise.

franciscop commented 2 years ago

The raw component tree (virtual DOM?) looks like this, so at this level the selected and data-selected look the same (this is from a different example):

pendingProps: {
  'data-id': '25',
  'data-selected': true,
  selected: true,
  children: 'Card'
},
memoizedProps: {
  'data-id': '25',
  'data-selected': true,
  selected: true,
  children: 'Card'
},

With a forked CodeSandbox, we can see the rendered HTML looks different from the parsed HTML from the string by the browser:

=== React === 
<INPUT disabled="" data-disabled="true"></INPUT>
1 "" 
2 true 
=== Javascript === 
<INPUT class="injected" disabled="" data-disabled=""></INPUT>
3 "" 
4 ""

So I'd like to theorize that it's when renderingn the VirtualDOM to the HTML that this transformation occurs only for the data-attributes. I'm not familiar with the React codebase, but I might guess it's somewhere around here that this happens? One if() uses the empty string, and the other stringifies the value, which would convert a true boolean into a "true" string:

    if (type === BOOLEAN || (type === OVERLOADED_BOOLEAN && value === true)) {
      // If attribute type is boolean, we know for sure it won't be an execution sink
      // and we won't require Trusted Type here.
      attributeValue = '';
    } else {
      // `setAttribute` with objects becomes only `[object]` in IE8/9,
      // ('' + value) makes it output the correct toString()-value.
      if (enableTrustedTypesIntegration) {
        attributeValue = (value: any);
      } else {
        if (__DEV__) {
          checkAttributeStringCoercion(value, attributeName);
        }
        attributeValue = '' + (value: any);
      }
DuffyWang926 commented 2 years ago

I try to find the answer of first question:

企业微信截图_1cbd0dd5-2320-4129-8beb-9c6b093e92ad

Because you don't set the value of ‘data-disabled’,so the react give the value ‘true’。 when react try to set property of Dom:

企业微信截图_3d226130-36ce-4bd1-b42f-f029ffc51b11

Because ‘ disabled’ is the property of Dom,react reset the value to ‘’。

企业微信截图_33370183-fd73-4c7d-80c3-230ea241deb9

Because ‘data-disabled’ is not the property of Dom,react set the value to ‘’ + rawValue。 That is the difference of react and javascript in your first question。

ziavii commented 2 years ago

I agree with the above clarification.

github-actions[bot] commented 5 months ago

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

savannah-yahsuanlin commented 5 months ago

Sincerely,

Savannah(Zi-Yuan) Lin Software engineer Email: @.>@. @.***> LinkedIn: https://www.linkedin.com/in/zi-yuan lingered/ https://www.linkedin.com/in/zi-yuanlin/

On Wed, Apr 10, 2024 at 6:33 PM Francisco Presencia < @.***> wrote:

Closed #24812 https://github.com/facebook/react/issues/24812 as completed.

— Reply to this email directly, view it on GitHub https://github.com/facebook/react/issues/24812#event-12419670545, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6WEVHMUJYLZWLOYJBVBATY4UIQNAVCNFSM52FY2LQ2U5DIOJSWCZC7NNSXTWQAEJEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW4OZRGI2DCOJWG4YDKNBV . You are receiving this because you are subscribed to this thread.Message ID: @.***>

otomad commented 3 months ago

Sorry, but this is bothering me.

I have to use data-disabled={disabled ? "" : undefined} to keep the functionality up and running.

Since I'm using [data-disabled] selector in CSS, if that's the case now, I'd have to write [data-disabled="true"], which would be more cumbersome.

If I really need to convert to a string, I should manually call disabled.toString(), instead of automatically implicitly converting like this.