Skip to content

Bugfix: scope CSS of input elements #2589

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

Merged
merged 11 commits into from
Jul 13, 2023
Merged

Bugfix: scope CSS of input elements #2589

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
dd6ed5b
Add class to Input elements so that CSS rules can be scoped
KoolADE85 Jul 6, 2023
745f0f3
Small changes to address code review feedback
KoolADE85 Jul 7, 2023
aa85b3c
Ensure no trailing space is added to className
KoolADE85 Jul 10, 2023
494e23d
Update CHANGELOG
KoolADE85 Jul 10, 2023
3e6b531
Exclude search element from html checks for now
KoolADE85 Jul 12, 2023
1623592
Remove duplicate key
KoolADE85 Jul 12, 2023
53bdf71
Revert "Exclude search element from html checks for now"
KoolADE85 Jul 12, 2023
b156dc4
build
KoolADE85 Jul 12, 2023
c4ca03b
Merge branch 'dev' into bugfix/dash-embedded-83
KoolADE85 Jul 12, 2023
11af805
build
KoolADE85 Jul 12, 2023
ffcc9eb
build
KoolADE85 Jul 13, 2023
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
All notable changes to `dash` will be documented in this file.
This project adheres to [Semantic Versioning](https://semver.org/).

## UNRELEASED

## Fixed

- [#2589](https://github.com/plotly/dash/pull/2589) CSS for input elements not scoped to Dash application

## [2.11.1] - 2023-06-29

## Fixed
Expand Down
4 changes: 4 additions & 0 deletions components/dash-core-components/src/components/Input.react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,18 +61,22 @@ export default class Input extends PureComponent {
const valprops =
this.props.type === 'number' ? {} : {value: this.state.value};
const {loading_state} = this.props;
let {className} = this.props;
className = 'dash-input' + (className ? ` ${className}` : '');
return (
<input
data-dash-is-loading={
(loading_state && loading_state.is_loading) || undefined
}
className={className}
ref={this.input}
onBlur={this.onBlur}
onChange={this.onChange}
onKeyPress={this.onKeyPress}
{...valprops}
{...omit(
[
'className',
Comment on lines 87 to +89
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI we use this a lot but I'd love to get rid of this ...omit pattern pretty much wherever we use it. It causes various problems, like I bet we're currently improperly forwarding persistence/persistence_type/persisted_props here, and some other places we still forward loading_state so the DOM ends up with `loading_state="[object Object]". Instead we should figure out which props we DO want and pick them, or just individually forward them. I thought I had made an issue for this but not seeing it now 🤔

'debounce',
'value',
'n_blur',
Expand Down
6 changes: 3 additions & 3 deletions components/dash-core-components/src/components/css/input.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
input:invalid {
input.dash-input:invalid {
outline: solid red;
}

input:valid {
input.dash-input:valid {
outline: none black;
}
}
31 changes: 31 additions & 0 deletions components/dash-core-components/tests/integration/input/test_input_basics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,34 @@ def test_inbs002_user_class(dash_dcc):
dash_dcc.wait_for_style_to_equal(".test-input-css input", "width", "420px")

assert dash_dcc.get_logs() == []


def test_inbs003_styles_are_scoped(dash_dcc):
app = Dash(__name__)

app.index_string = """
<html>
<body>
<input id="ExternalInput" required />
{%app_entry%}
{%config%}
{%scripts%}
{%renderer%}
</body>
</html>
"""

app.layout = html.Div(
className="test-input-css",
children=[dcc.Input(id="DashInput", required=True, className="unittest")],
)

dash_dcc.start_server(app)

external_input = dash_dcc.find_element("#ExternalInput")
dash_input = dash_dcc.find_element(".unittest")

external_outline_css = external_input.value_of_css_property("outline")
dash_outline_css = dash_input.value_of_css_property("outline")

assert external_outline_css != dash_outline_css
37 changes: 12 additions & 25 deletions components/dash-html-components/scripts/data/attributes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,6 @@
],
"description": "Indicates whether controls in this form can by default have their values automatically completed by the browser."
},
"autoFocus": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

weird, autoFocus disappeared from the reference docs? Yes: mdn/content#27734 - the PR mentions this page: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes which (a) we don't inspect at all during the build process, (b) implies autoFocus applies to a lot more than just these four element types.

This seems like it would be a breaking change - I don't actually know if this attribute works in a Dash app, we may be adding our elements too late in the page lifecycle for this to be honored, but even if it doesn't work we shouldn't break an app that already includes it.

More broadly, I don't like the fact that in every PR our tests are at the mercy of MDN's changes, we should really make "update the HTML spec from MDN" an optional thing that we do occasionally, maybe when we're updating dependencies or maybe even less frequently. @T4rk1n thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should change the generation to be more stable. The test allow us to see there is change, but maybe it should be a CI job that run once a month or so and create a PR if there is change.

An alternative could be to change the scraper and instead use the React types that are contained in JSX.IntrinsicElements. That would change only when the react version is changed and should more stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the changes to attributes.json don't actually need to be committed now. I reverted them, and the build/tests still function without them.
However, the change to extract-elements.js does need to be here in order to complete a build.
So I'd propose decoupling attributes.json from this PR, and we can think about this as a separate fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'll need to address this prior to a release regardless, as that process will call the full rebuild. But yeah for the sake of this PR we can ignore it.

"elements": [
"button",
"input",
"select",
"textarea"
],
"description": "The element should be automatically focused after the page loaded."
},
"autoPlay": {
"elements": [
"audio",
Expand Down Expand Up @@ -760,7 +751,6 @@
"accept",
"alt",
"autoComplete",
"autoFocus",
"capture",
"checked",
"disabled",
Expand Down Expand Up @@ -848,7 +838,6 @@
],
"select": [
"autoComplete",
"autoFocus",
"disabled",
"form",
"multiple",
Expand All @@ -858,7 +847,6 @@
],
"textarea": [
"autoComplete",
"autoFocus",
"cols",
"disabled",
"form",
Expand All @@ -872,19 +860,6 @@
"rows",
"wrap"
],
"button": [
"autoFocus",
"disabled",
"form",
"formAction",
"formEncType",
"formMethod",
"formNoValidate",
"formTarget",
"name",
"type",
"value"
],
"audio": [
"autoPlay",
"controls",
Expand Down Expand Up @@ -967,6 +942,18 @@
"src",
"srcLang"
],
"button": [
"disabled",
"form",
"formAction",
"formEncType",
"formMethod",
"formNoValidate",
"formTarget",
"name",
"type",
"value"
],
"fieldset": [
"disabled",
"form",
Expand Down
4 changes: 2 additions & 2 deletions components/dash-html-components/scripts/extract-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const expectedElCount = 125;
*/
function extractElements($) {
const excludeElements = [
'html', 'head', 'body', 'style', 'h1–h6', 'input',
'html', 'head', 'body', 'style', 'h1–h6', 'input', 'search',
// out of scope, different namespaces - but Mozilla added these to the
// above reference page Jan 2021 so we need to exclude them now.
// see https://github.com/mdn/content/pull/410
Expand All @@ -22,7 +22,7 @@ function extractElements($) {
'image', 'dir', 'tt', 'applet', 'noembed', 'bgsound', 'menu', 'menuitem',
'noframes',
// experimental, don't add yet
'portal'
'portal',
];
// `<section>` is for some reason missing from the reference tables.
const addElements = [
Expand Down