Summary: | Change text-based <input> types to use the new shadow DOM model | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Roland Steiner <rolandsteiner> | ||||||||||||||||||||||
Component: | DOM | Assignee: | Kent Tamura <tkent> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, dbates, dglazkov, dominicc, morrita, ossy, peter, rniwa, satish, tkent, webkit.review.bot | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||
Bug Depends on: | 52788, 52917, 54853, 55355, 58432, 58704, 61265, 61266 | ||||||||||||||||||||||||
Bug Blocks: | 7651, 27683, 45889, 60818, 61166, 61909 | ||||||||||||||||||||||||
Attachments: |
|
Description
Roland Steiner
2011-02-10 00:31:26 PST
I believe this bug should also cover textarea and input[type=color]. 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?)
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). (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()? (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 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. Ah, I see now that the patch is titled "work-in-progress". I was confused by the "r?" flag. 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. I'll take over the patch. 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. Created attachment 89210 [details]
work-in-progress 2
Unfortunately, we need to update render tree dumps with selections...
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 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 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 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. (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 (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. (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? (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 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. I'm struggling to fix editing/selection issues. ShadowRoot class broke an assumption that an editable tree doesn't contain DocumentFragment nodes. Created attachment 89769 [details]
work-in-progress 3
(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? *** Bug 52456 has been marked as a duplicate of this bug. *** Attachment 89769 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8457111 *** Bug 54174 has been marked as a duplicate of this bug. *** *** Bug 54175 has been marked as a duplicate of this bug. *** *** Bug 54177 has been marked as a duplicate of this bug. *** *** Bug 54178 has been marked as a duplicate of this bug. *** (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. Attachment 89769 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8452199 (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. (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. (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). Niwa-san, Roland, thank you for the comments. ok, I'll wait until Bug 58704 is resolved. Created attachment 90744 [details]
Patch
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? 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. We need to update SelectionController::textWillBeReplaced as well. 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 on attachment 90744 [details]
Patch
Removing r+ pending lively discussion on #webkit.
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? (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? (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. 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? May also fix https://bugs.webkit.org/show_bug.cgi?id=7651. 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 ^^^ (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. (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. Great! Could you upload a new patch with all assertions & new fixes? (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. Created attachment 94166 [details]
Patch 2
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.
Created attachment 94167 [details]
Patch 3
Fix test_expectations.txt
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 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
Created attachment 94175 [details]
Patch 4
Update input-readonly-autoscroll.html expectation for Chromium-linux
> 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.
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. (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 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 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?
(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. (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. (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. 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 on attachment 94321 [details] Patch for landing Clearing flags on attachment: 94321 Committed r87014: <http://trac.webkit.org/changeset/87014> All reviewed patches have been landed. Closing bug. (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> Created attachment 94379 [details]
backtrace from Qt debug bot
Reopen, because it was rolled out by http://trac.webkit.org/changeset/87062 (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. 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 |