RangeError when clearing authorship when line 1 has line attributes

[rhansen]

Describe the bug

Clicking the "clear authorship" button throws a RangeError if the first line in the pad has line attributes:

RangeError: selection starts before line start -- {
  errorId: '5DKIM2Ru2LXCJJhLRegR',
  type: 'Uncaught exception',
  msg: 'RangeError: selection starts before line start',
  url: 'http://localhost:9001/p/testing',
  source: 'http://localhost:9001/static/js/require-kernel.js?v=a88ec0d9 line 226 > Function',
  linenumber: 9390,
  userAgent: 'Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:90.0) Gecko/20100101 Firefox/90.0',
  stack: '_findRowRange@http://localhost:9001/static/js/require-kernel.js?v=a88ec0d9 line 226 > Function:9390:43\n' +
    'setAttributesOnRange@http://localhost:9001/static/js/require-kernel.js?v=a88ec0d9 line 226 > Function:9362:39\n' +
    'performDocumentApplyAttributesToCharRange@http://localhost:9001/static/js/require-kernel.js?v=a88ec0d9 line 226 > Function:1599:30\n' +
    'registerDefaultCommands/<@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=a88ec0d9:2436:15\n' +
    'registerAceCommand/</<@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=a88ec0d9:2161:19\n' +
    'wrapper@http://localhost:9001/static/js/require-kernel.js?v=a88ec0d9 line 226 > Function:646:27\n' +
    'wrapper@http://localhost:9001/static/js/require-kernel.js?v=a88ec0d9 line 226 > Function:652:9\n' +
    'inCallStack@http://localhost:9001/static/js/require-kernel.js?v=a88ec0d9 line 226 > Function:365:16\n' +
    'Ace2Inner/editorInfo.ace_callWithAce@http://localhost:9001/static/js/require-kernel.js?v=a88ec0d9 line 226 > Function:657:25\n' +
    'Ace2Editor/this[fnName]<@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=a88ec0d9:3069:29\n' +
    'action@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=a88ec0d9:3030:31\n' +
    'pendingInit/<@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=a88ec0d9:3031:24\n' +
    'registerAceCommand/<@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=a88ec0d9:2160:13\n' +
    'triggerCommand@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=a88ec0d9:2167:27\n' +
    'init/</<@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=a88ec0d9:2103:16\n' +
    'ep_etherpad-lite/static/js/pad_editbar.js/ToolbarItem.prototype.bind/<@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=a88ec0d9:2035:15\n' +
    'dispatch@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/ace2_common.js?callback=require.define&v=a88ec0d9:5478:27\n' +
    'add/elemData.handle@http://localhost:9001/javascripts/lib/ep_etherpad-lite/static/js/ace2_common.js?callback=require.define&v=a88ec0d9:5286:28\n'
}

To Reproduce
Steps to reproduce the behavior:

1. Create a new pad
2. Make the first line a bulleted list
3. Click "clear authorship"
4. See error

Server (please complete the following information):

• Etherpad version: Current tip of develop (commit ed6fa94)
• OS: Ubuntu 20.04
• Node.js version (node --version): v14.17.2
• npm version (npm --version): 6.14.13

Desktop (please complete the following information):

• OS: Ubuntu 20.04
• Browser: Firefox
• Version: 90

[markNZed]

Same problem here. As a work around you can select text and clear it eventually clearing all the author markup. I get a slightly different error message:

static/js/require-kernel.js?v=3c83cdd2 line 10 > Function at line 49

[JohnMcLear]

Confirmed. Nice debugging!

[JohnMcLear]

Draft PR in with test coverage and patch.

[rhansen]

I think the underlying bug is a violation of the assumptions about where a selection is allowed to start. I couldn't find any documenting comments that answer the following questions:

• Can a selection start at the end of a line? Or must it start at the beginning of the next line instead?
• Can a selection start at the beginning of a line? Or must it start at the end of the previous line instead?
• If a selection is allowed to start at either the end of a line or at the beginning of the next line, are the two equivalent?
• Can a selection end at the end of a line? Or must it end at the beginning of the next line instead?
• Can a selection end at the beginning of a line? Or must it end at the end of the previous line instead?
• If a selection is allowed to end at either the end of a line or at the beginning of the next line, are the two equivalent?
• If there are N lines, can the selection end at the start of the non-existent line with (zero-based) index N? If so, can it start there too?
• Is a selection allowed to start just before a line marker? (The answer to this seems to be "no".)
• Is a selection allowed to start just after a line marker? (The answer to this seems to be "yes".)
• If a selection is allowed to start before or after a line marker, are the two equivalent?
• Is a selection allowed to end just before a line marker?
• Is a selection allowed to end just after a line marker?
• If a selection is allowed to end before or after a line marker, are the two equivalent?
• What does it mean for a line marker to be included in a selection?

