Remove shadow DOM from `atom-text-editor` #12903

Merged
merged 38 commits into from Oct 18, 2016

Conversation

Projects
None yet
8 participants
@as-cii
Member

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

as-cii added some commits Sep 22, 2016

Throw an error if there is any deprecation in a spec
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.
Improve selector deprecation message
Signed-off-by: Nathan Sobo <nathan@github.com>
Fix decorations flashing more than once
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.

@simurai simurai referenced this pull request in simurai/one-dark-vivid-syntax Oct 8, 2016

Merged

Stop using shadow DOM selectors #3

@simurai simurai referenced this pull request in atom-community/ui-theme-template Oct 8, 2016

Merged

Stop using shadow DOM selectors #4

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jan 10, 2017

AHAHAH dat timing.

Alhadis commented Jan 10, 2017

AHAHAH dat timing.

@mikemcbride

This comment has been minimized.

Show comment
Hide comment
@mikemcbride

mikemcbride Jan 10, 2017

@Alhadis haha that happens far too often 😄

@Alhadis haha that happens far too often 😄

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Jan 10, 2017

it changes the specificity of CSS classes

What is the practical implication of this w.r.t. Atom's editor?

it elevates the risk of incorrectly applying styles to elements that happen to already exist with those class-names.

Is this actually a problem, though? Or just "it might be a problem"?

Also, the passive aggressive behaviour isn't really creating a great vibe here. I'm really interested in the tradeoffs as from the outside it looks like a pretty big trade-off but it's not clear what the advantages are and they aren't well explained in the blog post on atom.io.

it changes the specificity of CSS classes

What is the practical implication of this w.r.t. Atom's editor?

it elevates the risk of incorrectly applying styles to elements that happen to already exist with those class-names.

Is this actually a problem, though? Or just "it might be a problem"?

Also, the passive aggressive behaviour isn't really creating a great vibe here. I'm really interested in the tradeoffs as from the outside it looks like a pretty big trade-off but it's not clear what the advantages are and they aren't well explained in the blog post on atom.io.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jan 10, 2017

Member

Or just "it might be a problem"?

This is a very real problem. language-c at one point had scope names that included .notification in them. When Notifications were introduced, there was much unhappiness as any syntax theme that had styled the notification class was now inadvertently styling Notifications as well, and vice versa (causing editing a C file to be...a bit interesting).

Member

50Wliu commented Jan 10, 2017

Or just "it might be a problem"?

This is a very real problem. language-c at one point had scope names that included .notification in them. When Notifications were introduced, there was much unhappiness as any syntax theme that had styled the notification class was now inadvertently styling Notifications as well, and vice versa (causing editing a C file to be...a bit interesting).

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Jan 10, 2017

This is a very real problem. language-c at one point had scope names that included .notification in them. When Notifications were introduced, there was much unhappiness as any syntax theme that had styled the notification class was now inadvertently styling Notifications as well.

Is this the only instance of the problem?

Couldn't it be solved by scoping the CSS selector?

This is a very real problem. language-c at one point had scope names that included .notification in them. When Notifications were introduced, there was much unhappiness as any syntax theme that had styled the notification class was now inadvertently styling Notifications as well.

Is this the only instance of the problem?

Couldn't it be solved by scoping the CSS selector?

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jan 10, 2017

@ioquatix You need to remember that class-names are arbitrary. Prefixing them indiscriminately is the safest way to shield them from unexpected and unwanted styling.

Alhadis commented Jan 10, 2017

@ioquatix You need to remember that class-names are arbitrary. Prefixing them indiscriminately is the safest way to shield them from unexpected and unwanted styling.

@mikemcbride

This comment has been minimized.

Show comment
Hide comment
@mikemcbride

mikemcbride Jan 10, 2017

I appreciate your interest in the problem here, but the way they solved this really works quite well. As a theme author, it was trivial for me to update my themes to the new selectors, so I'd say they did a pretty good job.

So far the only concern I've read is that you find the syntax ugly which, to be fair to the maintainers, isn't a great reason to reimplement something of this magnitude...

I appreciate your interest in the problem here, but the way they solved this really works quite well. As a theme author, it was trivial for me to update my themes to the new selectors, so I'd say they did a pretty good job.

So far the only concern I've read is that you find the syntax ugly which, to be fair to the maintainers, isn't a great reason to reimplement something of this magnitude...

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jan 10, 2017

FWIW, I also find the syntax unsightly, but I appreciate the fact it's functional, self-explanatory, and the least likely to introduce "issues" with developing themes or language grammars.

