Bug 56025 - startOfBlock and endOfBlock may return a position inside hr
Summary: startOfBlock and endOfBlock may return a position inside hr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Minor
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 52099 56027
  Show dependency treegraph
 
Reported: 2011-03-09 09:59 PST by Ryosuke Niwa
Modified: 2011-03-10 16:53 PST (History)
9 users (show)

See Also:


Attachments
fixes the bug (11.48 KB, patch)
2011-03-09 10:22 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per Tony's comment (11.89 KB, patch)
2011-03-10 15:30 PST, Ryosuke Niwa
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-03-09 09:59:49 PST
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.
Comment 1 Ryosuke Niwa 2011-03-09 10:22:06 PST
Created attachment 85189 [details]
fixes the bug
Comment 2 Ryosuke Niwa 2011-03-09 12:40:53 PST
Any chance I can get a review on this patch?
Comment 3 Tony Chang 2011-03-10 15:05:25 PST
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).
Comment 4 Ryosuke Niwa 2011-03-10 15:24:52 PST
(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.
Comment 5 Ryosuke Niwa 2011-03-10 15:30:37 PST
Created attachment 85396 [details]
Fixed per Tony's comment
Comment 6 Ryosuke Niwa 2011-03-10 16:53:11 PST
Committed r80793: <http://trac.webkit.org/changeset/80793>