| Summary: | Stop instantiating legacy editing Positions in VisiblePosition | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Levi Weintraub <leviw> | ||||||||||||||||||
| Component: | HTML Editing | Assignee: | Levi Weintraub <leviw> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | abarth, darin, eric, gustavo.noronha, gustavo, leviw, ojan, rniwa, tony, webkit.review.bot, xan.lopez | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Bug Depends on: | 54043 | ||||||||||||||||||||
| Bug Blocks: | 52099 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Levi Weintraub
2011-01-21 14:05:30 PST
VisiblePosition also has a constructor that takes a [Node, Offset] pair that should go away. There are about 50 call sites that use it. Created attachment 80139 [details]
Rev1
Except for in the constructor, legacy positions are done away with in VisiblePosition.cpp.
Also, all call sites in WebCore are switched to use the VisiblePosition constructor that takes a Position instead of a node, offset. I put FIXMEs on the call sites I didn't change from creating legacy editing positions.
Created attachment 81280 [details]
Patch
(In reply to comment #3) > Created an attachment (id=81280) [details] > Patch This is resulting in slightly different layout test results due to the changes made in visible_units.cpp. I'm looking into why that is, but think this is otherwise correct. Attachment 81280 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7701131 I'll fix the GTK port (missed their usage of the legacy constructor while grepping). I've also gotten down to the bottom of my failing layout tests. Position::upstream and downstream are *broken* for new position types. Consider the following: <div>foo<img></div> A position at [img, PositionIsAfterAnchor]. Upstream for this returns ["foo", 3] because it only looks at the offset, which is 0 in this case. Created attachment 81313 [details]
Patch
(In reply to comment #7) > Created an attachment (id=81313) [details] > Patch This passes all layout tests and I believe it'll build on all platforms :) Attachment 81313 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7702165 Created attachment 81315 [details]
Patch
(In reply to comment #9) > Attachment 81313 [details] did not build on gtk: > Build output: http://queues.webkit.org/results/7702165 Sheesh, missed a file, sorry about that! Trying again :) Comment on attachment 81315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81315&action=review Thanks for tackling this! > Source/WebCore/accessibility/AXObjectCache.cpp:576 > - VisiblePosition visiblePos = VisiblePosition(textMarkerData.node, textMarkerData.offset, textMarkerData.affinity); > + VisiblePosition visiblePos = VisiblePosition(Position(textMarkerData.node, textMarkerData.offset, Position::PositionIsOffsetInAnchor), textMarkerData.affinity); Are you sure textMarkerData.node can always contain a position inside? i.e. is there a guarantee that node can't be br, img, etc...? > Source/WebCore/accessibility/AccessibilityObject.cpp:339 > - return VisiblePosition(startRenderer->node(), 0, VP_DEFAULT_AFFINITY); > + return VisiblePosition(firstPositionInOrBeforeNode(startRenderer->node()), VP_DEFAULT_AFFINITY); I think you can just return firstPositionInOrBeforeNode(startRenderer->node() here. > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2493 > - VisiblePosition startPosition = VisiblePosition(node, 0, DOWNSTREAM); > + VisiblePosition startPosition = VisiblePosition(Position(node, Position::PositionIsBeforeAnchor), DOWNSTREAM); You should be calling positionBeforeNode(node) instead. > Source/WebCore/dom/Position.cpp:516 > - PositionIterator lastVisible = *this; > + PositionIterator lastVisible = m_anchorType == PositionIsAfterAnchor ? Position(m_anchorNode, caretMaxOffset(m_anchorNode.get())) : *this; You don't consider the possibility of m_anchorType == PositionIsBeforeAnchor? Also, can't you call parentAnchoredEquivalent? Maybe we need a variant of parentAnchoredEquivalent that uses caretMaxOffset/caretMinOffset? > Source/WebCore/dom/Range.cpp:1573 > + VisiblePosition visiblePosition(Position(m_start.container(), m_start.offset(), Position::PositionIsOffsetInAnchor), VP_DEFAULT_AFFINITY); I think you can just do: VisiblePosition visiblePosition = Position(m_start.container(), m_start.offset(), Position::PositionIsOffsetInAnchor) > Source/WebCore/editing/ReplaceSelectionCommand.cpp:887 > - originalVisPosBeforeEndBR = VisiblePosition(endBR, 0, DOWNSTREAM).previous(); > + originalVisPosBeforeEndBR = VisiblePosition(Position(endBR, Position::PositionIsBeforeAnchor), DOWNSTREAM).previous(); You should be calling positionBeforeNode(endBR) instead. > Source/WebCore/editing/SelectionController.cpp:1319 > - VisiblePosition beforeOwnerElement(VisiblePosition(ownerElementParent, ownerElementNodeIndex, SEL_DEFAULT_AFFINITY)); > - VisiblePosition afterOwnerElement(VisiblePosition(ownerElementParent, ownerElementNodeIndex + 1, VP_UPSTREAM_IF_POSSIBLE)); > + VisiblePosition beforeOwnerElement(VisiblePosition(Position(ownerElementParent, ownerElementNodeIndex, Position::PositionIsOffsetInAnchor), SEL_DEFAULT_AFFINITY)); > + VisiblePosition afterOwnerElement(VisiblePosition(Position(ownerElementParent, ownerElementNodeIndex + 1, Position::PositionIsOffsetInAnchor), VP_UPSTREAM_IF_POSSIBLE)); We should just get rid of SEL_DEFAULT_AFFINITY. > Source/WebCore/editing/TextIterator.cpp:881 > + VisiblePosition startPos = VisiblePosition(Position(m_startContainer, m_startOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM); Ditto about m_startContainer can be br, img, etc... > Source/WebCore/editing/TextIterator.cpp:882 > + VisiblePosition currPos = VisiblePosition(Position(m_node, Position::PositionIsBeforeAnchor), DOWNSTREAM); Should be calling positionBeforeNode(m_node). > Source/WebCore/editing/VisiblePosition.cpp:512 > + Text* textNode = static_cast<Text*>(pos.containerNode()); > + unsigned offset = pos.offsetInContainerNode(); offsetInContainerNode will hit an assertion if pos can ever be before/after anchor node. You should probably check the anchor type above and bail out if the position was before/after a node. But maybe we need to take care of a position before a text node? Regardless, this change can't be right as is so r-. > Source/WebCore/editing/VisiblePosition.cpp:618 > - r->setStart(p.node(), p.deprecatedEditingOffset(), code); > + r->setStart(p.containerNode(), p.offsetInContainerNode(), code); Ditto about assertion in offsetInContainerNode. > Source/WebCore/editing/VisiblePosition.cpp:628 > - r->setEnd(p.node(), p.deprecatedEditingOffset(), code); > + r->setEnd(p.containerNode(), p.offsetInContainerNode(), code); Ditto. > Source/WebCore/editing/VisiblePosition.cpp:647 > + if (!p.node()->isDescendantOf(node)) Should this be calling containerNode? Or will that break something? > Source/WebCore/editing/VisiblePosition.cpp:661 > + if (!p.node()->isDescendantOf(node)) Ditto about node() > Source/WebCore/editing/visible_units.cpp:-434 > + Position pos; > if (endNode->hasTagName(brTag)) { > - endOffset = 0; I've got the feeling that this should really be checking editingIgnoresContent instead of just br. > Source/WebCore/editing/visible_units.cpp:434 > + pos = Position(endNode, Position::PositionIsBeforeAnchor); Should read: pos = positionBeforeNode(endNode) > Source/WebCore/editing/visible_units.cpp:440 > + pos = Position(endNode, endOffset, Position::PositionIsOffsetInAnchor); If you make the change about to use editingIgnoresContent then this position will be safe but otherwise, there is a potential that endNode is img, etc... > Source/WebCore/editing/visible_units.cpp:580 > + return VisiblePosition(Position(rootElement, 0, Position::PositionIsOffsetInAnchor), DOWNSTREAM); Should call firstPositionInNode(rootElement) > Source/WebCore/editing/visible_units.cpp:685 > - return VisiblePosition(rootElement, rootElement ? rootElement->childNodeCount() : 0, DOWNSTREAM); > + return VisiblePosition(Position(rootElement, rootElement ? rootElement->childNodeCount() : 0, Position::PositionIsOffsetInAnchor), DOWNSTREAM); Ditto. > Source/WebCore/editing/visible_units.cpp:803 > + > + return VisiblePosition(Position(node, type), DOWNSTREAM); Can we ASSERT(!offset) here? > Source/WebCore/editing/visible_units.cpp:941 > - return VisiblePosition(startBlock, startBlock->childNodeCount(), VP_DEFAULT_AFFINITY); > + return VisiblePosition(Position(startBlock, startBlock->childNodeCount(), Position::PositionIsOffsetInAnchor), VP_DEFAULT_AFFINITY); Should call lastPositionInNode(startBlock) > Source/WebCore/editing/visible_units.cpp:966 > - return VisiblePosition(node->document()->documentElement(), 0, DOWNSTREAM); > + return VisiblePosition(Position(node->document()->documentElement(), 0, Position::PositionIsOffsetInAnchor), DOWNSTREAM); Should call firstPositionInNode(node->document()->documentElement()). > Source/WebCore/editing/visible_units.cpp:980 > - return VisiblePosition(doc, doc->childNodeCount(), DOWNSTREAM); > + return VisiblePosition(Position(doc, doc->childNodeCount(), Position::PositionIsOffsetInAnchor), DOWNSTREAM); lastPositionInNode again. > Source/WebCore/editing/visible_units.cpp:1137 > + : VisiblePosition(Position(logicalStartNode, Position::PositionIsBeforeAnchor), DOWNSTREAM); Should call positionBeforeNode(logicalStartNode). > Source/WebCore/editing/visible_units.cpp:1173 > + pos = Position(logicalEndNode, Position::PositionIsBeforeAnchor); positionBeforeNode. > Source/WebCore/editing/visible_units.cpp:1181 > + pos = Position(logicalEndNode, Position::PositionIsAfterAnchor); positionAfterNode. > Source/WebCore/page/DOMSelection.cpp:214 > - m_frame->selection()->moveTo(VisiblePosition(node, offset, DOWNSTREAM)); > + m_frame->selection()->moveTo(VisiblePosition(Position(node, offset, Position::PositionIsOffsetInAnchor), DOWNSTREAM)); Is it always safe to create a position inside node? > Source/WebCore/page/DOMSelection.cpp:268 > - VisiblePosition visibleBase = VisiblePosition(baseNode, baseOffset, DOWNSTREAM); > - VisiblePosition visibleExtent = VisiblePosition(extentNode, extentOffset, DOWNSTREAM); > + VisiblePosition visibleBase = VisiblePosition(Position(baseNode, baseOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM); > + VisiblePosition visibleExtent = VisiblePosition(Position(extentNode, extentOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM); Ditto. > Source/WebCore/page/DOMSelection.cpp:285 > - m_frame->selection()->moveTo(VisiblePosition(node, offset, DOWNSTREAM)); > + m_frame->selection()->moveTo(VisiblePosition(Position(node, offset, Position::PositionIsOffsetInAnchor), DOWNSTREAM)); Ditto. > Source/WebCore/page/DOMSelection.cpp:356 > - m_frame->selection()->setExtent(VisiblePosition(node, offset, DOWNSTREAM)); > + m_frame->selection()->setExtent(VisiblePosition(Position(node, offset, Position::PositionIsOffsetInAnchor), DOWNSTREAM)); Ditto. > Source/WebCore/rendering/RenderObject.cpp:2657 > - return VisiblePosition(node, offset, affinity); > + return VisiblePosition(Position(node, offset), affinity); This is creating a legacy editing position. Is there a reason we can't get rid of this? > Source/WebCore/svg/SVGTextContentElement.cpp:148 > + VisiblePosition start(Position(const_cast<SVGTextContentElement*>(this), 0, Position::PositionIsOffsetInAnchor), SEL_DEFAULT_AFFINITY); firstPositionInNode Comment on attachment 81315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81315&action=review >> Source/WebCore/accessibility/AXObjectCache.cpp:576 >> + VisiblePosition visiblePos = VisiblePosition(Position(textMarkerData.node, textMarkerData.offset, Position::PositionIsOffsetInAnchor), textMarkerData.affinity); > > Are you sure textMarkerData.node can always contain a position inside? i.e. is there a guarantee that node can't be br, img, etc...? This concern is valid. >> Source/WebCore/accessibility/AccessibilityObject.cpp:339 >> + return VisiblePosition(firstPositionInOrBeforeNode(startRenderer->node()), VP_DEFAULT_AFFINITY); > > I think you can just return firstPositionInOrBeforeNode(startRenderer->node() here. I'm okay with this style change. >> Source/WebCore/dom/Position.cpp:516 >> + PositionIterator lastVisible = m_anchorType == PositionIsAfterAnchor ? Position(m_anchorNode, caretMaxOffset(m_anchorNode.get())) : *this; > > You don't consider the possibility of m_anchorType == PositionIsBeforeAnchor? Also, can't you call parentAnchoredEquivalent? Maybe we need a variant of parentAnchoredEquivalent that uses caretMaxOffset/caretMinOffset? This deserves a FIXME perhaps. PositionIterator doesn't check offsets, and has no notion of a position type. Because we don't store an offset for Before/After positions, we need to generate the offset to feed into PositionIterator for the After position, or it will be considered a Before one. >> Source/WebCore/dom/Range.cpp:1573 >> + VisiblePosition visiblePosition(Position(m_start.container(), m_start.offset(), Position::PositionIsOffsetInAnchor), VP_DEFAULT_AFFINITY); > > I think you can just do: > VisiblePosition visiblePosition = Position(m_start.container(), m_start.offset(), Position::PositionIsOffsetInAnchor) Good call. >> Source/WebCore/editing/SelectionController.cpp:1319 >> + VisiblePosition afterOwnerElement(VisiblePosition(Position(ownerElementParent, ownerElementNodeIndex + 1, Position::PositionIsOffsetInAnchor), VP_UPSTREAM_IF_POSSIBLE)); > > We should just get rid of SEL_DEFAULT_AFFINITY. New bug! :D >> Source/WebCore/editing/TextIterator.cpp:881 >> + VisiblePosition startPos = VisiblePosition(Position(m_startContainer, m_startOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM); > > Ditto about m_startContainer can be br, img, etc... We've already ensured m_startContainer is a container and m_node is one of its descendants. >> Source/WebCore/editing/VisiblePosition.cpp:512 >> + unsigned offset = pos.offsetInContainerNode(); > > offsetInContainerNode will hit an assertion if pos can ever be before/after anchor node. You should probably check the anchor type above and bail out if the position was before/after a node. But maybe we need to take care of a position before a text node? Regardless, this change can't be right as is so r-. There is an outstanding question of whether we should use before/after positions to represent text nodes. We should be dodging the assertion, and fix "after" positions. Before should simply use 0 as its offset. None of this is yet an issue because Position::downstream/upstream only return legacy positions :-/ >> Source/WebCore/editing/VisiblePosition.cpp:618 >> + r->setStart(p.containerNode(), p.offsetInContainerNode(), code); > > Ditto about assertion in offsetInContainerNode. Here we're explicitly constructing a parent anchored position, which fits with the usage model of offsetInContainerNode. >> Source/WebCore/editing/VisiblePosition.cpp:628 >> + r->setEnd(p.containerNode(), p.offsetInContainerNode(), code); > > Ditto. Ditto that this is being used properly. >> Source/WebCore/editing/VisiblePosition.cpp:647 >> + if (!p.node()->isDescendantOf(node)) > > Should this be calling containerNode? Or will that break something? Breaks things. I'll add a fixme. >> Source/WebCore/editing/visible_units.cpp:-434 >> - endOffset = 0; > > I've got the feeling that this should really be checking editingIgnoresContent instead of just br. This is simply br because we explicitly want the position *before* a br. We handle Text explicitly, then all other nodes we use an after position. >> Source/WebCore/editing/visible_units.cpp:440 >> + pos = Position(endNode, endOffset, Position::PositionIsOffsetInAnchor); > > If you make the change about to use editingIgnoresContent then this position will be safe but otherwise, there is a potential that endNode is img, etc... This call is only made for text boxes. This code will never be reached for any other types of content. >> Source/WebCore/editing/visible_units.cpp:803 >> + return VisiblePosition(Position(node, type), DOWNSTREAM); > > Can we ASSERT(!offset) here? We're not explicitly trying to prevent the offset from being set here. I don't think that would add any real protection. >> Source/WebCore/page/DOMSelection.cpp:214 >> + m_frame->selection()->moveTo(VisiblePosition(Position(node, offset, Position::PositionIsOffsetInAnchor), DOWNSTREAM)); > > Is it always safe to create a position inside node? As a DOMSelection, this should always be valid. I can add an ASSERT. >> Source/WebCore/rendering/RenderObject.cpp:2657 >> + return VisiblePosition(Position(node, offset), affinity); > > This is creating a legacy editing position. Is there a reason we can't get rid of this? Unfortunately yes. Since this function lives all the way down in RenderObject, we really know nearly nothing about sort of position we're really making. This deserves another FIXME and some thought about how to refactor this functionality. Comment on attachment 81315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81315&action=review >> Source/WebCore/page/DOMSelection.cpp:268 >> + VisiblePosition visibleExtent = VisiblePosition(Position(extentNode, extentOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM); > > Ditto. Given this is accessible via JavaScript, it seems like we kind of need the old behavior actually... It seems like we need a validation path for creating Positions from Node/Offset pairs we can't control. Adding a FIXME and sticking to legacy pending a more elegant solution. Created attachment 81552 [details]
Patch
Comment on attachment 81552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81552&action=review The patch looks better but I think we need one more iteration. r- for various nit and concerns. > Source/WebCore/dom/Position.cpp:517 > + // FIXME: PositionIterator should respect Before and After positions. > + PositionIterator lastVisible = m_anchorType == PositionIsAfterAnchor ? Position(m_anchorNode, caretMaxOffset(m_anchorNode.get())) : *this; This still instantiates legacy editing position but I guess your FIXME implies that as well? > Source/WebCore/dom/Position.cpp:639 > + // FIXME: PositionIterator should respect Before and After positions. > + PositionIterator lastVisible = m_anchorType == PositionIsAfterAnchor ? Position(m_anchorNode, caretMaxOffset(m_anchorNode.get())) : *this; Ditto. > Source/WebCore/editing/VisiblePosition.cpp:510 > + Node* node = pos.containerNode(); > + if (!node || !node->isTextNode() || pos.anchorType() == Position::PositionIsAfterAnchor) > return 0; You should assert that pos.anchorType() is either PositionIsBeforeAnchor or PositionIsOffsetInAnchor after this early exit in the case more types are added. > Source/WebCore/editing/VisiblePosition.cpp:647 > + Position p = visiblePosition.deepEquivalent(); > + > + if (!p.containerNode()->isDescendantOf(node)) Why can't we do visiblePosition.deepEquivalent().containerNode()->isDescendantOf(node) instead? > Source/WebCore/editing/VisiblePosition.cpp:661 > + Position p = visiblePosition.deepEquivalent(); > + > + if (!p.containerNode()->isDescendantOf(node)) Ditto. > Source/WebCore/editing/visible_units.cpp:386 > - VisiblePosition visPos = VisiblePosition(startNode, startOffset, DOWNSTREAM); > + VisiblePosition visPos = VisiblePosition(Position(startNode, startOffset, Position::PositionIsOffsetInAnchor), DOWNSTREAM); Why is it safe to create a position inside startNode here? i.e. what guarantees that startNode isn't br, img, etc...? > Source/WebCore/editing/visible_units.cpp:433 > if (endNode->hasTagName(brTag)) { Could you fix this manual check against brTag or file a followup bug? > Source/WebCore/editing/visible_units.cpp:787 > - return VisiblePosition(n, i + 1, DOWNSTREAM); > + return VisiblePosition(Position(n, i + 1, type), DOWNSTREAM); I think we should just pass Position::PositionIsOffsetInAnchor as the type instead of "type" so that the correctness is self-evident. > Source/WebCore/editing/visible_units.cpp:851 > - return VisiblePosition(n, i, DOWNSTREAM); > + return VisiblePosition(Position(n, i, type), DOWNSTREAM); Ditto. > Source/WebCore/editing/visible_units.cpp:1137 > + VisiblePosition visPos = logicalStartNode->isTextNode() ? VisiblePosition(Position(logicalStartNode, logicalStartBox->caretMinOffset(), Position::PositionIsOffsetInAnchor), DOWNSTREAM) > + : VisiblePosition(positionBeforeNode(logicalStartNode), DOWNSTREAM); It's not obvious to me why logicalStartNode can always have a position inside. > Source/WebCore/editing/visible_units.cpp:1172 > if (logicalEndNode->hasTagName(brTag)) Same comment about calling editingIgnoreContents instead of manually checking against brTag. > Source/WebCore/page/DOMSelection.cpp:359 > - m_frame->selection()->setExtent(VisiblePosition(node, offset, DOWNSTREAM)); > + m_frame->selection()->setExtent(VisiblePosition(Position(node, offset, Position::PositionIsOffsetInAnchor), DOWNSTREAM)); I still don't think it's correct to create a Position with a random node. Maybe we should throw an exception? Auto-correcting it to the parent-anchored equivalent position is fine as well. Comment on attachment 81552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81552&action=review >> Source/WebCore/dom/Position.cpp:517 >> + PositionIterator lastVisible = m_anchorType == PositionIsAfterAnchor ? Position(m_anchorNode, caretMaxOffset(m_anchorNode.get())) : *this; > > This still instantiates legacy editing position but I guess your FIXME implies that as well? Unfortunately we don't have a choice until PositionIterator is aware of AnchorTypes. >> Source/WebCore/editing/visible_units.cpp:433 >> if (endNode->hasTagName(brTag)) { > > Could you fix this manual check against brTag or file a followup bug? See my previous comment. We don't want editingIgnoresContent here. We want a position at [br, beforeAnchorNode] as it's the end of the line. >> Source/WebCore/editing/visible_units.cpp:787 >> + return VisiblePosition(Position(n, i + 1, type), DOWNSTREAM); > > I think we should just pass Position::PositionIsOffsetInAnchor as the type instead of "type" so that the correctness is self-evident. If you think that improves the style... >> Source/WebCore/editing/visible_units.cpp:1137 >> + : VisiblePosition(positionBeforeNode(logicalStartNode), DOWNSTREAM); > > It's not obvious to me why logicalStartNode can always have a position inside. ?? It only has a position inside it when it's a text node. All other cases it's a position before the node. >> Source/WebCore/editing/visible_units.cpp:1172 >> if (logicalEndNode->hasTagName(brTag)) > > Same comment about calling editingIgnoreContents instead of manually checking against brTag. This is again an end position where we want a a position before br tags. This *will* break for line and paragraph separators once we support them. I plan to file another bug that's dependent on 53203. Comment on attachment 81552 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81552&action=review >>> Source/WebCore/editing/visible_units.cpp:433 >>> if (endNode->hasTagName(brTag)) { >> >> Could you fix this manual check against brTag or file a followup bug? > > See my previous comment. We don't want editingIgnoresContent here. We want a position at [br, beforeAnchorNode] as it's the end of the line. Ah, got it. >>> Source/WebCore/editing/visible_units.cpp:1137 >>> + : VisiblePosition(positionBeforeNode(logicalStartNode), DOWNSTREAM); >> >> It's not obvious to me why logicalStartNode can always have a position inside. > > ?? It only has a position inside it when it's a text node. All other cases it's a position before the node. Oops, I misread the code. Right, this is safe. >>> Source/WebCore/editing/visible_units.cpp:1172 >>> if (logicalEndNode->hasTagName(brTag)) >> >> Same comment about calling editingIgnoreContents instead of manually checking against brTag. > > This is again an end position where we want a a position before br tags. This *will* break for line and paragraph separators once we support them. I plan to file another bug that's dependent on 53203. Ok. It'll be great if you could file a bug against it. Created attachment 81698 [details]
Patch
Comment on attachment 81698 [details] Patch Clearing flags on attachment: 81698 Committed r77980: <http://trac.webkit.org/changeset/77980> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/77980 might have broken Qt Linux Release The following tests are not passing: fast/forms/input-maxlength-unsupported.html fast/forms/input-number-keyoperation.html fast/forms/input-number-unacceptable-style.html Crashes on Linux/Windows, but works fine on Mac... I'll get to the bottom of it. Created attachment 81769 [details]
Patch
(In reply to comment #24) > Created an attachment (id=81769) [details] > Patch @@ -451,10 +445,11 @@ Position VisiblePosition::canonicalPosit // To fix this, we need to either a) add code to all paintCarets to pass the responsibility off to // the appropriate renderer for VisiblePosition's like these, or b) canonicalize to the rightmost candidate // unless the affinity is upstream. - Node* node = position.node(); - if (!node) + if (position.isNull()) return Position(); + Node* node = position.anchorNode(); My previous patch used containerNode() instead of anchorNode(), but there's not a guaranteed container (e.g. before/after positions in shadow nodes). Since we're only interested in the Document, we don't actually care whether it's the anchorNode or containerNode(). (In reply to comment #25) > My previous patch used containerNode() instead of anchorNode(), but there's not a guaranteed container (e.g. before/after positions in shadow nodes). Since we're only interested in the Document, we don't actually care whether it's the anchorNode or containerNode(). I don't think that's right. node is also used to do: if (node->hasTagName(htmlTag) && !node->isContentEditable() && node->document()->body() && node->document()->body()->isContentEditable()) and Node *originalBlock = node->enclosingBlockFlowElement(); bool nextIsOutsideOriginalBlock = !nextNode->isDescendantOf(originalBlock) && nextNode != originalBlock; bool prevIsOutsideOriginalBlock = !prevNode->isDescendantOf(originalBlock) && prevNode != originalBlock; if (nextIsOutsideOriginalBlock && !prevIsOutsideOriginalBlock) return prev; (In reply to comment #26) > (In reply to comment #25) > > My previous patch used containerNode() instead of anchorNode(), but there's not a guaranteed container (e.g. before/after positions in shadow nodes). Since we're only interested in the Document, we don't actually care whether it's the anchorNode or containerNode(). > > I don't think that's right. node is also used to do: > > if (node->hasTagName(htmlTag) && !node->isContentEditable() && node->document()->body() && node->document()->body()->isContentEditable()) > > and > > Node *originalBlock = node->enclosingBlockFlowElement(); > bool nextIsOutsideOriginalBlock = !nextNode->isDescendantOf(originalBlock) && nextNode != originalBlock; > bool prevIsOutsideOriginalBlock = !prevNode->isDescendantOf(originalBlock) && prevNode != originalBlock; > if (nextIsOutsideOriginalBlock && !prevIsOutsideOriginalBlock) > return prev; You're right that we use node in those cases as well, but in both of these we really want anchorNode as well! If we have a position at [HTML, 1] (as the comment implies), we really don't want the containerNode. It seems like a weird case already. Worthy of a test case. (In reply to comment #27) > Worthy of a test case. I guess that would be editing/selection/select-all-005.html... I verified that containerNode() actually works fine in this test case, so I'm inclined to go with something like this for consistency's sake. Node* node = position.containerNode() ? position.containerNode() : position.anchorNode(); Thoughts? (In reply to comment #28) > (In reply to comment #27) > > Worthy of a test case. > > I guess that would be editing/selection/select-all-005.html... > > I verified that containerNode() actually works fine in this test case, so I'm inclined to go with something like this for consistency's sake. > > Node* node = position.containerNode() ? position.containerNode() : position.anchorNode(); > > Thoughts? If position is before or after a root shadow element, then originalBlock should be null. containerNode should also work for if (node->hasTagName(htmlTag) && !node->isContentEditable() && node->document()->body() && node->document()->body()->isContentEditable()) since we can't have a html tag inside a shadow DOM. So all we need seems to be a null-pointer check here. Also see my patch for https://bugs.webkit.org/show_bug.cgi?id=54053. In that patch, I'm preventing to pass an invalid position into editing code in the first place. In short, I don't think it's correct to call anchorNode here because it doesn't make any semantic sense except when obtaining the document. We should add an inline document() to Position so that we don't call anchorNode just to get its document. Created attachment 81900 [details]
Patch
(In reply to comment #30) > Created an attachment (id=81900) [details] > Patch I like the advice on Position, since it cleans up dependence on anchorNode to get a valid Document. I think we're finally good to go :) Comment on attachment 81900 [details]
Patch
r=me provided you have ran all layout tests on Windows and/or Linux.
Thanks for the patch!
Levi, are you going to land this patch? Or have you found any problems with this patch? Committed r80059: <http://trac.webkit.org/changeset/80059> http://trac.webkit.org/changeset/80059 might have broken GTK Linux 64-bit Debug The following tests are not passing: fast/viewport/viewport-112.html fast/viewport/viewport-121.html fast/viewport/viewport-122.html fast/viewport/viewport-125.html fast/viewport/viewport-129.html fast/viewport/viewport-35.html fast/viewport/viewport-46.html fast/viewport/viewport-52.html fast/viewport/viewport-53.html fast/viewport/viewport-54.html fast/viewport/viewport-55.html fast/viewport/viewport-66.html fast/viewport/viewport-67.html fast/viewport/viewport-68.html fast/viewport/viewport-69.html fast/viewport/viewport-70.html fast/viewport/viewport-71.html fast/viewport/viewport-72.html fast/viewport/viewport-73.html fast/viewport/viewport-74.html fast/viewport/viewport-75.html fast/viewport/viewport-77.html fast/viewport/viewport-78.html fast/viewport/viewport-79.html fast/viewport/viewport-83.html fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml http/tests/security/xss-DENIED-xsl-document-redirect.xml http/tests/security/xss-DENIED-xsl-external-entity-redirect.xml http/tests/xmlviewer/dumpAsText/wml.xml |