You can always alias it locally using a Less variable or something.

Alhadis commented Jan 10, 2017

FWIW, I also find the syntax unsightly, but I appreciate the fact it's functional, self-explanatory, and the least likely to introduce "issues" with developing themes or language grammars.

You can always alias it locally using a Less variable or something.

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Jan 10, 2017

You need to remember that class-names are arbitrary. Prefixing them indiscriminately is the safest way to shield them from unexpected and unwanted styling.

Your absolutely correct. But, what's wrong with using .syntax .language .attribute-name (where you obviously replace .language with something specific). I actually use this in a syntax highlighter I wrote, and it works really well. It allows you to do semantic queries, e.g. style all variables, or all ruby variables, etc.

I appreciate your interest in the problem here, but the way they solved this really works quite well.

That's great, I'm interested in understanding more about how it works. But, I'm also interested in how the above case is handled, (e.g. style all variables, style all ruby variables, etc)

As a theme author, it was trivial for me to update my themes to the new selectors, so I'd say they did a pretty good job.

Yes, I also have several packages for Atom which will need to be updated, so that's good to hear.

So far the only concern I've read is that you find the syntax ugly which, to be fair to the maintainers, isn't a great reason to reimplement something of this magnitude...

I didn't mean to sound like I don't appreciate the work that's gone into this. I don't think I said anywhere to re-implement anything. I'm just voicing my concern about the syntax. I don't think the trade-offs of this choice were well discussed anywhere and as you've said yourself, it's a fairly deep and fundamental change to the way styling works within Atom.

You need to remember that class-names are arbitrary. Prefixing them indiscriminately is the safest way to shield them from unexpected and unwanted styling.

Your absolutely correct. But, what's wrong with using .syntax .language .attribute-name (where you obviously replace .language with something specific). I actually use this in a syntax highlighter I wrote, and it works really well. It allows you to do semantic queries, e.g. style all variables, or all ruby variables, etc.

I appreciate your interest in the problem here, but the way they solved this really works quite well.

That's great, I'm interested in understanding more about how it works. But, I'm also interested in how the above case is handled, (e.g. style all variables, style all ruby variables, etc)

As a theme author, it was trivial for me to update my themes to the new selectors, so I'd say they did a pretty good job.

Yes, I also have several packages for Atom which will need to be updated, so that's good to hear.

So far the only concern I've read is that you find the syntax ugly which, to be fair to the maintainers, isn't a great reason to reimplement something of this magnitude...

I didn't mean to sound like I don't appreciate the work that's gone into this. I don't think I said anywhere to re-implement anything. I'm just voicing my concern about the syntax. I don't think the trade-offs of this choice were well discussed anywhere and as you've said yourself, it's a fairly deep and fundamental change to the way styling works within Atom.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Jan 10, 2017

But, what's wrong with using .syntax .language .attribute-name (where you obviously replace .language with something specific).

.attribute-name is at risk of being targeted by a theme or package's styling, which broadly targets .attribute-name in its stylesheet.

And remember, the class in question might be even less specific-sounding than .attribute-name. It could be .comment, which could prepend a comment-icon to every comment-line in one's editor, depending on what packages are active...

Alhadis commented Jan 10, 2017

But, what's wrong with using .syntax .language .attribute-name (where you obviously replace .language with something specific).

.attribute-name is at risk of being targeted by a theme or package's styling, which broadly targets .attribute-name in its stylesheet.

And remember, the class in question might be even less specific-sounding than .attribute-name. It could be .comment, which could prepend a comment-icon to every comment-line in one's editor, depending on what packages are active...

@ioquatix

This comment has been minimized.

Show comment
Hide comment
@ioquatix

ioquatix Jan 10, 2017

.attribute-name is at risk of being targeted by a theme or package's styling, which broadly targets .attribute-name in its stylesheet.

Isn't any class name at risk of being targeted by a theme or package's CSS? What makes .syntax-- less likely to be a problem?

.attribute-name is at risk of being targeted by a theme or package's styling, which broadly targets .attribute-name in its stylesheet.

Isn't any class name at risk of being targeted by a theme or package's CSS? What makes .syntax-- less likely to be a problem?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Jan 10, 2017

Member

A few reasons.

  1. We are implementing this now, and featuring a blog post about it. People will know/learn not to use .syntax-- for things not related to syntax highlighting.
  2. Every scope is prepended with .syntax--.
  3. While .syntax-name is fairly common, double-dashes in CSS is much less common, disregarding BEM.

