Bug 56025

Summary: startOfBlock and endOfBlock may return a position inside hr
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Minor CC: darin, enrica, eric, kalman, leviw, ojan, tkent, tony, xji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 52099, 56027    
Attachments:
Description Flags
fixes the bug
none
Fixed per Tony's comment tony: review+

Ryosuke Niwa
Reported 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.
Attachments
fixes the bug (11.48 KB, patch)
2011-03-09 10:22 PST, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Fixed per Tony's comment (11.89 KB, patch)
2011-03-10 15:30 PST, Ryosuke Niwa 		tony: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-03-09 10:22:06 PST
Created attachment 85189 [details] fixes the bug
Ryosuke Niwa
Comment 2 2011-03-09 12:40:53 PST
Any chance I can get a review on this patch?
Tony Chang
Comment 3 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).
Ryosuke Niwa
Comment 4 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.
Ryosuke Niwa
Comment 5 2011-03-10 15:30:37 PST
Created attachment 85396 [details] Fixed per Tony's comment
Ryosuke Niwa
Comment 6 2011-03-10 16:53:11 PST
Committed r80793: <http://trac.webkit.org/changeset/80793>
Note You need to log in before you can comment on or make changes to this bug.