Bug 54179 - Change text-based <input> types to use the new shadow DOM model
Summary: Change text-based <input> types to use the new shadow DOM model
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
: 52456 54174 54175 54177 54178 (view as bug list)
Depends on: 52788 52917 54853 55355 58432 58704 61265 61266
Blocks: 7651 27683 45889 60818 61166 61909
  Show dependency treegraph
 
Reported: 2011-02-10 00:31 PST by Roland Steiner
Modified: 2011-06-01 23:35 PDT (History)
12 users (show)

See Also:


Attachments
work-in-progress (65.56 KB, patch)
2011-03-04 00:53 PST, Roland Steiner
no flags Details | Formatted Diff | Diff
work-in-progress 2 (102.11 KB, patch)
2011-04-12 08:52 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
work-in-progress 3 (132.60 KB, patch)
2011-04-15 04:37 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch (275.68 KB, patch)
2011-04-22 13:02 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (299.79 KB, patch)
2011-05-19 21:06 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (300.35 KB, patch)
2011-05-19 21:13 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.48 MB, application/zip)
2011-05-19 22:09 PDT, WebKit Review Bot
no flags Details
Patch 4 (301.51 KB, patch)
2011-05-19 22:17 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch for landing (299.94 KB, patch)
2011-05-20 22:36 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
backtrace from Qt debug bot (10.45 KB, text/plain)
2011-05-23 01:18 PDT, Csaba Osztrogonác
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Steiner 2011-02-10 00:31:26 PST
Input types:

    Text
    Password
    Email
    URL
    Telephone

"search" is more complicated and got its own bug: https://bugs.webkit.org/show_bug.cgi?id=54178
Comment 1 Dominic Cooney 2011-02-13 18:33:38 PST
I believe this bug should also cover textarea and input[type=color].
Comment 2 Roland Steiner 2011-03-04 00:53:16 PST
Created attachment 84705 [details]
work-in-progress

Work in progress, doesn't run yet, mainly to gather early feedback and see what breaks (disregard r?)
Comment 3 Roland Steiner 2011-03-04 01:08:30 PST
Some notes on the work-in-progress patch:

.) It also includes type=search (hence the renaming of this bug entry), but search should really have it's own RenderTextControl sub-type.

.) The shadow DOM objects are so far largely unchanged from the original code. Layouting is still handled by the RenderTExtControlSingleLine. If possible, I would keep this arrangement for the initial version, and move the layouting logic to the shadow Elements in subsequent bugs/patches.

.) There is some inefficiency that the shadow DOM for type=text is created by default, then destroyed and overwritten once the type attribute is read. Not sure how we can fix this (or whether it really matters).
Comment 4 Kent Tamura 2011-03-04 01:21:03 PST
(In reply to comment #3)
> .) There is some inefficiency that the shadow DOM for type=text is created by default, then destroyed and overwritten once the type attribute is read. Not sure how we can fix this (or whether it really matters).


Can't we delay calling InputType::createShadowSubtree() until attach()?
Comment 5 Dimitri Glazkov (Google) 2011-03-04 07:20:15 PST
(In reply to comment #3)
> Some notes on the work-in-progress patch:
> 
> .) It also includes type=search (hence the renaming of this bug entry), but search should really have it's own RenderTextControl sub-type.
> 
> .) The shadow DOM objects are so far largely unchanged from the original code. Layouting is still handled by the RenderTExtControlSingleLine. If possible, I would keep this arrangement for the initial version, and move the layouting logic to the shadow Elements in subsequent bugs/patches.
> 
> .) There is some inefficiency that the shadow DOM for type=text is created by default, then destroyed and overwritten once the type attribute is read. Not sure how we can fix this (or whether it really matters).

I am concerned that the patch is so large. Can it be broken into smaller pieces?
Comment 6 Dimitri Glazkov (Google) 2011-03-04 10:57:54 PST
Comment on attachment 84705 [details]
work-in-progress

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

Overall, I understand now why this patch is so large. I think it's fine to go with this, provided that we have green bubbles on EWS. But I hope this is not the end of it. Ideally, render objects shouldn't dip into shadow subtree to do layout calculations. Instead the render objects inside the shadow tree should provide this information to parent -- that's the way normal layout works.

> Source/WebCore/html/InputType.cpp:369
> +    shadowParent->parserAddChild(shadowChild);

What notification messages?

shadowParent->appendChild(shadowChild)