Basically, the chances that someone by accident decides to create something with a class name of .syntax--whatever that isn't related to syntax highlighting and then style it in their CSS file is far lower than before.

Member

50Wliu commented Jan 10, 2017

A few reasons.

  1. We are implementing this now, and featuring a blog post about it. People will know/learn not to use .syntax-- for things not related to syntax highlighting.
  2. Every scope is prepended with .syntax--.
  3. While .syntax-name is fairly common, double-dashes in CSS is much less common, disregarding BEM.

Basically, the chances that someone by accident decides to create something with a class name of .syntax--whatever that isn't related to syntax highlighting and then style it in their CSS file is far lower than before.

@smockle smockle referenced this pull request in smockle/colorful-json Jan 10, 2017

Closed

Deprecated selector in `colorful-json/styles/colorful-json.less` #4

@ryantriangles ryantriangles referenced this pull request in sailorhg/fairyfloss Jan 10, 2017

Open

Update to satisfy Atom 1.13's desires #19

jgebhardt added a commit to facebook/nuclide that referenced this pull request Jan 11, 2017

Fix hyperclick specs in Atom 1.13
Summary: Previously, `HyperclickForTextEditor`'s `mousemove` handler attached to the `textEditorView` element (whose behavior changed in Atom 1.13+, see atom/atom#12903), while `mousedown` was attached to the `linesComponent` inside the `textEditorView`. This caused `mousemove` events to not be handled in specs.

Reviewed By: zertosh

Differential Revision: D4394666

fbshipit-source-id: e7f5e6d4ab48fca4d5a2f5a7dd463092d79146d4

jgebhardt added a commit to facebook/nuclide that referenced this pull request Jan 11, 2017

Backwards-compatible Atom 1.13 shadow DOM fixes
Summary:
See
atom/atom#12903
and
http://blog.atom.io/2016/11/14/removing-shadow-dom-boundary-from-text-editor-elements.html

Reviewed By: vjeux

Differential Revision: D4167548

fbshipit-source-id: d3b9175a1bbcc86218f60196e9e1418b7e1bf6f7

caleb531 added a commit to caleb531/dotfiles that referenced this pull request Jan 11, 2017

jdollar added a commit to jdollar/atom-glowing-cursor that referenced this pull request Jan 12, 2017

Updating deprecated shadow psuedo-selectors
With one of the latest releases of atom the shadow and host psuedoselectors
were deprecated. atom/atom#12903. Removing
those deprecated psuedoselectors as they are no longer needed.

@jdollar jdollar referenced this pull request in Matthew-Smith/atom-glowing-cursor Jan 12, 2017

Merged

Updating deprecated shadow psuedo-selectors #6

@vanossj vanossj referenced this pull request in 31i73/atom-dbg Jan 12, 2017

Merged

removed depreciated shadow DOM #9

@MaherFa MaherFa referenced this pull request in thomaslindstrom/color-picker Jan 14, 2017

Closed

atom-text-editor.Object.defineProperty.get is deprecated. #199

3runoDesign added a commit to 3runoDesign/my-Editor that referenced this pull request Jan 16, 2017

@leggsimon leggsimon referenced this pull request in caleb/gruvbox-syntax-atom Jan 27, 2017

Closed

update selectors #8

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

Fix deprecation warning
Shadow DOM removed from atom-text-editor in
atom/atom#12903

@vt5491 vt5491 referenced this pull request in vt5491/multi-theme-applicator Feb 2, 2017

Closed

Uncaught TypeError: Cannot read property 'getURI' of undefined #2

@simurai simurai referenced this pull request in atom/github Feb 4, 2017

Closed

Add PR timeline events #494

@usagi-f usagi-f referenced this pull request in gnomus/atom-monokai-flat Feb 6, 2017

Closed

Fix deprecation warnings #2

@danielbayley

This comment has been minimized.

Show comment
Hide comment
@danielbayley

danielbayley Feb 21, 2017

Contributor

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?

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

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis 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...

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

This comment has been minimized.

Show comment
Hide comment
@danielbayley

danielbayley Feb 22, 2017

Contributor

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

Contributor

danielbayley commented Feb 22, 2017

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

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Feb 22, 2017

Member

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.

Member

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.

@Antikythera Antikythera referenced this pull request in simeydotme/atom-gooey-syntax Mar 6, 2017

Merged

Add `.syntax--` prefix #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment