Bug 54179 – Change text-based <input> types to use the new shadow DOM model

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Roland Steiner]

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

[Dominic Cooney]

I believe this bug should also cover textarea and input[type=color].

[Roland Steiner]

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?)

[Roland Steiner]

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).

[Kent Tamura]

(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()?

[Dimitri Glazkov (Google)]

(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?

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

Ah, I see now that the patch is titled "work-in-progress". I was confused by the "r?" flag.

[Kent Tamura]

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.

[Kent Tamura]

I'll take over the patch.

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 89210 [details]
work-in-progress 2

Unfortunately, we need to update render tree dumps with selections...

[Dimitri Glazkov (Google)]

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.

[Kent Tamura]

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?

[Dimitri Glazkov (Google)]

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.

[Kent Tamura]

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.

[Roland Steiner]

(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

[Dimitri Glazkov (Google)]

(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.

[Roland Steiner]

(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?

[Dimitri Glazkov (Google)]

(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.

[Kent Tamura]

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.

[Kent Tamura]

I'm struggling to fix editing/selection issues.
ShadowRoot class broke an assumption that an editable tree doesn't contain DocumentFragment nodes.

[Kent Tamura]

Created attachment 89769 [details]
work-in-progress 3

[Kent Tamura]

(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?

[Kent Tamura]

*** Bug 52456 has been marked as a duplicate of this bug. ***

[WebKit Review Bot]

Attachment 89769 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8457111

[Kent Tamura]

*** Bug 54174 has been marked as a duplicate of this bug. ***

[Kent Tamura]

*** Bug 54175 has been marked as a duplicate of this bug. ***

[Kent Tamura]

*** Bug 54177 has been marked as a duplicate of this bug. ***

[Kent Tamura]

*** Bug 54178 has been marked as a duplicate of this bug. ***

[Roland Steiner]

(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.

[WebKit Review Bot]

Attachment 89769 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8452199

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

(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.

[Roland Steiner]

(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).

[Kent Tamura]

Niwa-san, Roland, thank you for the comments.
ok, I'll wait until Bug 58704 is resolved.

[Kent Tamura]

Created attachment 90744 [details]
Patch

[Dimitri Glazkov (Google)]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

We need to update SelectionController::textWillBeReplaced as well.

[Ryosuke Niwa]

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?

[Dimitri Glazkov (Google)]

Comment on attachment 90744 [details]
Patch 

Removing r+ pending lively discussion on #webkit.

[Dimitri Glazkov (Google)]

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?

[Kent Tamura]

(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?

[Dimitri Glazkov (Google)]

(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.

[Ryosuke Niwa]

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?

[Ryosuke Niwa]

Ditto: https://bugs.webkit.org/show_bug.cgi?id=45889

[Ryosuke Niwa]

May also fix https://bugs.webkit.org/show_bug.cgi?id=7651.

[Ryosuke Niwa]

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 ^^^

[Kent Tamura]

(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.

[Kent Tamura]

(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.

[Ryosuke Niwa]

Great!  Could you upload a new patch with all assertions & new fixes?

[Kent Tamura]

(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.

[Kent Tamura]

Created attachment 94166 [details]
Patch 2

[WebKit Review Bot]

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.

[Kent Tamura]

Created attachment 94167 [details]
Patch 3

Fix test_expectations.txt

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kent Tamura]

Created attachment 94175 [details]
Patch 4

Update input-readonly-autoscroll.html expectation for Chromium-linux

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

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.

[Kent Tamura]

(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.

[Dimitri Glazkov (Google)]

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.

[Ryosuke Niwa]

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?

[Kent Tamura]

(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.

[Ryosuke Niwa]

(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.

[Kent Tamura]

(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.

[Kent Tamura]

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

[WebKit Commit Bot]

Comment on attachment 94321 [details]
Patch for landing

Clearing flags on attachment: 94321

Committed r87014: <http://trac.webkit.org/changeset/87014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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>

[Csaba Osztrogonác]

Created attachment 94379 [details]
backtrace from Qt debug bot

[Csaba Osztrogonác]

Reopen, because it was rolled out by http://trac.webkit.org/changeset/87062

[Kent Tamura]

(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.

[Kent Tamura]

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