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

React DOM: Support boolean values for inert prop #24730

Merged
merged 11 commits into from
Mar 13, 2024

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Jun 15, 2022

Implementation alternates:

  1. 78f1ad6
  2. 78391f0
  3. 1e928d8

3 leverages fallthroughs more but requires dropping the default case in favor of early return. I don't know how else I can make it work with the flag.
basically, how to implement

function isBooleanProp(enableNewBooleanProps, prop) {
    switch (prop) {
    case 'scoped':
        if (!enableNewBooleanProps) {
            return true
        }
    // fallthrough with enableNewBooleanProps
    case 'inert':
        if (enableNewBooleanProps) {
           return true
        } else {
            // Goto default how?
        }
    // We should not fallthrough here
    case 'download':
        // previous cases cannot fallthrough into this
        return 'overloaded'
    default:
        return false
    }
}

Summary

Adds support for HTMLElement.inert behind enableNewBooleanProps which is turned on in experimental builds.

Note that the previous workaround (inert="") will no longer work since the empty string is considered false for boolean props.

Closes #17157

How did you test this change?

You need Chrome >=102 (or any supporting browser listed in https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/inert#browser_compatibility).

@sebmarkbage
Copy link
Collaborator

Note that the previous workaround (inert="") will no longer work since the empty string is considered false for boolean props.

Seems like we might want to add this as a breaking change for 19 then? Maybe add it behind a flag in experimental for now so we know to enable it later?

@eps1lon
Copy link
Collaborator Author

eps1lon commented Jun 16, 2022

Seems like we might want to add this as a breaking change for 19 then? Maybe add it behind a flag in experimental for now so we know to enable it later?

Yeah that seems reasonable.

Though we'll run into this scenario over and over and it's a bit annoying for React users since inert is part of the standard and supported by now.

Would it make sense for unknown props to check if a prop is an IDL attribute (e.g. 'inert' in htmlElement) and then set the IDL attribute to the prop value? Relying on the fact the browser is responsible for potentially reflecting the value in the content attribute. This would match the behavior behind enableCustomElementPropertySupport for custom component tags.

@sizebot
Copy link

sizebot commented Jun 16, 2022

Comparing: 4cd788a...b052e93

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.79 kB 131.79 kB = 42.40 kB 42.40 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.02% 137.06 kB 137.08 kB +0.04% 44.00 kB 44.01 kB
facebook-www/ReactDOM-prod.classic.js = 457.17 kB 457.17 kB = 83.22 kB 83.22 kB
facebook-www/ReactDOM-prod.modern.js +0.01% 442.41 kB 442.47 kB +0.03% 80.94 kB 80.96 kB
facebook-www/ReactDOMForked-prod.classic.js = 457.94 kB 457.94 kB = 83.33 kB 83.33 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactFlightDOMRelayServer-prod.modern.js +0.29% 27.91 kB 28.00 kB +0.31% 7.37 kB 7.40 kB

Generated by 🚫 dangerJS against b052e93

@eps1lon
Copy link
Collaborator Author

eps1lon commented Jun 16, 2022

@sebmarkbage I could add hidden to OVERLOADED_BOOLEAN which would ensure compat with inert="". This also has the benefit to prevent future compat issues like hidden has: adding support for hidden="until-found" (see #24740) means hidden="" will now be considered true instead of false.

@react-sizebot
Copy link

react-sizebot commented Feb 11, 2023

Comparing: bb0944f...cf2173e

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.02% 176.80 kB 176.83 kB +0.01% 54.90 kB 54.91 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 173.53 kB 173.55 kB +0.01% 54.11 kB 54.11 kB
facebook-www/ReactDOM-prod.classic.js +0.01% 593.95 kB 594.04 kB = 104.36 kB 104.37 kB
facebook-www/ReactDOM-prod.modern.js +0.01% 577.21 kB 577.30 kB = 101.41 kB 101.42 kB
test_utils/ReactAllWarnings.js Deleted 66.60 kB 0.00 kB Deleted 16.28 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-dom/cjs/react-dom-server.bun.development.js +0.32% 430.51 kB 431.90 kB +0.27% 95.71 kB 95.97 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.development.js +0.32% 457.21 kB 458.67 kB +0.28% 98.57 kB 98.84 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.development.js +0.32% 436.82 kB 438.20 kB +0.27% 97.61 kB 97.87 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.development.js +0.31% 438.67 kB 440.05 kB +0.27% 98.07 kB 98.34 kB
oss-experimental/react-dom/umd/react-dom-server.browser.development.js +0.31% 466.46 kB 467.92 kB +0.28% 99.73 kB 100.01 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js +0.31% 444.09 kB 445.48 kB +0.27% 98.00 kB 98.27 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js +0.31% 445.66 kB 447.05 kB +0.26% 98.79 kB 99.05 kB
oss-experimental/react-dom/cjs/react-dom-server.edge.development.js +0.31% 446.25 kB 447.63 kB +0.26% 98.93 kB 99.18 kB
test_utils/ReactAllWarnings.js Deleted 66.60 kB 0.00 kB Deleted 16.28 kB 0.00 kB

Generated by 🚫 dangerJS against cf2173e

@jfbrennan
Copy link

jfbrennan commented May 12, 2023

All major browsers support the inert attribute.

The original request for React to "whitelist" this attribute was opened 3 1/2 years ago, presumably to prevent this very situation where React, not the browsers, is the blocker.

This PR was opened 1 year ago. It remains open and those who maintain React projects are prevented from using this new HTML attribute.

I'm sure the React team is a great bunch of folks - honestly - but it is unacceptable for a project like React to drag its feet for more than 3 years now and prevent developers from using native HTML features. This is IE behavior. React isn't a little open-source project maintained by volunteers, React is a product marketed to developers with a multi-million dollar budget and paid full-time engineers. Again, great people I'm sure, but great people don't always deliver early or even on time. Support for inert is objectively very late. Let's please get this merged!

@DIPANJAN01
Copy link

Its 2023, June, and React still doesn't recognize the inert attribute. I really hope its implemented as soon as possible. Such a basic thing taking sooooo long

@ingomc
Copy link

ingomc commented Aug 9, 2023

Still waiting for a 3 year old html native attribut...

@mattcarrollcode
Copy link
Contributor

@jfbrennan @DIPANJAN01 @ingomc sorry about this taking so long. in the mean time you should be able to work around this by using adding the inert="" attribute e.g. <div inert="" />. If this workaround doesn't' work for your use case please let us know so we have the chance to fix it in this PR.

@jfbrennan
Copy link

jfbrennan commented Aug 30, 2023

Thanks @mattcarrollcode I did just that and also had to add this to get TS to shut up:

declare module 'react' {
  interface HTMLAttributes<T> extends AriaAttributes, DOMAttributes<T> {
    inert?: ''; // TODO Treating this totally valid HTML attribute as custom to make React work. Remove after this bug is fixed https://github.com/facebook/react/pull/24730
  }
}

@Link2Twenty
Copy link

I'd like to add my +1 to this being sorted.

@eps1lon eps1lon force-pushed the feat/inert branch 2 times, most recently from b6e9c7a to 09191f6 Compare January 6, 2024 17:53
@@ -711,6 +712,7 @@ function setProp(
case 'disableRemotePlayback':
case 'formNoValidate':
case 'hidden':
case enableNewBooleanProps ? 'inert' : 'formNoValidate':
Copy link
Collaborator Author

@eps1lon eps1lon Jan 6, 2024

Choose a reason for hiding this comment

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

This is quite hacky but allows for a minimal diff as far as I can tell. We want the case to be just case 'inert' when enableNewBooleanProps is enabled and ideally the case to disappear without enableNewBooleanProps which we get indirectly by reusing a value we already covered earlier. I didn't choose hidden because that might move in the future when we add hidden="until-found".

It's quite lazy so open to refactoring this if you feel uncomfortable shipping it like this. The alternatives I explored look quite verbose and doesn't DCE quite as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alternate would be: 133867b (#24730)

I'm not sure how to add it behind a flag without runtime impact.

@eps1lon eps1lon merged commit bbc571a into facebook:main Mar 13, 2024
38 checks passed
@eps1lon eps1lon deleted the feat/inert branch March 13, 2024 22:10
github-actions bot pushed a commit that referenced this pull request Mar 13, 2024
jackpope pushed a commit that referenced this pull request Mar 14, 2024
github-actions bot pushed a commit that referenced this pull request Mar 14, 2024
gnoff added a commit to gnoff/next.js that referenced this pull request Mar 25, 2024
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
gnoff added a commit to gnoff/next.js that referenced this pull request Mar 25, 2024
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
eps1lon added a commit to vercel/next.js that referenced this pull request Apr 19, 2024
### React upstream changes

- facebook/react#28643
- facebook/react#28628
- facebook/react#28361
- facebook/react#28513
- facebook/react#28299
- facebook/react#28617
- facebook/react#28618
- facebook/react#28621
- facebook/react#28614
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
joonassandell added a commit to joonassandell/ui-lab that referenced this pull request Jul 20, 2024
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.

[DOM] Add support for the inert attribute