> Source/WebCore/html/SearchInputType.h:65
> +    RefPtr<TextControlInnerElement> m_innerBlock;
> +    RefPtr<SearchFieldResultsButtonElement> m_resultsButton;
> +    RefPtr<SearchFieldCancelButtonElement> m_cancelButton;

Do these need to be RefPtr?

> Source/WebCore/rendering/RenderTextControl.cpp:490
>  void RenderTextControl::forwardEvent(Event* event)
>  {
>      if (event->type() == eventNames().blurEvent || event->type() == eventNames().focusEvent)

Event forwarding should not be necessary now that shadowRoot is set properly.

> Source/WebCore/rendering/RenderTextControlMultiLine.cpp:110
> +/* $$$ remove

Probably didn't mean to leave this in.
Comment 7 Dimitri Glazkov (Google) 2011-03-04 11:00:01 PST
Ah, I see now that the patch is titled "work-in-progress". I was confused by the "r?" flag.
Comment 8 Kent Tamura 2011-04-03 22:09:37 PDT
I reconsidered what's the best way to transit to new shadow DOM in RenderTextSingleControl, and I think Roland's patch is good for the first step and it's not possible to reduce the patch size.
Comment 9 Kent Tamura 2011-04-05 02:44:50 PDT
I'll take over the patch.
Comment 10 Kent Tamura 2011-04-05 19:15:43 PDT
I found we need to make the patch very larger.
RenderTextControl* assume shadow nodes already have their renderers in the first call of RenderObject::setStyle(), but they don't.  We need massive change for shadow style creation.
Comment 11 Kent Tamura 2011-04-12 08:52:38 PDT
Created attachment 89210 [details]
work-in-progress 2

Unfortunately, we need to update render tree dumps with selections...
Comment 12 Dimitri Glazkov (Google) 2011-04-12 10:58:48 PDT
Comment on attachment 89210 [details]
work-in-progress 2

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

Thanks for your hard work, Kent-san.

I looked over a patch. Here are some preliminary comments:

> LayoutTests/fast/forms/focus-selection-input-expected.txt:7
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of DIV > #document-fragment to 0 of DIV > #document-fragment affinity:NSSelectionAffinityDownstream stillSelecting:FALSE

It seems like we'll be better of skipping ShadowRoot in the dumping code.

> Source/WebCore/html/HTMLInputElement.cpp:80
> +    m_inputType->createShadowSubtree();

This should be in ::create, not the constructor.

> Source/WebCore/html/HTMLTextAreaElement.cpp:88
> +    createShadowSubtreeIfNeeded();

Creating DOM elements on attach seems dangerous. Can we put it elsewhere, like into a ::create method?

> Source/WebCore/html/InputType.cpp:370
> +    shadowParent->parserAddChild(shadowChild);

Yes. We should use appendChild here. What notifications are being fired? Perhaps we need to find a way to stop them? I am pretty sure mutation events aren't escaping the shadow DOM boundary.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:237
> +    return m_isInner ? parentRenderer->createInnerSpinButtonStyle() : parentRenderer->createOuterSpinButtonStyle();

We should just switch these to use shadowPseudoId. Maybe that's step one.

> Source/WebCore/rendering/RenderTextControl.cpp:95
> +        // FIXME: Are they needed?

It seems that these should be in layout code, not setStyle code. But a FIXME (and a bug?) is ok for now.

> Source/WebCore/rendering/RenderTextControl.cpp:111
> +    innerText->renderer()->setStyle(textStyle);
> +    for (Node* n = innerText->firstChild(); n; n = n->traverseNextNode(innerText)) {
> +        if (n->renderer())
> +            n->renderer()->setStyle(textStyle);

This is odd. We shouldn't need this code. Standard style recalc should do this correctly.
Comment 13 Kent Tamura 2011-04-12 16:51:49 PDT
Comment on attachment 89210 [details]
work-in-progress 2

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

Thank you for the review.

>> LayoutTests/fast/forms/focus-selection-input-expected.txt:7
>> +EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of DIV > #document-fragment to 0 of DIV > #document-fragment affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> 
> It seems like we'll be better of skipping ShadowRoot in the dumping code.

I don't think skipping ShadowRoot is a good idea.  It would hide actual tree structures.
It might be better to show "[shadow-root]" instead of "#document-fragment".

>> Source/WebCore/html/HTMLInputElement.cpp:80
>> +    m_inputType->createShadowSubtree();
> 
> This should be in ::create, not the constructor.

Why should it be?

>> Source/WebCore/html/InputType.cpp:370
>> +    shadowParent->parserAddChild(shadowChild);
> 
> Yes. We should use appendChild here. What notifications are being fired? Perhaps we need to find a way to stop them? I am pretty sure mutation events aren't escaping the shadow DOM boundary.

I don't know what notifications.  Roland?
Comment 14 Dimitri Glazkov (Google) 2011-04-12 19:05:04 PDT
Comment on attachment 89210 [details]
work-in-progress 2

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

>>> LayoutTests/fast/forms/focus-selection-input-expected.txt:7
>>> +EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of DIV > #document-fragment to 0 of DIV > #document-fragment affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
>> 
>> It seems like we'll be better of skipping ShadowRoot in the dumping code.
> 
> I don't think skipping ShadowRoot is a good idea.  It would hide actual tree structures.
> It might be better to show "[shadow-root]" instead of "#document-fragment".

Good point.

>>> Source/WebCore/html/HTMLInputElement.cpp:80
>>> +    m_inputType->createShadowSubtree();
>> 
>> This should be in ::create, not the constructor.
> 
> Why should it be?

Inside of a constructor, the element is in a vulnerable state, which may cause it to ref-count itself to 0. Darin Adler recently pointed this out in one of his reviews.
Comment 15 Kent Tamura 2011-04-13 07:21:50 PDT
Comment on attachment 89210 [details]
work-in-progress 2

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

>>>> Source/WebCore/html/HTMLInputElement.cpp:80
>>>> +    m_inputType->createShadowSubtree();
>>> 
>>> This should be in ::create, not the constructor.
>> 
>> Why should it be?
> 
> Inside of a constructor, the element is in a vulnerable state, which may cause it to ref-count itself to 0. Darin Adler recently pointed this out in one of his reviews.

I understand.  Thank you.
Comment 16 Roland Steiner 2011-04-13 11:23:21 PDT
(In reply to comment #13)
> (From update of attachment 89210 [details])
> >> Source/WebCore/html/InputType.cpp:370
> >> +    shadowParent->parserAddChild(shadowChild);
> > 
> > Yes. We should use appendChild here. What notifications are being fired? Perhaps we need to find a way to stop them? I am pretty sure mutation events aren't escaping the shadow DOM boundary.
> 
> I don't know what notifications.  Roland?

appendChild calls:

    InspectorInstrumentation::willInsertDOMNode
    childrenChanged
    notifyChildInserted
    dispatchSubtreeModifiedEvent

TBH, I'm not sure if these will cause problems, but it seemed to me that this function is meant to implement the JS appendChild function. I therefore followed the lead of the existing code and used parserAppendChild instead, which only calls

    InspectorInstrumentation::willInsertDOMNode
    childrenChanged
Comment 17 Dimitri Glazkov (Google) 2011-04-13 11:26:52 PDT
(In reply to comment #16)
> (In reply to comment #13)
> > (From update of attachment 89210 [details] [details])
> > >> Source/WebCore/html/InputType.cpp:370
> > >> +    shadowParent->parserAddChild(shadowChild);
> > > 
> > > Yes. We should use appendChild here. What notifications are being fired? Perhaps we need to find a way to stop them? I am pretty sure mutation events aren't escaping the shadow DOM boundary.
> > 
> > I don't know what notifications.  Roland?
> 
> appendChild calls:
> 
>     InspectorInstrumentation::willInsertDOMNode
>     childrenChanged
>     notifyChildInserted

This is actually useful, since it calls insertedIntoDocument.

>     dispatchSubtreeModifiedEvent

Mutation events don't escape the shadow subtree, so we're ok here.

> 
> TBH, I'm not sure if these will cause problems, but it seemed to me that this function is meant to implement the JS appendChild function. I therefore followed the lead of the existing code and used parserAppendChild instead, which only calls
> 
>     InspectorInstrumentation::willInsertDOMNode
>     childrenChanged

I think we're ok with using appendChild.
Comment 18 Roland Steiner 2011-04-13 11:46:42 PDT
(In reply to comment #17)
> >     dispatchSubtreeModifiedEvent
> 
> Mutation events don't escape the shadow subtree, so we're ok here.

Won't this cause problems with JS on the shadow tree, down the line?
Comment 19 Dimitri Glazkov (Google) 2011-04-13 12:35:56 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > >     dispatchSubtreeModifiedEvent
> > 
> > Mutation events don't escape the shadow subtree, so we're ok here.
> 
> Won't this cause problems with JS on the shadow tree, down the line?

Nope. If it does, this is would be a spec change.
Comment 20 Kent Tamura 2011-04-13 22:48:42 PDT
Comment on attachment 89210 [details]
work-in-progress 2

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

> LayoutTests/platform/mac/fast/forms/textarea-scrollbar-expected.txt:35
>  layer at (10,28) size 161x84 clip at (11,29) size 144x82 scrollHeight 121
>    RenderTextControl {TEXTAREA} at (2,20) size 161x84 [bgcolor=#FFFFFF] [border: (1px solid #000000)]
>      RenderBlock {DIV} at (3,3) size 140x117
> -      RenderText {#text} at (0,0) size 7x52
> -        text run at (0,0) width 7: "1"
> -        text run at (6,0) width 1: " "
> -        text run at (0,13) width 7: "2"
> -        text run at (6,13) width 1: " "
> -        text run at (0,26) width 7: "3"
> -        text run at (6,26) width 1: " "
> -        text run at (0,39) width 7: "4"
> -        text run at (6,39) width 1: " "
> -      RenderText {#text} at (0,52) size 7x13
> -        text run at (0,52) width 7: "5"
> -      RenderText {#text} at (6,52) size 1x13
> -        text run at (6,52) width 1: " "
> -      RenderText {#text} at (0,65) size 7x13
> -        text run at (0,65) width 7: "6"
> -      RenderText {#text} at (6,65) size 1x13
> -        text run at (6,65) width 1: " "
> -      RenderText {#text} at (0,78) size 7x13
> -        text run at (0,78) width 7: "7"
> -      RenderText {#text} at (6,78) size 1x13
> -        text run at (6,78) width 1: " "
> -      RenderText {#text} at (0,91) size 7x13
> -        text run at (0,91) width 7: "8"
> -      RenderText {#text} at (6,91) size 1x13
> -        text run at (6,91) width 1: " "
> -      RenderText {#text} at (0,104) size 0x13
> -        text run at (0,104) width 0: " "
> -caret: position 0 of child 9 {#text} of child 0 {DIV} of child 3 {TEXTAREA} of body
> +      RenderBlock (anonymous) at (0,0) size 140x65
> +        RenderText {#text} at (0,0) size 7x52
> +          text run at (0,0) width 7: "1"
> +          text run at (6,0) width 1: " "
> +          text run at (0,13) width 7: "2"
> +          text run at (6,13) width 1: " "
> +          text run at (0,26) width 7: "3"
> +          text run at (6,26) width 1: " "
> +          text run at (0,39) width 7: "4"
> +          text run at (6,39) width 1: " "
> +        RenderText {#text} at (0,52) size 7x13
> +          text run at (0,52) width 7: "5"
> +      RenderBlock {DIV} at (0,65) size 140x13
> +        RenderText {#text} at (0,0) size 7x13
> +          text run at (0,0) width 7: "6"
> +      RenderBlock {DIV} at (0,78) size 140x13
> +        RenderText {#text} at (0,0) size 7x13
> +          text run at (0,0) width 7: "7"
> +      RenderBlock {DIV} at (0,91) size 140x13
> +        RenderText {#text} at (0,0) size 7x13
> +          text run at (0,0) width 7: "8"
> +      RenderBlock {DIV} at (0,104) size 140x13
> +        RenderBR {BR} at (0,0) size 0x13

It seems we have to do something for InsertParagraphSeparatorCommand.
Comment 21 Kent Tamura 2011-04-15 03:33:44 PDT
I'm struggling to fix editing/selection issues.
ShadowRoot class broke an assumption that an editable tree doesn't contain DocumentFragment nodes.
Comment 22 Kent Tamura 2011-04-15 04:37:47 PDT
Created attachment 89769 [details]
work-in-progress 3
Comment 23 Kent Tamura 2011-04-15 04:47:27 PDT
(In reply to comment #22)
> Created an attachment (id=89769) [details]
> work-in-progress 3

Niwa-san,
Would you check editing code changes and editing test result changes in the patch please?

We're going to change shadow tree structure of <input> and <textarea> so that dom/ShadowRoot, which is a subclass of DocumentFragment, is inserted as a shadow root between the host element and a shadow element which was the shadow root element.

So, many test results changes which simply add "> #shadow-root" or " of {#shadow-root}" are expected. However I don't understand the following tests result changes:
 LayoutTests/editing/selection/select-all-textarea-expected.txt
 LayoutTests/fast/forms/focus-selection-input-expected.txt
 LayoutTests/fast/forms/focus-selection-textarea-expected.txt
 LayoutTests/platform/mac/editing/pasteboard/pasting-tabs-expected.txt
Do we need to correct them?
Comment 24 Kent Tamura 2011-04-15 04:53:03 PDT
*** Bug 52456 has been marked as a duplicate of this bug. ***
Comment 25 WebKit Review Bot 2011-04-15 04:54:35 PDT
Attachment 89769 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8457111
Comment 26 Kent Tamura 2011-04-15 04:54:43 PDT
*** Bug 54174 has been marked as a duplicate of this bug. ***
Comment 27 Kent Tamura 2011-04-15 04:55:05 PDT
*** Bug 54175 has been marked as a duplicate of this bug. ***
Comment 28 Kent Tamura 2011-04-15 04:55:28 PDT
*** Bug 54177 has been marked as a duplicate of this bug. ***
Comment 29 Kent Tamura 2011-04-15 04:55:46 PDT
*** Bug 54178 has been marked as a duplicate of this bug. ***
Comment 30 Roland Steiner 2011-04-15 10:24:33 PDT
(In reply to comment #21)
> I'm struggling to fix editing/selection issues.
> ShadowRoot class broke an assumption that an editable tree doesn't contain DocumentFragment nodes.

(In reply to comment #23)
> We're going to change shadow tree structure of <input> and <textarea> so that dom/ShadowRoot, which is a subclass of DocumentFragment, is inserted as a shadow root between the host element and a shadow element which was the shadow root element.

Note that I'm currently working on a patch for bug 52963 that will most likely change the base class of ShadowRoot from DocumentFragment to TreeScope.
Comment 31 WebKit Review Bot 2011-04-15 11:24:48 PDT
Attachment 89769 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8452199
Comment 32 Ryosuke Niwa 2011-04-15 15:28:25 PDT
(In reply to comment #23)
> So, many test results changes which simply add "> #shadow-root" or " of {#shadow-root}" are expected. However I don't understand the following tests result changes:
>  LayoutTests/editing/selection/select-all-textarea-expected.txt
>  LayoutTests/fast/forms/focus-selection-input-expected.txt
>  LayoutTests/fast/forms/focus-selection-textarea-expected.txt
>  LayoutTests/platform/mac/editing/pasteboard/pasting-tabs-expected.txt

I wouldn't get too nervous about editing callback dumps especially if their visible positions are the same.  Most of these changes seem to be [#text, 0] > [div, 0] and I suspect that #text is the first child of div.  If that's the case, then we're probably just taking a different code path and generating different positions so I wouldn't worry about it.
Comment 33 Ryosuke Niwa 2011-04-15 15:38:43 PDT
(In reply to comment #30)
> Note that I'm currently working on a patch for bug 52963 that will most likely change the base class of ShadowRoot from DocumentFragment to TreeScope.

It seems like we must land that patch first.  There are so many places in editing where we assume that DocumentFragment isn't a part of any document that it's probably not worth the time to fix them.
Comment 34 Roland Steiner 2011-04-15 17:38:10 PDT
(In reply to comment #33)
> It seems like we must land that patch first.  There are so many places in editing where we assume that DocumentFragment isn't a part of any document that it's probably not worth the time to fix them.

That patch landed in Wk r84050.

Note that the ShadowRoot's nodeType will still return DOCUMENT_FRAGMENT_NODE for now (see also bug 58704).
Comment 35 Kent Tamura 2011-04-17 22:02:00 PDT
Niwa-san, Roland, thank you for the comments.
ok, I'll wait until Bug 58704 is resolved.
Comment 36 Kent Tamura 2011-04-22 13:02:11 PDT
Created attachment 90744 [details]
Patch
Comment 37 Dimitri Glazkov (Google) 2011-04-22 13:34:41 PDT
Comment on attachment 90744 [details]
Patch 

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

I would have liked this to be a sub-bug of bug 54179, because it doesn't really finish the work, but I guess we can track those as separate bugs that block bug 52962.

> Source/WebCore/bindings/objc/DOM.mm:331
> +            // FIXME: Should provide DOMShadowRoot?

Please file a bug to track this and mention in the FIXME comment.

> Source/WebCore/dom/Element.cpp:1914
> +        if (parent && parent->isElementNode())

Maybe rewrite as:

if (!parent)
    element = 0
else if ...

> Source/WebCore/rendering/RenderTextControlSingleLine.h:40
> +    // FIXME: Move create*Style() to their classes.

Can you file a bug on this and add to the comment?
Comment 38 Ryosuke Niwa 2011-04-22 13:41:31 PDT
I don't think we should r+ this patch.  It clearly contains at least one bug as it doesn't update SelectionController::nodeWillBeRemoved.  Clearly, we DO need to update the selection when nodes inside a shadow DOM is removed.
Comment 39 Ryosuke Niwa 2011-04-22 13:43:16 PDT
We need to update SelectionController::textWillBeReplaced as well.
Comment 40 Ryosuke Niwa 2011-04-22 13:50:19 PDT
Comment on attachment 90744 [details]
Patch 

Okay, Dimitri clarified on IRC that the node type is no longer DOCUMENT_FRAGMENT.  So my previous accusations are false. But I'm still concerned about the change in InsertParagraphSeparatorCommand::doApply.  Why do we need to call startBlock->parentNode()->isShadowBoundary() in L171?
Comment 41 Dimitri Glazkov (Google) 2011-04-22 14:39:51 PDT
Comment on attachment 90744 [details]
Patch 

Removing r+ pending lively discussion on #webkit.
Comment 42 Dimitri Glazkov (Google) 2011-05-18 14:35:37 PDT
Quick update:

We finished traversing the editing code and got this list of action
items (see https://docs.google.com/a/chromium.org/document/d/1Q65QPyN6EsfusVYi3aaYUTerFJAGjVnWTiB8NJId8Vk/edit?hl=en&authkey=CNOVk10&pli=1#
for details):

Assert in Position::parentAnchoredEquivalent that
containerNode()->isShadowBoundary() and PositionIsBeforeAnchor aren’t
possible at the moment.

Assert in Position::containerNode and
Position::computeOffsetInContainerNode that the container node is not
a shadow root

Assert in ReplacementFragment::removeNode,
ReplacementFragement::insertNodeBefore that node->parentNode is not a
shadow root

Check for !boundary->parentNode()->isShadowBoundary() in
Position::parentEditingBoundary:369.

CompositeEditCommand::insertNodeAfter should have it’s assert updated
to check both that the parent is not null and that it is not the
shadow root.

CompositeEditCommand::removeNode should ASSERT that the node’s parent
is not the shadow root.

visiblePositionBeforeNode, visiblePositionAfterNode: Add an assertion
that the parent node is not a shadow root

Position::previous, Position::next: Add a check on parent. If parent
is also a shadow root, return itself -- don’t go to parent!

Position::atStartOfTree, Position::atEndOfTree -- Return true if
parentNode() is shadow root.

We mulled it over and thought it would be best if these are just added
to the Kent-san's patch, not landed separately. Kent-san, can you put
them in?
Comment 43 Kent Tamura 2011-05-18 15:16:45 PDT
(In reply to comment #42)
> We mulled it over and thought it would be best if these are just added
> to the Kent-san's patch, not landed separately. Kent-san, can you put
> them in?

Yes, I'll do.  Do we have test cases for them?
Comment 44 Dimitri Glazkov (Google) 2011-05-18 15:21:20 PDT
(In reply to comment #43)
> (In reply to comment #42)
> > We mulled it over and thought it would be best if these are just added
> > to the Kent-san's patch, not landed separately. Kent-san, can you put
> > them in?
> 
> Yes, I'll do.  Do we have test cases for them?

We don't have the test cases. It is quite possible that we can't actually trigger these (especially ones in Position). We just found these by code inspection and verified that if supplied an argument, we will return valid result. But how this argument would be supplied is an unknown.
Comment 45 Ryosuke Niwa 2011-05-19 10:00:13 PDT
According to Dimitri, making this transition should help us fixing https://bugs.webkit.org/show_bug.cgi?id=27683 as well.

tkent, can you verify that your patch also fixes the bug 27683?
Comment 46 Ryosuke Niwa 2011-05-19 10:02:44 PDT
Ditto: https://bugs.webkit.org/show_bug.cgi?id=45889
Comment 47 Ryosuke Niwa 2011-05-19 11:25:05 PDT
May also fix https://bugs.webkit.org/show_bug.cgi?id=7651.
Comment 48 Ryosuke Niwa 2011-05-19 11:33:16 PDT
ap: rniwa: you could check old bugs with NativeTextArea and NativeTextField keywords - some may be fixed by the refactoring, but more importantly, some may hint at what areas the new refactoring is likely to further break
ap: tkent: perhaps you would be interested in looking at those, too ^^^
Comment 49 Kent Tamura 2011-05-19 18:55:27 PDT
(In reply to comment #45)
> According to Dimitri, making this transition should help us fixing https://bugs.webkit.org/show_bug.cgi?id=27683 as well.
> 
> tkent, can you verify that your patch also fixes the bug 27683?

I thought this patch would fix it, but actually no.  I don't know the reason.

(In reply to comment #46)
> Ditto: https://bugs.webkit.org/show_bug.cgi?id=45889

Yes,  It will be fixed by this patch.

(In reply to comment #47)
> May also fix https://bugs.webkit.org/show_bug.cgi?id=7651.

I couldn't reproduce the problem with Chrome dev.
Comment 50 Kent Tamura 2011-05-19 18:57:06 PDT
(In reply to comment #42)
> We finished traversing the editing code and got this list of action
> items (see https://docs.google.com/a/chromium.org/document/d/1Q65QPyN6EsfusVYi3aaYUTerFJAGjVnWTiB8NJId8Vk/edit?hl=en&authkey=CNOVk10&pli=1#
> for details):
> 
> Assert in Position::parentAnchoredEquivalent that
> containerNode()->isShadowBoundary() and PositionIsBeforeAnchor aren’t
> possible at the moment.
> ...

I fixed them, and confirmed they didn't make regressions for existing tests.
Comment 51 Ryosuke Niwa 2011-05-19 18:59:50 PDT
Great!  Could you upload a new patch with all assertions & new fixes?
Comment 52 Kent Tamura 2011-05-19 19:02:35 PDT
(In reply to comment #48)
> ap: rniwa: you could check old bugs with NativeTextArea and NativeTextField keywords - some may be fixed by the refactoring, but more importantly, some may hint at what areas the new refactoring is likely to further break
> ap: tkent: perhaps you would be interested in looking at those, too ^^^

I think WebKit has no support for NativeTextArea and NativeTextField anymore.
Comment 53 Kent Tamura 2011-05-19 21:06:30 PDT
Created attachment 94166 [details]
Patch 2
Comment 54 WebKit Review Bot 2011-05-19 21:08:37 PDT
Attachment 94166 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

LayoutTests/platform/chromium/test_expectations.txt:4105:  More specific entry on line 4105 overrides line 3535 fast/speech/input-appearance-searchandspeech.html  [test/expectations] [5]
Total errors found: 1 in 205 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 55 Kent Tamura 2011-05-19 21:13:15 PDT
Created attachment 94167 [details]
Patch 3

Fix test_expectations.txt
Comment 56 WebKit Review Bot 2011-05-19 22:09:14 PDT
Comment on attachment 94167 [details]
Patch 3

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

New failing tests:
fast/forms/input-readonly-autoscroll.html
Comment 57 WebKit Review Bot 2011-05-19 22:09:21 PDT
Created attachment 94173 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 58 Kent Tamura 2011-05-19 22:17:23 PDT
Created attachment 94175 [details]
Patch 4

Update input-readonly-autoscroll.html expectation for Chromium-linux
Comment 59 Alexey Proskuryakov 2011-05-19 22:38:52 PDT
> I think WebKit has no support for NativeTextArea and NativeTextField anymore.

In fact, these were the names for the current implementation. Native meant "native to WebKit", not taken from the underlying platform, like NSTextField have been.

Anyway, that's not my point. What I'm saying is that this set of bugs is something that's likely to be related to the changes being made now, providing some deep context.
Comment 60 Alexey Proskuryakov 2011-05-19 22:41:12 PDT
The reason why I suggested to look at these bugs is that I've been asked whether I remembered any good bugs to look at in relation to this work.
Comment 61 Kent Tamura 2011-05-19 23:23:14 PDT
(In reply to comment #59)
> In fact, these were the names for the current implementation. Native meant "native to WebKit", not taken from the underlying platform, like NSTextField have been.
> 
> Anyway, that's not my point. What I'm saying is that this set of bugs is something that's likely to be related to the changes being made now, providing some deep context.

Oh, I understand.  Thank you.
I'll take a look at such bugs.
Comment 62 Dimitri Glazkov (Google) 2011-05-20 09:22:10 PDT
Comment on attachment 94175 [details]
Patch 4

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

Good lord, that was a long patch to review. I think I am ready for this to land.

> Source/WebCore/dom/Node.cpp:1743
> +Node* Node::nonBoundaryShadowTreeRootNode()

We should have bugs filed to remove these once all elements are converted to new shadow DOM.
Comment 63 Ryosuke Niwa 2011-05-20 10:29:08 PDT
Comment on attachment 94175 [details]
Patch 4

This patch is missing an assertion in computeOffsetInContainerNode. Also, is there a reason you added nonShadowBoundaryParentNode instead of asserting that we never hit it in various places?
Comment 64 Kent Tamura 2011-05-20 17:00:13 PDT
(In reply to comment #63)
> (From update of attachment 94175 [details])
> This patch is missing an assertion in computeOffsetInContainerNode. 

Do you mean we need ASSERT(!m_anchorNode->isShadowBoundary()) ?
If so, probably we had better add assertions in Position constructors.

> Also, is there a reason you added nonShadowBoundaryParentNode instead of asserting that we never hit it in various places? 

Even if we add assertions in various places, we still need to use nonShadowBoundaryParenNode() to avoid assertion failures.
Comment 65 Ryosuke Niwa 2011-05-20 17:02:03 PDT
(In reply to comment #64)
> (In reply to comment #63)
> > Also, is there a reason you added nonShadowBoundaryParentNode instead of asserting that we never hit it in various places? 
> 
> Even if we add assertions in various places, we still need to use nonShadowBoundaryParenNode() to avoid assertion failures.

So are you saying that we hit those assertions now?  We'd have to investigate each case if we do.
Comment 66 Kent Tamura 2011-05-20 17:24:06 PDT
(In reply to comment #65)
> > Even if we add assertions in various places, we still need to use nonShadowBoundaryParenNode() to avoid assertion failures.
> 
> So are you saying that we hit those assertions now?  We'd have to investigate each case if we do.

It's unclear to me where we should add assertions.  I haven't tried to add assertions.
Comment 67 Kent Tamura 2011-05-20 22:36:21 PDT
Created attachment 94321 [details]
Patch for landing

* Add assertions to all of Position constructor
* Remove LayoutTests/platform/qt/editing/pasteboard/copy-in-password-file-expected.txt change
Comment 68 WebKit Commit Bot 2011-05-21 03:48:14 PDT
Comment on attachment 94321 [details]
Patch for landing

Clearing flags on attachment: 94321

Committed r87014: <http://trac.webkit.org/changeset/87014>
Comment 69 WebKit Commit Bot 2011-05-21 03:48:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 70 Csaba Osztrogonác 2011-05-23 01:17:33 PDT
(In reply to comment #69)
> All reviewed patches have been landed.  Closing bug.

It caused assertions on debug bots:
- http://build.webkit.org/results/Leopard%20Intel%20Debug%20%28Tests%29/r87014%20%2830521%29/http/tests/inspector/resource-parameters-stderr.txt

- backtrace from Qt debug bot: <attached>
Comment 71 Csaba Osztrogonác 2011-05-23 01:18:09 PDT
Created attachment 94379 [details]
backtrace from Qt debug bot
Comment 72 Csaba Osztrogonác 2011-05-23 02:02:53 PDT
Reopen, because it was rolled out by http://trac.webkit.org/changeset/87062
Comment 73 Kent Tamura 2011-05-23 02:18:29 PDT
(In reply to comment #70)
> (In reply to comment #69)
> > All reviewed patches have been landed.  Closing bug.
> 
> It caused assertions on debug bots:
> - http://build.webkit.org/results/Leopard%20Intel%20Debug%20%28Tests%29/r87014%20%2830521%29/http/tests/inspector/resource-parameters-stderr.txt
> 
> - backtrace from Qt debug bot: <attached>

Is that all?
 Unfortunately it didn't happen on SnowLeopard Debug, but I found the root cause.
Comment 74 Kent Tamura 2011-05-23 03:13:48 PDT
Re-landed as http://trac.webkit.org/changeset/87067 with a small fix of Element::removeShadowRoot().

http://trac.webkit.org/changeset/87067/trunk/Source/WebCore/dom/Element.cpp#file0