Bug 86649

I'm fixing isStartOfDocument and isEndOfDocument to cross editing boundary and adding isEndOfEditableOrNonEditableContent which does what isEndOfDocument used to do. After this change, selectFrameElementInParentIfFullySelected is the only client of isStartOfDocument and isEndOfDocument. The problem is that this function was added in http://trac.webkit.org/changeset/8942 without any tests without any tests. As far as I know, this function had been broken for ages (it only checked if the selection started and ended at the beginning and the end of the current editable region). So I'm very inclined to just delete it altogether. If anything, we should probably support control selection on iframes per https://bugs.webkit.org/show_bug.cgi?id=12250. Darin, what is your opinion on this?

Created attachment 142833 [details] Renames isEndOfDocument and adds fixed versions functions

Created attachment 142836 [details] Removed erroneous change in FrameSelection

Comment on attachment 142836 [details] Removed erroneous change in FrameSelection Attachment 142836 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12726614

Comment on attachment 142836 [details] Removed erroneous change in FrameSelection Attachment 142836 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12731052

Comment on attachment 142836 [details] Removed erroneous change in FrameSelection Attachment 142836 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12732062

Comment on attachment 142836 [details] Removed erroneous change in FrameSelection View in context: https://bugs.webkit.org/attachment.cgi?id=142836&action=review > Source/WebCore/editing/visible_units.cpp:-1379 > -bool isStartOfDocument(const VisiblePosition &p) > -{ > - return p.isNotNull() && p.previous().isNull(); > -} > - > -bool isEndOfDocument(const VisiblePosition &p) > -{ > - return p.isNotNull() && p.next().isNull(); > -} > - How odd. I shouldn't be removing these functions. Will fix.

Comment on attachment 142836 [details] Removed erroneous change in FrameSelection Attachment 142836 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12730070

Comment on attachment 142836 [details] Removed erroneous change in FrameSelection Attachment 142836 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12729275

Created attachment 142847 [details] Fixed builds

For now, I kept selectFrameElementInParentIfFullySelected intact (in fact I'm probably fixing a bug in it).

Comment on attachment 142847 [details] Fixed builds View in context: https://bugs.webkit.org/attachment.cgi?id=142847&action=review > Source/WebCore/ChangeLog:10 > + Also added new versions of isStartOfDocument and isEndOfDocument that correctly cross editing boundaries Is this true? I don't see the new versions.

(In reply to comment #12) > (From update of attachment 142847 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142847&action=review > > > Source/WebCore/ChangeLog:10 > > + Also added new versions of isStartOfDocument and isEndOfDocument that correctly cross editing boundaries > > Is this true? I don't see the new versions. Ugh... my change in isStartOfDocument and isEndOfDocument have been lost.

Created attachment 143050 [details] Put back the missing changes

Comment on attachment 143050 [details] Put back the missing changes View in context: https://bugs.webkit.org/attachment.cgi?id=143050&action=review > Source/WebCore/ChangeLog:8 > + isEndOfDocument to isEndOfEditableOrNonEditableContent because that's what this function checks. I believe you're missing the word "Changed" at the beginning :) > Source/WebCore/ChangeLog:11 > + Also added new versions of isStartOfDocument and isEndOfDocument that correctly cross editing boundaries > + to be used in selectFrameElementInParentIfFullySelected. Since you didn't create a isStartOfEditableOrNonEditableContent (man that's long) was isStartOfDocument just never used? Is isEndOfDocument still used after this patch?

Comment on attachment 143050 [details] Put back the missing changes View in context: https://bugs.webkit.org/attachment.cgi?id=143050&action=review >> Source/WebCore/ChangeLog:11 >> + to be used in selectFrameElementInParentIfFullySelected. > > Since you didn't create a isStartOfEditableOrNonEditableContent (man that's long) was isStartOfDocument just never used? Is isEndOfDocument still used after this patch? isEndOfDocument and isStartOfDocument are still used in selectFrameElementInParentIfFullySelected. See my comment #1.

Comment on attachment 143050 [details] Put back the missing changes View in context: https://bugs.webkit.org/attachment.cgi?id=143050&action=review > Source/WebCore/editing/visible_units.cpp:1400 > +bool isEndOfEditableOrNonEditableContent(const VisiblePosition &p) This is still a pretty poor name. How about isAtEditableBoundary or even isAtEndOfEditableBoundary?

(In reply to comment #17) > (From update of attachment 143050 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143050&action=review > > > Source/WebCore/editing/visible_units.cpp:1400 > > +bool isEndOfEditableOrNonEditableContent(const VisiblePosition &p) > > This is still a pretty poor name. How about isAtEditableBoundary or even isAtEndOfEditableBoundary? isAtEditableBoundary is inaccurate. isAtEndOfEditableBoundary doesn't make sense because an editing boundary is a point in DOM and a point/position doesn't have a start/end.

> isAtEditableBoundary is inaccurate. isAtEndOfEditableBoundary doesn't make sense because an editing boundary is a point in DOM and a point/position doesn't have a start/end. Then what is isEndOfEditableOrNonEditableContent trying to communicate if it isn't a boundary? I'm confused.

Comment on attachment 143050 [details] Put back the missing changes View in context: https://bugs.webkit.org/attachment.cgi?id=143050&action=review >> Source/WebCore/ChangeLog:8 >> + isEndOfDocument to isEndOfEditableOrNonEditableContent because that's what this function checks. > > I believe you're missing the word "Changed" at the beginning :) Make this a sentence. >>> Source/WebCore/editing/visible_units.cpp:1400 >>> +bool isEndOfEditableOrNonEditableContent(const VisiblePosition &p) >> >> This is still a pretty poor name. How about isAtEditableBoundary or even isAtEndOfEditableBoundary? > > isAtEditableBoundary is inaccurate. isAtEndOfEditableBoundary doesn't make sense because an editing boundary is a point in DOM and a point/position doesn't have a start/end. I'm not in love with this name, but it's at least accurate in what it portrays :)

Committed r117813: <http://trac.webkit.org/changeset/117813>