Richpad editing - new line cancels attribute toggle off

[Mika Genic]

A strange bug here..

To reproduce, start a new line, click bold, type a few characters, click bold again (toggle off), press enter (new line), type a few characters

You will see that the characters on the second line are still bold despite toggling bold off before pressing enter.

can anyone comment on fixing this ?

Thanks!

[Michael Lehenbauer]

Hrm, interesting.

I'm guessing that after you insert a newline, we (for some reason) end up calling RichTextCodeMirror.updateCurrentAttributes_() which basically walks backwards over newlines to find the first actual character and copies the attributes from that to currentAttributes_ so we're picking up the attributes from the previous line.

We probably need to figure out why updateCurrentAttributes_ is being called and try to avoid it (it's meant to run after you click in a random place in the text).  For reference the code that runs to implement the newline functionality is here.

If you feel like digging into it, awesome.  Else, can you add it to the issue list?  

Thanks!

-Michael

--
You received this message because you are subscribed to the Google Groups "Firepad" group.
To unsubscribe from this group and stop receiving emails from it, send an email to firepad-io+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/firepad-io/36885643-bd09-4653-9f09-c4bf08720b28%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Mika Genic]

This bug can also be reproduced by pasting text under a line that ended with a rich formatted text (ie bold)

Looking at the code you attached (RichTextCodeMirror.prototype.newline) it explicitly calls updateLineAttributes and documents that with // Copy line attributes forward.

Are you saying this call should be removed ? I don't know what other implications this may have ?

Thx!

[Michael Lehenbauer]

Oh, interesting.  Well, you could try removing it and see if something else breaks.  Offhand, I don't remember why that's there.  I'd kind of expect that currentAttributes_ would be preserved across a newline automatically, but that comment certainly suggests otherwise.  Sorry, I don't know without digging into the code.

-Michael

To view this discussion on the web visit https://groups.google.com/d/msgid/firepad-io/0f256154-d0e6-4ec5-8d20-69fe262d5be6%40googlegroups.com.

[Mika Genic]

This issue makes the basic operation of creating a heading  buggy, it is very common for a user to make a line bold and/or bigger font as a heading, then press enter to type the normal text.

without knowing much about the code, maybe updateCurrentAttributes_ is the wrong direction ? even if this updateCurrentAttributes_ is called, why would it produce this bug ? Since we toggled off the attribute (bold), why is it copied from the line before ?

[Mika Genic]

Hi Michael

I tried to trace through the code but can no luck.

I tried to compare the two cases, one in which the letter before pressing new line has an attribute (bold) and second when the letter before pressing new line does not have an attribute (plain text).

In RichTextCodeMirror.prototype.newline, I Put a debugger breakpoint inside the callback in
    this.updateLineAttributes(cursorLine+1, cursorLine+1, function(attributes) {
it seems that the 'attributes' object is the same in both cases (containing just the sentinel attribute it seems).

This makes me again think the problem is not in RichTextCodeMirror.prototype.newline

Can you help me think about where this may originate ? is there another place where the code looks at the line above to determine the attribute of the current line ?

Thanks!

[Mika Genic]

Ok I looked further...

The problem seems to issue from RichTextCodeMirror.prototype.updateCurrentAttributes_, which is called continuously from RichTextCodeMirror.prototype.onCursorActivity_

The code always tries to pick up the attributes (getAnnotatedSpansForPos) of the previous character for the current character. 

This makes sense when you enter a new character inside an annotated span (ie a bolded text), but not in our case.

What I tried was to modify the code so that if we are on the first character of the line, do not use any attributes of the line before..

So in RichTextCodeMirror.prototype.updateCurrentAttributes_

Instead of 

    } else {

      // Back up before any newlines or line sentinels.

      while(pos > 0) {

        c = this.getRange(pos-1, pos);

        if (c !== '\n' && c !== LineSentinelCharacter)

          break;

        pos--;

      }

    }

I did:

    } else {

      // Back up before any newlines or line sentinels.

      c = this.getRange(pos-1, pos);

      if (c=='\n' || c==LineSentinelCharacter) { // a new line always starts with no attributes

        this.currentAttributes_ = {};

        return;

      }

      while(pos > 0) {

        c = this.getRange(pos-1, pos);

        if (c !== '\n' && c !== LineSentinelCharacter)

          break;

        pos--;

      }

    }

This is not perfect at all. It means that you loose any formatting on a new line. So if for example you type in bold and then press new line, bold is automatically turned off. However it is much better as far as the original issue is- that is when starting to type in a new line you don't surprisingly get the style of the line before.

I would be very grateful if you can have a quick look at this and give any ideas into a more proper solution ?

Thx !  

[Michael Lehenbauer]

Thanks Mika,

I don't think this is the right fix, but I am curious what happens if you remove "this.currentAttributes_ = {};" from your modified version.  Basically I'm wondering if, for the newline case, we should skip updateCurrentAttributes_ entirely and just keep the attributes that we have.

Thanks,

-Michael

--

You received this message because you are subscribed to the Google Groups "Firepad" group.
To unsubscribe from this group and stop receiving emails from it, send an email to firepad-io+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/firepad-io/3dd5aa02-7b0c-4504-8f35-6b57f27372d2%40googlegroups.com.

[Mika Genic]

Ok I think I found a solution (ignore the fix before)

