8000 Remove shadow DOM from `atom-text-editor` by as-cii · Pull Request #12903 · atom/atom · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Remove shadow DOM from atom-text-editor #12903

Merged
merged 38 commits into from
Oct 18, 2016
Merged

Remove shadow DOM from atom-text-editor #12903

merged 38 commits into from
Oct 18, 2016

Conversation

as-cii
Copy link
Contributor
@as-cii as-cii commented Oct 7, 2016

Background

Back in #3943, we started rendering text editor contents inside a shadow DOM boundary to prevent outer style sheets from altering the editor inner contents. Although this introduced many benefits, it carried several tradeoffs too.

In particular, we started duplicating some of the style sheets ({context: 'atom-text-editor'}) inside each shadow root so that package and theme authors could still be able to explicitly target the editor visual elements. An alternative solution to this problem that we suggested (before the shadow DOM spec was finalized) was to use /deep/ and ::shadow pseudo selectors, which unfortunately are now being deprecated.

Even more importantly, this architecture was getting in the way of making atom-text-editor a reusable component, because it made it even more coupled to Atom and its ecosystem. Other problems involved things like #4590, and a more complex user experience for package and theme authors in general. Ultimately, we started feeling like the extra complexity of the shadow DOM wasn't worth its benefits.

Removing shadow DOM from atom-text-editor

This pull request aims at removing the shadow DOM in a way that prevents elements within atom-text-editor that we don't fully control (i.e. the grammar scopes applied as CSS classes to every line's inner <span>) from being mistakenly styled from the outside. Specifically, every syntactic class name will now be prepended with syntax--; styling a JavaScript operator, for example, should now look like the following:

.syntax--source.syntax--js .syntax--operator {
  color: green;
}

Given the wide scope of this change, we are trying to reduce package breakage to a minimum by transforming deprecated selectors automatically and emitting a deprecation in deprecation-cop. Applying these transformations can be quite onerous and therefore we have introduced a layer of caching in 07d56b2 to ensure their performance impact stays low after the first time Atom is launched.

Other notable changes include:

  • Block decorations rendering has been rewritten from scratch (it previously worked only with the shadow DOM).

  • When the editor is focused, document.activeElement will now return the hidden <input> inside atom-text-editor, instead of atom-text-editor itself. I could find only one third party package relying on this behavior, and I am going to submit a pull request shortly to use document.activeElement.closest('atom-text-editor') instead.

  • Any Grim or style sheet deprecation will now cause specs to fail, see ensureNoDeprecatedFunctionCalls and ensureNoDeprecatedStylesheets.

  • Rendering flashes for decorations has been broken for a long time, but removing the shadow DOM highlighted the cause of the issue, and so they have been fixed in 1091b0e.

    flash

Conclusion

Removing the shadow DOM is an important step for the editor's future, as doing so will allow us to extract it, clean it up and optimize it. Considering the invasive nature of this change, we are planning to 🚢 it after rolling the railcars next week, so that everyone can build it from master and extensively test it for an entire release cycle.

Before merging this, we will also need to merge the following core package pull requests, so that we stop using any deprecated API or selector, and make the test suite green again:

/cc: @atom/core

Antonio Scandurra added 27 commits October 7, 2016 10:40
Previously this logic lived in atom-reporter, but it seems more
reasonable to throw errors in spec-helper instead, so that the test
suite fails in CI as well whenever a deprecated method or stylesheet is
used.
Signed-off-by: Nathan Sobo <nathan@github.com>
When, after flashing a decoration, the decorated range moved, Atom was
showing an additional flash, even if the previous one had already been
consumed. This bug originated in `HighlightsComponent`, where we
maintained state about a certain highlight's flash count. The problem
with this approach, however, is that highlight objects in the component
are very volatile, and we could even have more than one for a single
decoration (i.e. when such decoration spans multiple tiles).

To fix this, we'll now maintain some additional state in
`TextEditorPresenter`, which will set a `needsFlash` attribute on the
highlight state objects, thereby preventing `HighlightsComponent` from
showing the flash animation more than once when the decorated range
changes.
3runoDesign added a commit to 3runoDesign/my-Editor that referenced this pull request Jan 16, 2017
jmuise pushed a commit to jmuise/atom-zenburn that referenced this pull request Jan 29, 2017
aaronvb added a commit to aaronvb/nifty-syntax that referenced this pull request Jan 29, 2017
Shadow DOM removed from atom-text-editor in
atom/atom#12903
@danielbayley
Copy link
Contributor
danielbayley commented Feb 21, 2017

In a syntax theme I am working on, I utilise postcss-prefixer to add all the syntax-- noise without having to actually deal with that in my source files… I get the reasoning here, but I'm just thinking it would be a much better “DX” if we did something similar in core instead?

@Alhadis
Copy link
Alhadis commented Feb 22, 2017

Wasn't it possible to prefix class selectors automagically if the stylesheet's filename ended with ".syntax.less"? Or am I remembering wrong?

I recall reading about something like that somewhere...

@danielbayley
Copy link
Contributor

@Alhadis I've never heard of that… without PostCSS you mean?

@simurai
Copy link
Contributor
simurai commented Feb 22, 2017

Wasn't it possible to prefix class selectors automagically if the stylesheet's filename ended with ".syntax.less"? Or am I remembering wrong?

I think that didn't prefix selectors, but it loaded the stylesheet into atom-text-editor's Shadow DOM boundary. So you didn't have to use ::shadow.

@agus725

This comment has been minimized.

@agus725

This comment has been minimized.

@quangnam15091977

This comment has been minimized.

@quangnam15091977

This comment has been minimized.

@emir0103

This comment has been minimized.

@Tanapl02

This comment has been minimized.

@Tanapl02

This comment has been minimized.

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

Successfully merging this pull request may close these issues.

0