Right now, startOfBlock and endOfBlock call Node::enclosingBlockFlowElement to find the enclosing block node. However, this allows the enclosing block to be an hr element because they don't check the value of editingIgnoresContent. We should call enclosingBlock instead so that we don't return such a position.
Created attachment 85189 [details] fixes the bug
Any chance I can get a review on this patch?
Comment on attachment 85189 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=85189&action=review Why is there no layout test? > Source/WebCore/editing/htmlediting.h:241 > +String stringWithRebalancedWhitespace(const String&, bool startIsStartOfParagraph, bool endIsEndOfParagraph); This looks like a good candidate for future deboolification. > Source/WebCore/editing/visible_units.cpp:917 > +VisiblePosition startOfBlock(const VisiblePosition &c, EditingBoundaryCrossingRule rule) Nit: Why is VisiblePosition |c|? Might be worth a more descriptive name. > Source/WebCore/editing/visible_units.cpp:937 > - return !a.isNull() && enclosingBlockFlowElement(a) == enclosingBlockFlowElement(b); > + return !a.isNull() && enclosingBlock(a.deepEquivalent().containerNode()) == enclosingBlock(b.deepEquivalent().containerNode()); Is this just a cleanup? It doesn't seem directly related to the rest of the change (startOfBlock/endOfBlock).
(In reply to comment #3) > (From update of attachment 85189 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=85189&action=review > > Why is there no layout test? This change can't be tested because those positions are used only internally in editing code. I'll add comment in the change log entry. > > Source/WebCore/editing/htmlediting.h:241 > > +String stringWithRebalancedWhitespace(const String&, bool startIsStartOfParagraph, bool endIsEndOfParagraph); > > This looks like a good candidate for future deboolification. Agreed probably not in this patch though. > > Source/WebCore/editing/visible_units.cpp:917 > > +VisiblePosition startOfBlock(const VisiblePosition &c, EditingBoundaryCrossingRule rule) > > Nit: Why is VisiblePosition |c|? Might be worth a more descriptive name. Will rename c to visiblePosition and p to position. > > Source/WebCore/editing/visible_units.cpp:937 > > - return !a.isNull() && enclosingBlockFlowElement(a) == enclosingBlockFlowElement(b); > > + return !a.isNull() && enclosingBlock(a.deepEquivalent().containerNode()) == enclosingBlock(b.deepEquivalent().containerNode()); > > Is this just a cleanup? It doesn't seem directly related to the rest of the change (startOfBlock/endOfBlock). No, this is a part of the bug fix because startOfBlock / endOfBlock now use enclosingBlock, we need to use it here as well or otherwise we'll have inconsistency.
Created attachment 85396 [details] Fixed per Tony's comment
Committed r80793: <http://trac.webkit.org/changeset/80793>