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

Unexpected JSX Whitespace Handling with Leading and Trailing Tags #4134

Closed
jmar777 opened this issue Jun 15, 2015 · 4 comments
Closed

Unexpected JSX Whitespace Handling with Leading and Trailing Tags #4134

jmar777 opened this issue Jun 15, 2015 · 4 comments

Comments

@jmar777
Copy link

jmar777 commented Jun 15, 2015

Related (closed) issue: #65

I'm not sure whether this is considered a bug or if it's working as intended, but I'll at least throw in a vote that the behavior is unexpected. Given the following example:

var Test = React.createClass({
  render: function() {
    return (
      <p>
        Demonstration of <span>unexpected</span>
        whitespace handling in
        <span>ReactJS</span>.
      </p>
    );
  }
});

React.render(<Test />, document.body)

[Demonstration on jsbin].

I would suggest that the expected rendered output of the above component would be:

Demonstration of unexpected whitespace handling in ReactJS.

Whereas the actual output is:

Demonstration of unexpectedwhitespace handling inReactJS.

(No whitespace between "unexpected" and "whitespace" or between "in" and "ReactJS").

I would consider this unexpected for two primary reasons:

  • It's inconsistent with HTML's handling of whitespace.
  • Removing the <span> tags (but leaving newlines and indentations unchanged) causes whitespace to be preserved.

If this is simply a bug, then I suppose there's no need to belabor the point. If not, is there some rationale for leading/trailing tags behaving different than leading/trailing plain text?

Thanks!

@zpao
Copy link
Member

zpao commented Jun 15, 2015

It is intentional and we know it's inconsistent with HTML (which is honestly pretty odd). The reason removing the spans works is because then the whole child is read as a single string (with spaces).

There is some more detail in the blog post when we made this change (>1 year ago): http://facebook.github.io/react/blog/2014/02/20/react-v0.9.html#jsx-whitespace

The short version is that it's what we believe to be the lesser of 2 (whitespace handling) evils.

@zpao zpao closed this as completed Jun 15, 2015
@jmar777
Copy link
Author

jmar777 commented Jun 15, 2015

@zpao Given a tossup between protecting developers from whitespace that they unintentionally added (but added nonetheless) vs. departing from the obvious HTML analog and actively trimming whitespace that was intentionally added...

I'll just stop, since I guess this ship has sailed? Either way, thanks for the prompt reply. Mad <3's for the team.

@icirellik
Copy link

I do have a question regarding this, mostly because I'm starting to see pull requests come through containing   to correct this "feature", is there a preferred way to ensure correct spacing on the client?

Of the following choices, its tough to choose a winner:

  • The React way generates far too much boiler plate.
  •   is rather difficult to read
  • [:space:] seems ripe for a developer to remove during a refactor.

React

Demonstration of <span>unexpected</span>
{' '}whitespace handling in{' '}
<span>ReactJS</span>.

nbsp

Demonstration of <span>unexpected</span>
&nbsp;whitespace handling in&nbsp;
<span>ReactJS</span>.

<span>

Demonstration of <span>unexpected</span>
<span> whitespace handling in </span>
<span>ReactJS</span>.

@syranide
Copy link
Contributor

syranide commented Jul 3, 2015

Related: facebook/jsx#28, facebook/jsx#35, facebook/jsx#8

I think it's important to note that JSX is not HTML. JSX compiles to JS which can then target any frontend. While you may expect implicit whitespace to appear in certain places it currently doesn't due to familiarity with HTML, if you check the resulting code without whitespace trimming then you'll see that you definitely do not want that.

<span>
  <span>
    X
  </span>
</span>

With our current white-space rules it generates:

React.createElement('span', {}, React.createElement('span', {}, 'X'));

But without it would generate:

React.createElement('span', {}, '\n  ', React.createElement('span', {}, '\n    X\n  '), '\n  ');

Is that what you intended? I'm pretty sure it's not, especially if you consider that most frontends would be expected to take whitespace literally and not trim it like HTML does (can be disabled with white-space: pre-wrap which is really nice).

Personally I can count the number of times I've had to use {' '} in our codebase on one hand, and I even prefer it that way because it also shows that I explicitly wanted those to be there.

akinsho added a commit to akinsho/oni that referenced this issue Dec 24, 2017
using whitespace: pre-wrap from one of see
facebook/react#4134
html auto removes trailing white space breaking
the cursor's position functionality
bryphe pushed a commit to onivim/oni that referenced this issue Jan 4, 2018
* Rework styling to match quickopen menu

