Bug 69022 – Get rid of EditingText

• Component: HTML Editing
• Status: NEW

[Ryosuke Niwa]

http://trac.webkit.org/browser/trunk/Source/WebCore/dom/EditingText.h

It seems like introducing WebCore-only node is a bad idea because it breaks down whenever the page is reloaded or rendered by other engines.

For the reference, this class was initially introduced by http://trac.webkit.org/changeset/6341/trunk/WebCore/khtml/xml/dom_textimpl.cpp. Dave told me this class was introduced to let users edit inside an empty block.

[Kaustubh Atrawalkar]

Created attachment 109446 [details]
Patch

Perhaps not proper way but just giving a try to Get rid of EditingText. Not sure if its right. 
Ryosuke can you check once?

[Kaustubh Atrawalkar]

I will be sending out full patch with removed EditingText files (.cpp/.h) and updated Makefiles as well once this patch is approved.

[Ryosuke Niwa]

Comment on attachment 109446 [details]
Patch

No, this is what we want to do. I'd like to get rid of the heck in rendererIsNeeded. This just moves the hack from one place to another.

[Kaustubh Atrawalkar]

(In reply to comment #3)
> (From update of attachment 109446 [details])
> No, this is what we want to do. I'd like to get rid of the heck in rendererIsNeeded. This just moves the hack from one place to another.

Agree, then is it that we need to allow empty blocks to be editable, by changing InsertTextCommand itself where its inserting empty textNode (EditingText node as of now)

[Ryosuke Niwa]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 109446 [details] [details])
> > No, this is what we want to do. I'd like to get rid of the heck in rendererIsNeeded. This just moves the hack from one place to another.
> 
> Agree, then is it that we need to allow empty blocks to be editable, by changing InsertTextCommand itself where its inserting empty textNode (EditingText node as of now)

Yeah, I thought we normally insert br in empty blocks but maybe br has zero-height? We just need to figure out other ways to make it work.

[Kaustubh Atrawalkar]

(In reply to comment #5)
> 
> Yeah, I thought we normally insert br in empty blocks but maybe br has zero-height? We just need to figure out other ways to make it work.

Got it. May be we can use white spaces? They do create renderer.

[Darin Adler]

What we need is a set of tests covering all the good effects of EditingText. Then we can try out different ways of accomplishing the same thing. We should design the tests so that they test only the actual end-user-visible effects and don’t expose the internals.

[Kaustubh Atrawalkar]

(In reply to comment #5)
> Yeah, I thought we normally insert br in empty blocks but maybe br has zero-height? We just need to figure out other ways to make it work.

I had a look through the code and found that EditingText is used to create empty text nodes having renderer in InsertTextCommand, htmlediting (for tabSpan) and ReplaceCommandSelection . I could not find other way to make blank text node with renderer. We might have to just add a special case for Text node as i tried in previous patch or we might have to change the logic wherever EditingText is being used. Any guidelines/suggestions will be helpful. 

(In reply to comment #7)
> What we need is a set of tests covering all the good effects of EditingText. Then we can try out different ways of accomplishing the same thing. We should design the tests so that they test only the actual end-user-visible effects and don’t expose the internals.

True, I can try to get some good tests covering EditingText use cases.

[Ryosuke Niwa]

(In reply to comment #8)
> (In reply to comment #5)
> > Yeah, I thought we normally insert br in empty blocks but maybe br has zero-height? We just need to figure out other ways to make it work.
> 
> I had a look through the code and found that EditingText is used to create empty text nodes having renderer in InsertTextCommand, htmlediting (for tabSpan) and ReplaceCommandSelection . I could not find other way to make blank text node with renderer. We might have to just add a special case for Text node as i tried in previous patch or we might have to change the logic wherever EditingText is being used. Any guidelines/suggestions will be helpful.

I think the only reason we do that is to allow empty blocks editable but we can accomplish the same by inserting a br. Whether it's an empty text node or br isn't important as long as you can edit an empty block.

[Kaustubh Atrawalkar]

(In reply to comment #9)
> 
> I think the only reason we do that is to allow empty blocks editable but we can accomplish the same by inserting a br. Whether it's an empty text node or br isn't important as long as you can edit an empty block.

Adding br element is breaking many of the current use cases. Also adding new known tag element inside editing block might create problem for java script which might be listening or operating on br tag may be.

[Kaustubh Atrawalkar]

Created attachment 111767 [details]
Updated Patch

Instead of inserting br, i tried with empty text nodes which does allows editing inside empty block as well.

[WebKit Review Bot]

Comment on attachment 111767 [details]
Updated Patch

Attachment 111767 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10183485

New failing tests:
editing/inserting/insert-at-end-02.html
editing/input/password-echo-passnode.html

[Ryosuke Niwa]

(In reply to comment #11)
> Created an attachment (id=111767) [details]
> Updated Patch
> 
> Instead of inserting br, i tried with empty text nodes which does allows editing inside empty block as well.

That's interesting...

(In reply to comment #12)
> (From update of attachment 111767 [details])
> Attachment 111767 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10183485
> 
> New failing tests:
> editing/inserting/insert-at-end-02.html
> editing/input/password-echo-passnode.html

But these two tests are failing. Did you run layout tests on whatever platform you're working on?

[Kaustubh Atrawalkar]

(In reply to comment #13)
> > 
> > New failing tests:
> > editing/inserting/insert-at-end-02.html
> > editing/input/password-echo-passnode.html
> 
> But these two tests are failing. Did you run layout tests on whatever platform you're working on?

Yes I did, but I tried all those test cases which were added specifically for EditingText mainly editing empty blocks test cases. insert-at-end-02.html test is actually working with GtkLauncher but has one diff added (RenderText node added at the end) which can be modified. 2nd test cases seems to be related to setPasswordEchoEnabled feature added here - https://trac.webkit.org/changeset/93291 and has again a diff where it is checking for right characters is secured or not. I am debugging this currently but you can suggest if we can modify the expected.txt

[Ryosuke Niwa]

Rebaseline is fine as long as the test passes semantically (i.e. no odd behavior, etc... and the feature the test claims to work as expected).

[Kaustubh Atrawalkar]

(In reply to comment #15)
> Rebaseline is fine as long as the test passes semantically (i.e. no odd behavior, etc... and the feature the test claims to work as expected).

For 1) insert-at-end-02.html -> The test expectation is check the visual position at the end of an editable block. This test has no changes as per user perspective other than adding an empty Text Node at the end of editable block in the render tree. 

For 2) password-echo-passnode.html -> This tests if the passwords entered can be secured right after or after a delay. Initially the test expectation was secure right after will fail and only secure after delay works. After the changes, secure right after for regular texts is success. Again no change from user perspective.

Is re-baseline possible in the both cases?

[Kaustubh Atrawalkar]

Created attachment 111911 [details]
Updated Patch

Added Rebaseline for removing EditingText

[Ryosuke Niwa]

Comment on attachment 111911 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111911&action=review

> LayoutTests/editing/input/password-echo-passnode.html:6
> -        [['a'], false, true, true],         // test password (when only 1 char) is only secured after a delay(regular).
> +        [['a'], true, true, true],         // test password (when only 1 char) is only secured after a delay(regular).

Why is it okay to change this expectation?

> LayoutTests/platform/gtk/editing/inserting/insert-at-end-02-expected.txt:30
> +          RenderText {#text} at (0,0) size 0x0

Why are adding an empty RenderText now? Ideally, we'd like to remove these.

[WebKit Review Bot]

Comment on attachment 111911 [details]
Updated Patch

Attachment 111911 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10178754

New failing tests:
editing/inserting/insert-at-end-02.html

[Chang Shu]

> > LayoutTests/editing/input/password-echo-passnode.html:6
> > -        [['a'], false, true, true],         // test password (when only 1 char) is only secured after a delay(regular).
> > +        [['a'], true, true, true],         // test password (when only 1 char) is only secured after a delay(regular).
> 
> Why is it okay to change this expectation?

It looks to me the code breaks the original test case.

[Chang Shu]

Comment on attachment 111911 [details]
Updated Patch

I have to r- this patch since it breaks the test case. Btw, the test case [['a'], false, true, true] means you enter 'a' and you expects 'a' remains the same and after a delay it changes to '*'. The last 'true' simply tells the test engine to check the expectations before running to the 2nd line. While in password-echo-passnode2.html, you don't check result for 1st line, i.e., you type two characters without a delay in between. I suggest you put some breakpoints in my original patch to debug this.

[Chang Shu]

>. I suggest you put some breakpoints in my original patch to debug this.

Another way to debug it is to enable password echo (Settings.cpp) and run it in your browser. This should be easier than debugging DRT.