I think we need to clearly define the semantics of a selection, then update all callers to adhere.

[rhansen]

My preference would be to do the following:

• Eliminate the confusing and error-prone line marker. Line attributes (e.g., unordered list) would instead be applied to an explicit end-of-line \n character. This character would exist at the end of every line, including the last line.
• Allow selections to start or end just before a line's \n character.
• Prohibit a selection from starting just after the \n character. It must instead start at the beginning of the next line. Ditto for the selection's end. This results in an unambiguous, 1:1 mapping between index-based selections and their {row, column}-based equivalents.
• If there are N lines, a selection is allowed to start or end at the beginning of the non-existent line with (zero-based) index N.
• If the end-of-line \n character is selected, any operations that affect line attributes affect that \n character. Joining two lines discards the second line's attributes (applied to the second \n), and the attributes on the deleted \n character are moved to the second \n character. For example, if the lines and selection look like this:

  foo bar[\n
  ]baz bif\n
  

  where [ and ] mark the start and end of the selection, then:
  • Clicking the bulleted list button toggles the bulleted list attribute on the first line (which is stored in the selected \n character).
  • Typing any text, pressing the delete key, or pressing the backspace key joins the two lines. Any line attributes for the second line are discarded, and the line attributes on the first line are moved to (what used to be) the second \n.

Replacing the line marker with an end-of-line newline would probably take a lot of work, especially to retain compatibility with existing database entries and .etherpad exports. A compromise solution would be:

• Always include the line marker, even if there are no line attributes.
• Allow selections to start or end just after the last character of a line.
• Prohibit a selection from starting just before a line's marker. It must start just after the line marker instead. Ditto for the selection's end.
• If there are N lines, a selection is allowed to start or end just after a phantom line marker for the non-existent line with (zero-based) index N.
• If a line marker is selected, any operations that affect line attributes affect the previous line marker instead. Joining two lines discards the second line's attributes (applied to the deleted marker between the two lines). For example, if the lines and selection look like this:

  *foo bar[
  *]baz bif
  

  where * is the line marker and [ and ] mark the start and end of the selection, then:
  • Clicking the bulleted list button toggles the bulleted list attribute on the first line marker, not the second.
  • Typing any text, pressing the delete key, or pressing the backspace key joins the two lines. Any line attributes applied to the selected line marker are discarded.

[JohnMcLear]

Responses inline, I think about this as getSelection works and caret positioning so it's possible I'm missing some details / getting things confused.

• Can a selection start at the end of a line?

Yes

• Can a selection start at the beginning of a line?

Yes

• If a selection is allowed to start at either the end of a line or at the beginning of the next line, are the two equivalent?

No. "\nfoo" for the former, "foo" for the latter.

• Can a selection end at the end of a line?

Yes.

• Can a selection end at the beginning of a line?

Yes.

• If a selection is allowed to end at either the end of a line or at the beginning of the next line, are the two equivalent?

No, "foo" !== "foo\n". Or in selection terms [[0,0],[0,2]] !== [[0,0],[1,0]]

• If there are N lines, can the selection end at the start of the non-existent line with (zero-based) index N? If so, can it start there too?

Yes. In fact isn't this how Etherpad works? The concept of a non-existent line doesn't exist, an additional line break is always present in the logic afaik.

• Is a selection allowed to start just before a line marker? (The answer to this seems to be "no".)

Line Markers here are some weird horky borky weirdness I really dislike. I don't want to state my opinion on something I'm not happy with anyway.

• Is a selection allowed to start just after a line marker? (The answer to this seems to be "yes".)
• If a selection is allowed to start before or after a line marker, are the two equivalent?
• Is a selection allowed to end just before a line marker?
• Is a selection allowed to end just after a line marker?
• If a selection is allowed to end before or after a line marker, are the two equivalent?
• What does it mean for a line marker to be included in a selection?

I didn't answer the questions about line markers because I think the implementation of line markers requires refinement. I think assigning the line marker to the line break is a much better approach than having a hidden character. However, I'm sure this will introduce a whole new world of edge cases. I'd say current UX is subpar tho and this has been reported in various plugins and afaik @Luiza attempted to fix it while at @storybook but we didn't get it done. I remember she wrote tests that might still exist somewhere which really highlight the problem.

One final thing to note, while the UX isn't great, the number of plugins setting line attributes using the hacky * method is going to cause a problem to migrate from and I'm not sure it's worth the fight vs just throwing in this ghetto patch to keep the UX "working" as far as the user can see... I'm mindful this could turn into a severe amount of disruption to do it "right" where as just applying this patch to mute the problem while maintaining the correct UX. TLDR; be careful of the scope dragon.

[rhansen]

For now, let's allow the selection to start/end before the line marker, but treat it the same as if it started/ended after the line marker. Sound OK?

[JohnMcLear]

@rhansen yep!