Bug 56025 – startOfBlock and endOfBlock may return a position inside hr

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 85189 [details]
fixes the bug

[Ryosuke Niwa]

Any chance I can get a review on this patch?

[Tony Chang]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 85396 [details]
Fixed per Tony's comment

[Ryosuke Niwa]

Committed r80793: <http://trac.webkit.org/changeset/80793>