1005776 - execCommand indent doesn't maintain DOM structure of hidden elements within selected text

Status: NEW

(Core :: DOM: Editor, defect, P5)

Description

[Sam Hemelryk]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140428193813

Steps to reproduce:

I've created a JSFiddle to reproduce this http://jsfiddle.net/2TN33/12/

Alternatively:
# Create an editable content area
# Give it this content: This <span id="foo" style="line-height: 0; display: none;">&#65279;</span>is a<span id="bah" style="line-height: 0; display: none;">&#65279;</span> test
# Select the text using your mouse
# Trigger the following: document.execCommand('indent', false, null);


Actual results:

The HTML content for the editable area changed to:

<blockquote> This is a test </blockquote><span id="foo"  style="line-height: 0; display: none;"></span><span id="bah"  style="line-height: 0; display: none;"></span>

Notice how the two spans are now after the blockquote.


Expected results:

I would expect the two spans (both hidden) would remain in place within the text.
I do not expect them to be moved outside of the indent.

Comment 1

[Sam Hemelryk]

Just to add background.

This was encountered while working with the Rangy (https://code.google.com/p/rangy/) library and is reproduced when using the rangy.saveSelection() method.
It causes rangy.restoreSelection() to put the selection back into the wrong place as obviously its markers (the spans) have being moved outside of the selected, indented content.

Comment 2

[:Gijs (he/him)]

Does this only happen with the blockquote command, or also with other editor commands? And do you know if this is a regression (id est, did it used to work on Firefox?)

Comment 3

[Sam Hemelryk]

Hi Gijs,

In practise I've only encountered this using the indent command. However I've completed the following command testing using http://jsfiddle.net/2TN33/18/, the results are:

backcolor : OK
bold : OK
createlink : OK
decreasefontsize : OK
fontsize : OK
fontcolor : OK
formatblock (blockquote) : Fail - spans moved outside of blockquote.
formatblock (div) : Fail - spans moved outside of div.
heading (h1) : Fail - spans moved outside of h1.
indent : Fail - spans moved outside of blockquote.
italic : OK
strikethrough : OK
subscript : OK
underline : OK

In summary formatblock, heading, and indent are failing in my test.

Unfortunately I am not able to test for you whether this is a regression or not. I've only a Firefox 29 available for testing presently and am not able to commit time to this testing presently sorry.

Hope that helps.

Comment 4

[:Gijs (he/him)]

(In reply to Sam Hemelryk from comment #3)
> Hi Gijs,
> 
> In practise I've only encountered this using the indent command. However
> I've completed the following command testing using
> http://jsfiddle.net/2TN33/18/, the results are:
> 
> backcolor : OK
> bold : OK
> createlink : OK
> decreasefontsize : OK
> fontsize : OK
> fontcolor : OK
> formatblock (blockquote) : Fail - spans moved outside of blockquote.
> formatblock (div) : Fail - spans moved outside of div.
> heading (h1) : Fail - spans moved outside of h1.
> indent : Fail - spans moved outside of blockquote.
> italic : OK
> strikethrough : OK
> subscript : OK
> underline : OK
> 
> In summary formatblock, heading, and indent are failing in my test.
> 
> Unfortunately I am not able to test for you whether this is a regression or
> not. I've only a Firefox 29 available for testing presently and am not able
> to commit time to this testing presently sorry.
> 
> Hope that helps.

Yes, thank you!

Comment 5

[(no longer active)]

Given the fact that we don't really have any code to make sure to handle this properly across the board, I would expect lots of editor commands to be broken in the same way...  :(

Is rangy a popular library?

Comment 6

[Sam Hemelryk]

Hi Ehsan,

I can't really talk to the popularity of the Rangy library sorry, but I can tell you about how we use it if that is any help.

I am a developer at Moodle (http://moodle.org) and for our upcoming release we've created a contenteditable editor and as part of that work have pulled in the Rangy library for cross-browser selection support.
Within Moodle we encountered this issue while working on a solution for https://tracker.moodle.org/browse/MDL-44224 wherein we wanted to change indent from producing blockquotes to producing divs with inline styles.
The impact of this issue for Moodle is trivial, it affects the usability of our editor as upon an indent occurring in certain conditions the users current selection is lost. The worst outcome of which is that if the user triggers the indent action more than once in succession they may experience the wrong element being acted upon.
This occurs because Rangy uses the spans to re-select the users original selection after we replace blockquotes with divs in the DOM.
That is all very specific to the project we are working on of course, and the end bug for us can be traced back to this one issue when the user is using Firefox.

Anything from me on the how widely used Rangy is or how popular it is outside of the scope of Moodle would be purely speculation.
Moodle having chosen to make use of it will in the future mean that tens of thousands of sites when upgraded will be in turn using it as part of our new editor.
Again though - it is a minor issue to us. I don't imagine people will realise in most cases what has happened.
It'd be great to see it fixed of course still!

Do let me know if there is anything more I can do to help out here and I'll see what I can do.

Comment 7

[(no longer active)]

Hey Sam, Thanks a lot for the detailed answer.  The reason I asked was to try to get a sense of the importance of this bug in terms of how many websites it's going to affect.  Unfortunately this area of our code base is not actively worked on, and I think properly fixing this bug will require a considerable amount of effort.

Is this something that you expect to affect all of those tens of thousands of websites using Moodle?  Are there any workarounds that you think you can make in your code to work around this bug?

Comment 8

[Sam Hemelryk]

Hi Ehsan, I wouldn't give this any weight in your queue at present in relation to us.
I've discussed this with my colleagues and the consensus is that it is minor, and will not block our upcoming release nor any future releases presently.
We consider its affects minor enough that we won't be working on a workaround in Moodle in the near future unless something changes. However we've opened a bug for it on our issue tracker https://tracker.moodle.org/browse/MDL-45420.
Should we end up seeing users commenting or voting then we will absolutely look for a workaround.
If we find a work around I will come back here to share it.
If someone pops up to fix this in Firefox before we get there all the better.
For now we are quite happy to see this sit here as is and we'll keep a lazy eye on it.
Thanks for the interest.

Comment 9

[Sam Hemelryk]

Ah - we've had another bug identified in Firefox by the looks, in the same area as this but with different experience.
Using http://jsfiddle.net/GFv77/4/ select the indent option in the dropdown, then select the second paragraph, and now click "Test: press me". Note that nothing happens. The expected outcome would be for the paragraph to be indented.
Tested in Chrome as as a cross reference where it works as expected.
We've got an issue in our tracker for this https://tracker.moodle.org/browse/MDL-45422

Would you like me to report this as a separate issue or are you happy with it simply being mentioned here?

Comment 10

[:Gijs (he/him)]

(In reply to Sam Hemelryk from comment #9)
> Ah - we've had another bug identified in Firefox by the looks, in the same
> area as this but with different experience.
> Using http://jsfiddle.net/GFv77/4/ select the indent option in the dropdown,
> then select the second paragraph, and now click "Test: press me". Note that
> nothing happens. The expected outcome would be for the paragraph to be
> indented.
> Tested in Chrome as as a cross reference where it works as expected.
> We've got an issue in our tracker for this
> https://tracker.moodle.org/browse/MDL-45422
> 
> Would you like me to report this as a separate issue or are you happy with
> it simply being mentioned here?

Ehsan would know best whether this is the same issue at its core.

Comment 11

[(no longer active)]

Please file that as a separate bug.  Thanks!

Comment 12

[Sam Hemelryk]

Great thanks guys will do - new bug is at https://bugzilla.mozilla.org/show_bug.cgi?id=1006793
Noting the colleague who found the above bug was using Firefox 26 and confirmed he could reproduce this bug in that particular version of Firefox as well.

Cheers
Sam Hemelryk

Comment 13

[[:Aleksej]]

-g 2011-08-08-03-08-04-mozilla-central-firefox-8.0a1.en-US.linux-x86_64
-g 2011-08-24-03-08-13-mozilla-central-firefox-9.0a1.en-US.linux-x86_64
-b 2011-08-25-03-08-23-mozilla-central-firefox-9.0a1.en-US.linux-x86_64
-b 2014-05-13-03-02-01-mozilla-central-firefox-32.0a1.ru.linux-x86_64
Same range as that of bug 1006793, see bug 1006793 comment 3.

Comment 14

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, >=5 years untouched DOM/Storage bugs' priority.

If you have reason to believe this is wrong (especially for the severity), please write a comment and ni :jstutte.