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

New table lexer does not allow for selection of code #1274

Closed
jneen opened this issue Jul 24, 2019 · 11 comments · Fixed by #1276
Closed

New table lexer does not allow for selection of code #1274

jneen opened this issue Jul 24, 2019 · 11 comments · Fixed by #1276
Labels
enhancement-request A request for an enhancement to be developed.

Comments

@jneen
Copy link
Member

jneen commented Jul 24, 2019

I have suddenly remembered the reason we used the single-row table approach. While the new table lexer does allow for wrapping of lines, when a user attempts to select code, the line numbers become included in the selection. I'm not sure if there is a way around this, but I would like to look into it sometime in the near future.

@jneen
Copy link
Member Author

jneen commented Jul 24, 2019

cc @ashmaroli

@ashmaroli
Copy link
Contributor

I believe this can be easily overcome by CSS:

.rouge-line-table .rouge-gutter {
  -moz-user-select: none;
  -webkit-user-select: none;
  user-select: none;
}

@jneen
Copy link
Member Author

jneen commented Jul 24, 2019

Is there a way to include this in the default configuration? I would honestly be okay inlining this sort of thing.

@pyrmont pyrmont added the enhancement-request A request for an enhancement to be developed. label Jul 24, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jul 24, 2019

Perhaps somewhere around here?

rouge/lib/rouge/theme.rb

Lines 171 to 173 in 73662c9

# shared styles for tableized line numbers
yield "#{@scope} table td { padding: 5px; }"
yield "#{@scope} table pre { margin: 0; }"

@ashmaroli
Copy link
Contributor

@pyrmont I think the styles above should be inlined only for the HTMLLineTable formatter and not for all formatters / themes..

@pyrmont
Copy link
Contributor

pyrmont commented Jul 24, 2019

@ashmaroli If the above rules are inserted regardless of whether the formatter is inserting a table or not, why not also insert rules about line selection?

@ashmaroli
Copy link
Contributor

@pyrmont There are couple of reasons:

  • The above code (the lines you've linked to) will probably be removed in Rouge 4.0 (even though there hasn't been an official decision yet).
  • We're aiming to disable selection of line-number cell only and since the cells' classname is configurable, we'd have to select the cell(s) by the element tags (i.e. table td:first-of-type)
  • By having the above hardcoded for all formatters, there's a chance we may affect a custom formatter downstream, especially if that formatter outputs the table with the first column for some other purpose other than housing preformatted line-number cells.
  • By having the styles hardcoded only for html-line-table, the effects of (those syles) are contained within a single formatter (that is not even activated by default via the Legacy Formatter).

@pyrmont
Copy link
Contributor

pyrmont commented Jul 25, 2019

@ashmaroli These are good counterarguments. My objection is grounded in the fact that this feels like a breaking change.

That said, while I think that's technically true I do acknowledge:

  1. this formatter is unlikely to be in widespread use given when it was added;
  2. the proposed rules are very narrow and unlikely to cause unintended behaviour;
  3. you can always create a custom formatter if, for some reason, you wanted the original behaviour.

After considering this further, I'd say that I'm OK with the approach proposed.

@ashmaroli
Copy link
Contributor

Thanks Michael. And I wouldn't consider it a breaking-change. Just enhancing / fixing a very recently released feature. ( It is subjective to how the situation is perceived 😉 ).

@pyrmont
Copy link
Contributor

pyrmont commented Jul 25, 2019

Again, it's somewhat academic given the scope of the changes but I strongly disagree. Using this formatter previously allowed content in the gutter to be selected. Now that functionality is both gone and cannot be enabled as the CSS rules are to be inserted inline and so cannot be overridden.

@ashmaroli
Copy link
Contributor

Ah! That's true, indeed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement-request A request for an enhancement to be developed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants