Bug 69022

Summary: Get rid of EditingText
Product: WebKit Reporter: Ryosuke Niwa <rniwa@webkit.org>
Component: HTML EditingAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: NEW    
Severity: Normal CC: cshu@webkit.org, darin@apple.com, dglazkov@chromium.org, enrica@apple.com, hyatt@apple.com, justin.garcia@apple.com, kaustubh.ra@gmail.com, leviw@chromium.org, sullivan@chromium.org, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
rniwa: review-
Updated Patch
webkit.review.bot: commit‑queue-
Updated Patch cshu: review-, webkit.review.bot: commit‑queue-

Description From 2011-09-28 13:16:30 PST
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.
------- Comment #1 From 2011-10-02 23:49:40 PST -------
Created an attachment (id=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?
------- Comment #2 From 2011-10-03 00:02:01 PST -------
I will be sending out full patch with removed EditingText files (.cpp/.h) and updated Makefiles as well once this patch is approved.
------- Comment #3 From 2011-10-03 00:16:17 PST -------
(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.
------- Comment #4 From 2011-10-03 00:24:29 PST -------
(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)
------- Comment #5 From 2011-10-03 00:25:44 PST -------
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 109446 [details] [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.
------- Comment #6 From 2011-10-03 00:42:09 PST -------
(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.
------- Comment #7 From 2011-10-03 07:52:21 PST -------
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.
------- Comment #8 From 2011-10-07 04:07:08 PST -------
(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.
------- Comment #9 From 2011-10-07 10:06:31 PST -------
(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.
------- Comment #10 From 2011-10-18 03:51:17 PST -------
(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.
------- Comment #11 From 2011-10-20 07:22:21 PST -------
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.
------- Comment #12 From 2011-10-20 09:06:01 PST -------
(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
------- Comment #13 From 2011-10-20 10:18:39 PST -------
(In reply to comment #11)
> Created an attachment (id=111767) [details] [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] [details])
> Attachment 111767 [details] [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?
------- Comment #14 From 2011-10-20 11:18:59 PST -------
(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
------- Comment #15 From 2011-10-20 11:37:08 PST -------
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).
------- Comment #16 From 2011-10-20 19:04:29 PST -------
(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?
------- Comment #17 From 2011-10-20 23:44:56 PST -------
Created an attachment (id=111911) [details]
Updated Patch

Added Rebaseline for removing EditingText
------- Comment #18 From 2011-10-21 00:04:39 PST -------
(From update of attachment 111911 [details])
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.
------- Comment #19 From 2011-10-21 00:27:44 PST -------
(From update of attachment 111911 [details])
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
------- Comment #20 From 2011-10-21 06:22:14 PST -------
> > 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.
------- Comment #21 From 2011-10-21 06:35:27 PST -------
(From update of attachment 111911 [details])
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.
------- Comment #22 From 2011-10-21 06:39:24 PST -------
>. 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.