I modified the code as follows (changes in red):

  RichTextCodeMirror.prototype.updateCurrentAttributes_ = function() {
    var cm = this.codeMirror;
    var anchor = cm.indexFromPos(cm.getCursor('anchor')), head = cm.indexFromPos(cm.getCursor('head'));
    var pos = head;
    var newline=false;
    if (anchor > head) { // backwards selection
      // Advance past any newlines or line sentinels.
      while(pos < this.end()) {
        var c = this.getRange(pos, pos+1);

        if (c !== '\n' && c !== LineSentinelCharacter)
          break;

        pos++;
      }
      if (pos < this.end())
        pos++; // since we're going to look at the annotation span to the left to decide what attributes to use.
    } else {
      c = this.getRange(pos-1, pos);
      if (c=='\n' || c==LineSentinelCharacter) newline=true;

      // Back up before any newlines or line sentinels.
      while(pos > 0) {
        c = this.getRange(pos-1, pos);
        if (c !== '\n' && c !== LineSentinelCharacter)
          break;
        pos--;
      }
    }

    var spans = this.annotationList_.getAnnotatedSpansForPos(pos);
    this.currentAttributes_ = {};

    var attributes = {};
    // Use the attributes to the left unless they're line attributes (in which case use the ones to the right.
    if (spans.length > 0 && (!(ATTR.LINE_SENTINEL in spans[0].annotation.attributes))) {
      attributes = spans[newline? spans.length-1 : 0].annotation.attributes;

    } else if (spans.length > 1) {
      firepad.utils.assert(!(ATTR.LINE_SENTINEL in spans[1].annotation.attributes), "Cursor can't be between two line sentinel characters.");
      attributes = spans[1].annotation.attributes;
    }
    for(var attr in attributes) {
      // Don't copy line or entity attributes.
      if (attr !== 'l' && attr !== 'lt' && attr !== 'li' && attr.indexOf(ATTR.ENTITY_SENTINEL) !== 0) {
        this.currentAttributes_[attr] = attributes[attr];
      }
    }
  };

Basically when starting a new line I use the second attribute in spans (if existing) instead of the first one.

As far as I can tell this solves the issue of new lines picking the attributes of the last character of the line above even when the attribute was toggled off.

I would appreciate it if you can have a look and tell me what you think.

Thanks!

[Mika Genic]

(some of the post above got colored gray for some  reason ... I only changed the parts in red)

[Michael Lehenbauer]

Thanks Mika,

Offhand I don't think this is probably quite the right approach since:

1. The "this.currentAttributes_ = {};" is going to clear the current attributes (e.g. if you had turned on bold), so I actually don't understand how your fix works to solve the problem.
2. updateCurrentAttributes_ is called any time the cursor moves (e.g. you click with the mouse to place the cursor) and so I suspect your code will "kick in" any time you move the cursor to after a newline, which doesn't sound correct to me.
   

Maybe you can just log a github issue and I'll look at it when I get a chance?

Thanks,

-Michael

On Thu, Oct 8, 2015 at 3:05 AM, Mika Genic <mika...@gmail.com> wrote:

(some of the post above got colored gray for some  reason ... I only changed the parts in red)

--

You received this message because you are subscribed to the Google Groups "Firepad" group.
To unsubscribe from this group and stop receiving emails from it, send an email to firepad-io+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/firepad-io/0e2182a0-003e-4695-a3bd-e3c5e897bf26%40googlegroups.com.

[Mika Genic]

pls look again at the changes I've done (in red). I did not add the this.currentAttributes_ = {}; it is in the original code.

My changes simply pick a different attribute from the annotationList at the position (the second instead of the first one).

The code needs to kick in every time the cursor moves in a the beginning of the line to pick up the correct attribute from the line before in the same way it needs to kick in every time the cursor moves inside a line to pick up the correct attribute from the letter before.

thx!

[Mika Genic]

ok I created a pull request: https://github.com/firebase/firepad/pull/204

I'm pretty new to github so excuse me if I've done something terribly wrong ...)

 

[Mika Genic]

I want to raise this issue again as it's becoming more pertinent for me.

I discovered another related issue, start a new line and press ctrl+b bla ctrl+b

then move the cursor at the beginning of the line (before the 'b' of 'bla') and type

text comes up as plain instead of bold

together with the original issue this creates a confusing user experience.

I would love some help with that.

thanks!

[Mika Genic]

and looking more into the code of updateCurrentAttributes_ it seems to be saying:

if we're at the beginning of the line use the attribute of the line before

otherwise use the attribute of the letter to the right

where I think what we need is:

if we're at the beginning of the line and the line is empty use the attribute of the line before

otherwise use the attribute of the letter to the right

would that be correct ?

[Mika Genic]

I submitted https://github.com/firebase/firepad/pull/241

This PR fixes updateCurrentAttributes_ to correctly handle attribute changes.

Two issues are fixed,:

First the original code did not properly handle attribute changes at the end of the line, for example turning bold off and then inserting a newline would still make text on the new line bold. 

The reason was the original code backs up before the newline which holds this attribute change. The new code backs up to the correct pos.

Secondly the original code did not properly handle cursor at the beginning of a non empty line - it would copy the attribute s from the left (line before) and not the right (first char). The new code handles this situation correctly.

Please have a look!

This replaces https://github.com/firebase/firepad/pull/204 which I now closed