also add icon - file icon atm ?alternatives

* Render wild menu into the stack layer

* Reduce padding

* Compose wild menu with commandline

* Configure wild menu and commandline

Components now render in the same layer and
can be positioned similarly if both present
or interchangeably if one options is disabled

* Render components separately inside of common

container

* Attempt to manually place external menus not working

* add space to constructor

* Add timeout to prevent flickering of cmdline

Also only render overlay if both elements present

* revert changes to neovim surface

* wire up setcursor actions

* Add a cursor to commandline

* Remove excess space

* Remove refs from commandline components

* re-add focus as this is essential for functionality

* important: fix html trimming whitespace bug

using whitespace: pre-wrap from one of see
facebook/react#4134
html auto removes trailing white space breaking
the cursor's position functionality

* add in the prompt segment

* fix overflow styling

* add pointer events but not selection

* Add overflow handling for command line

* minor change to comment position

* adjust cursor position

* re-add missing max width
to ensure commandline and wild menu have
same width

* re-arrange css to rerun tests

* re-arrange css again..

* reduce timeout to 80ms

* Add spurious CR to rerun test
bryphe pushed a commit to onivim/oni that referenced this issue Jan 5, 2018
* Rework styling to match quickopen menu

also add icon - file icon atm ?alternatives

* Render wild menu into the stack layer

* Reduce padding

* Compose wild menu with commandline

* Configure wild menu and commandline

Components now render in the same layer and
can be positioned similarly if both present
or interchangeably if one options is disabled

* Render components separately inside of common

container

* Attempt to manually place external menus not working

* add space to constructor

* Add timeout to prevent flickering of cmdline

Also only render overlay if both elements present

* revert changes to neovim surface

* wire up setcursor actions

* Add a cursor to commandline

* Remove excess space

* Remove refs from commandline components

* re-add focus as this is essential for functionality

* important: fix html trimming whitespace bug

using whitespace: pre-wrap from one of see
facebook/react#4134
html auto removes trailing white space breaking
the cursor's position functionality

* add in the prompt segment

* fix overflow styling

* add pointer events but not selection

* Add overflow handling for command line

* minor change to comment position

* adjust cursor position

* re-add missing max width
to ensure commandline and wild menu have
same width

* re-arrange css to rerun tests

* re-arrange css again..

* reduce timeout to 80ms

* Add spurious CR to rerun test

* Add icons to replace the first character

* Add configuration to show and hide icons
gbirke added a commit to wmde/fundraising-banners-until-2022 that referenced this issue Oct 17, 2019
Pass the callback to the child method up the component hierarchy, via a
"trigger method", so the mobile banner can call the method for the
animation when it shows the fullpage banner.

Add explicitly evaluated spaces to TextHighlight child text to
circumvent [JSX space-eating behavior][1].

[1]: facebook/react#4134
gbirke added a commit to wmde/fundraising-banners-until-2022 that referenced this issue Oct 21, 2019
Pass the callback to the child method up the component hierarchy, via a
"trigger method", so the mobile banner can call the method for the
animation when it shows the fullpage banner.

Add explicitly evaluated spaces to TextHighlight child text to
circumvent [JSX space-eating behavior][1].

[1]: facebook/react#4134
gbirke added a commit to wmde/fundraising-banners-until-2022 that referenced this issue Oct 21, 2019
Pass the callback to the child method up the component hierarchy, via a
"trigger method", so the mobile banner can call the method for the
animation when it shows the fullpage banner.

Add explicitly evaluated spaces to TextHighlight child text to
circumvent [JSX space-eating behavior][1].

[1]: facebook/react#4134
gbirke added a commit to wmde/fundraising-banners-until-2022 that referenced this issue Oct 23, 2019
Updated dependencies for Babel 7
Added jsx plugin
Change shared banner modules to ES modules

Babel 7 and the `useBuiltIns: 'usage'` setting injects ES6 imports into
the CommonJS-based banners. This leads to the error message
TypeError: "exports" is read-only` in the compiled banners (See also
webpack/webpack#3997 and
babel/babel#6980 ).

Changed `getSkin` in "BannerFunctions" to preload all skin
manipulating classes.

Add Preact desktop banner

Pass the callback to the child method up the component hierarchy, via a
"trigger method", so the mobile banner can call the method for the
animation when it shows the fullpage banner.

Add explicitly evaluated spaces to TextHighlight child text to
circumvent [JSX space-eating behavior][1].

[1]: facebook/react#4134
LukeShu added a commit to telepresenceio/telepresence.io that referenced this issue Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants