Bug 51001

Created attachment 76483 [details] Repro

Created attachment 76506 [details] Patch

I've looked at the failing test, and it's caused by the following behaviour: given a dom like <div> Foo <span contenteditable=false>Bar|</span> Baz </div> (Cursor indicated with a | vertical bar), moving backwards by a word results in <div> Foo| <span contenteditable>Bar</span> Baz </div> (causing the selection to be cancelled altogether, it seems), rather than <div contenteditable> Foo <span contenteditable=false>|Bar</span> Baz </div> http://trac.webkit.org/changeset/64974 fixed this problem by stopping iteration when a contenteditable boundary is crossed, a side-effect being the problem described in this bug. I've upload a potential fix (with a test case) which makes the editability-crossing check more specific*. However, I think a nicer approach would be to remove the check altogether and address the actual cause of the issue, which I think is a quirk in VisiblePosition::canonicalPosition. I haven't looked into it much further than that, however (it's a slightly mindbending method at first glance). * and renames accordingly.

Oops, those 3 html snippets need sed run over them. sed s/contenteditable=false/contenteditable/g

Comment on attachment 76506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76506&action=review Are we sure we want to make this change? I feel like previousBoundary can now cross editing boundaries and potentially return a position inside non-content editable area even when c is in content editable area. e.g. if we had something like <div contenteditable>hello wo<span contenteditable="false">rld W</span>ebkit</div>, do we still do the right thing? i.e. do not enter inside span when moving forward from hello by word. > LayoutTests/ChangeLog:7 > + Could you add a description as to what kind of test you're adding here? > LayoutTests/editing/selection/extend-backward-by-word-over-non-editable-expected.txt:6 > +Tests that extending backwards over a non-contenteditable word only extends over that word > +| "foo bar <#selection-focus>" > +| <span> > +| contenteditable="false" > +| "baz" > +| "<#selection-anchor> qux quux" Could you output PASS/FAIl based on the final selection? I'm concerned that people wrongfully rebaseline this test when this regresses or they'll have a hard time figuring out what is the right behavior. Printing PASS/FAIL makes it much easier to know what's correct and what's not. > WebCore/ChangeLog:8 > + > + Test: editing/selection/extend-backward-by-word-over-non-editable.html You need to describe what you're change is doing. I usually try to explain what caused the bug and how I fixed it. > WebCore/editing/TextIterator.h:44 > - TextIteratorEndsAtEditingBoundary = 1 << 3, > + TextIteratorEndsWhenAscendsAcrossEditingBoundary = 1 << 3, I'm not sure "when ascends across" is descriptive but I can't think of a better name now.

Comment on attachment 76506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76506&action=review In response to your overall comment: yes it's fine, and doesn't break the case you describe. I also thought it might, checked it, and it must have always worked; I haven't looked into it too much, but there must be code elsewhere that prevents this from occurring. I think the only reason the weirdness happens here is due to something in VisiblePosition::canonicalPosition, as I mentioned. (Also note that the change which introduced TextIteratorEndsAtEditingBoundary was only committed a few months ago.) I'll fix up the other parts of this patch tonight or tomorrow morning. >> WebCore/editing/TextIterator.h:44 >> + TextIteratorEndsWhenAscendsAcrossEditingBoundary = 1 << 3, > > I'm not sure "when ascends across" is descriptive but I can't think of a better name now. I agree it sounds weird, but likewise couldn't think of a better name at the time.

I’d like to hear Enrica’s thoughts on this change. She added the editable boundary machinery relatively recently. I’m concerned that this is a bit inflexible because only editable boundary changes that are immediate children are considered. Could there be cases where there is one additional level of nesting but are otherwise correct to handle in this manner?

(In reply to comment #6) > (From update of attachment 76506 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76506&action=review > > In response to your overall comment: yes it's fine, and doesn't break the case you describe. I also thought it might, checked it, and it must have always worked; I haven't looked into it too much, but there must be code elsewhere that prevents this from occurring. I think the only reason the weirdness happens here is due to something in VisiblePosition::canonicalPosition, as I mentioned. You're right. In the case of selection, we do fix that in respondToSelectionChange. However, previousWordPosition is also called by removeSpellAndCorrectionMarkersFromWordsToBeEdited also calls previousWordPosition. I don't see how it can be taking care of this problem. Also, I'm quite surprised that this fix only applies to previousBoundary and not to nextBoundary. Also I'd like to see some comment or better yet an encapsulation to prevent us from calling nextWordPosition & previousWordPosition wrongfully at places where crossing editing boundary is not okay.

(In reply to comment #7) > I’d like to hear Enrica’s thoughts on this change. She added the editable boundary machinery relatively recently. I was under the impression these were added by morrita? http://trac.webkit.org/changeset/64974 (this is the changeset linked to elsewhere in this bug report). The bug itself is https://bugs.webkit.org/show_bug.cgi?id=36359. > > I’m concerned that this is a bit inflexible because only editable boundary changes that are immediate children are considered. Could there be cases where there is one additional level of nesting but are otherwise correct to handle in this manner? Yes, there probably do exist corner cases. That said, this bug is caused by a regression from ~4 months ago, and as a tradeoff imo this bug is more important to fix. Not that I'm suggesting this is the right thing to do -- I will into VisiblePosition::canonicalPosition and its handling of contenteditable boundaries, and hopefully fix it there (and then remove the TextIteratorEndsAtEditingBoundary thing altogether).

> You're right. In the case of selection, we do fix that in respondToSelectionChange. However, previousWordPosition is also called by removeSpellAndCorrectionMarkersFromWordsToBeEdited also calls previousWordPosition. I don't see how it can be taking care of this problem. What problem to you mean? > Also, I'm quite surprised that this fix only applies to previousBoundary and not to nextBoundary. Only previousBoundary uses the TextIteratorEndsAtEditingBoundary, because the patch which caused the regression only changed previousBoundary. As I mentioned, there is indeed a bug with moving forward by words but I haven't looked into whether it's caused by nextBoundary. > > Also I'd like to see some comment or better yet an encapsulation to prevent us from calling nextWordPosition & previousWordPosition wrongfully at places where crossing editing boundary is not okay. I think that it was always fine, and the patch which caused this regression introduced the editing boundary crossing check purely to fix the bug addressed by the patch (https://bugs.webkit.org/show_bug.cgi?id=36359) i.e. cannot select a word in a contenteditable span at the start of a line.

Created attachment 76850 [details] Patch

After staring at the code for a while and playing with the debugger, the original patch doesn't make much sense. Instead, I've reverted the change[1] which caused this regression and fixed the problem in a different way[2]. [1] Adding editability boundary checks to TextIterator.h/cpp wasn't necessary; this is done elsewhere (rniwa: I can't find the respondToSelectionChange you mention, but anyway...). [2] This is the use-rightmost-Position stuff. Honestly I'm not entirely sure how legitimate that is, but it's a pretty clean way of fixing the bug imo. Otherwise I'd say, just revert the editability boundary check stuff and leave it at that.

Comment on attachment 76850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76850&action=review > LayoutTests/editing/selection/extend-backward-by-word-over-non-editable-expected.txt:11 > +Dump of markup 2: > +| "PASS" This doesn't look right. I don't think this is a good usage of dump-as-markup. > LayoutTests/editing/selection/extend-backward-by-word-over-non-editable.html:19 > + Markup.description("Tests that extending backwards over a non-contenteditable word only extends over that word"); > + Markup.dump(textContainer); > + Markup.dump(result); Is there any point in using dump-as-markup.js here? Doesn't printing PASS/FAIL suffice? I think we should just call dumpAsText() and print PASS/FAIL. > WebCore/editing/visible_units.cpp:129 > + pos = it.range().get()->startPosition(); No need to call get(). We have overloaded operator->().

(In reply to comment #12) > After staring at the code for a while and playing with the debugger, the original patch doesn't make much sense. Instead, I've reverted the change[1] which caused this regression and fixed the problem in a different way[2]. You're probably referring to the changeset http://trac.webkit.org/changeset/64974 but this change set moved some of editing boundary checks from visible_units into TextIterator. Your patch seems to revert the changes in TextIterator but does not introduce new checks into visible_units. How is this correct? > [1] Adding editability boundary checks to TextIterator.h/cpp wasn't necessary; this is done elsewhere (rniwa: I can't find the respondToSelectionChange you mention, but anyway...). I meant VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries.

Comment on attachment 76850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76850&action=review >> LayoutTests/editing/selection/extend-backward-by-word-over-non-editable.html:19 >> + Markup.dump(result); > > Is there any point in using dump-as-markup.js here? Doesn't printing PASS/FAIL suffice? I think we should just call dumpAsText() and print PASS/FAIL. Yes I suppose. otoh it might be nice to see if it fails what the failure is, and when running the test by hand at least this way it also says pass/fail in the dom. >> WebCore/editing/visible_units.cpp:129 >> + pos = it.range().get()->startPosition(); > > No need to call get(). We have overloaded operator->(). oops, remnant of gdb reshuffling

(In reply to comment #14) > (In reply to comment #12) > > After staring at the code for a while and playing with the debugger, the original patch doesn't make much sense. Instead, I've reverted the change[1] which caused this regression and fixed the problem in a different way[2]. > > You're probably referring to the changeset http://trac.webkit.org/changeset/64974 but this change set moved some of editing boundary checks from visible_units into TextIterator. Your patch seems to revert the changes in TextIterator but does not introduce new checks into visible_units. How is this correct? Yep I'm referring to that changeset. It seems to me like the significant editability checks were moved to Position::parentEditingBoundary() as opposed to the text iterator changes that I reverted. The patch did multiple things, and has a non-trivial test, which still passes (and no other editing tests fail either). > > > [1] Adding editability boundary checks to TextIterator.h/cpp wasn't necessary; this is done elsewhere (rniwa: I can't find the respondToSelectionChange you mention, but anyway...). > > I meant VisibleSelection::adjustSelectionToAvoidCrossingEditingBoundaries. Thanks.

(In reply to comment #15) > (From update of attachment 76850 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76850&action=review > > >> LayoutTests/editing/selection/extend-backward-by-word-over-non-editable.html:19 > >> + Markup.dump(result); > > > > Is there any point in using dump-as-markup.js here? Doesn't printing PASS/FAIL suffice? I think we should just call dumpAsText() and print PASS/FAIL. > > Yes I suppose. otoh it might be nice to see if it fails what the failure is, and when running the test by hand at least this way it also says pass/fail in the dom. I think we should just be informative in FAIL case. You could print information like "wrong offset" or "wrong container" > Yep I'm referring to that changeset. It seems to me like the significant editability checks were moved to Position::parentEditingBoundary() as opposed to the text iterator changes that I reverted. The patch did multiple things, and has a non-trivial test, which still passes (and no other editing tests fail either). Ah, now I see it. We should have Morrita-san & Ojan take a look at this patch regardless. I'm very scared of removing editing boundary checks because 1. I feel we don't have adequate test coverage and 2. we hit assertions if we try to modify non-editable nodes.

Comment on attachment 76850 [details] Patch I don’t understand the status here. Ryosuke made a couple comments, but did not say review- or review+. I think the change is fine, but I think Ryosuke’s comments should be dealt with. Setting review+ and commit-queue- for now.

Created attachment 77653 [details] Patch

(In reply to comment #18) > (From update of attachment 76850 [details]) > I don’t understand the status here. Ryosuke made a couple comments, but did not say review- or review+. I think the change is fine, but I think Ryosuke’s comments should be dealt with. Setting review+ and commit-queue- for now. Coincidentally I was just (finally, after weeks) updating the patch when you replied to this. Thanks for looking at it. I've rewritten the test and fixed that typo as Ryosuke requested.

Comment on attachment 77653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77653&action=review For now I’m setting commit-queue- because you might want to make changes based on my comments. > WebCore/editing/TextIterator.cpp:1239 > +bool SimplifiedBackwardsTextIterator::setNodeIfNotNull(Node* node) All this function does is an assignment statement and a null check. I think both call sites would be clearer if this was written there instead of using a function. > WebCore/editing/visible_units.cpp:52 > +static Position rightmostEquivalent(Position p) In WebKit code normally don’t use letters for names. The word “position” would be better than “p” given our usual style. Although there are quite a few counterexamples in this source file. For new code it would be better to use words.

Created attachment 77655 [details] Patch

(In reply to comment #21) > (From update of attachment 77653 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77653&action=review > > For now I’m setting commit-queue- because you might want to make changes based on my comments. > > > WebCore/editing/TextIterator.cpp:1239 > > +bool SimplifiedBackwardsTextIterator::setNodeIfNotNull(Node* node) > > All this function does is an assignment statement and a null check. I think both call sites would be clearer if this was written there instead of using a function. > > > WebCore/editing/visible_units.cpp:52 > > +static Position rightmostEquivalent(Position p) > > In WebKit code normally don’t use letters for names. The word “position” would be better than “p” given our usual style. Although there are quite a few counterexamples in this source file. For new code it would be better to use words. Thanks again.

Comment on attachment 77655 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77655&action=review > WebCore/editing/TextIterator.cpp:1143 > + if (!m_node->parentOrHostNode()) > break; > + m_node = m_node->parentOrHostNode(); I would have used a local variable here, but it’s probably fine not to.

Comment on attachment 77655 [details] Patch Rejecting attachment 77655 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: .................................................................................................................................................................................................................................... editing/spelling ......... editing/spelling/spelling-hasspellingmarker.html -> failed Exiting early after 1 failures. 5409 tests run. 112.57s total testing time 5408 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 3 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7219245

Comment on attachment 77655 [details] Patch Lets try the commit queue again.

Comment on attachment 77655 [details] Patch Rejecting attachment 77655 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: .................................................................................................................................................................................................................................... editing/spelling ......... editing/spelling/spelling-hasspellingmarker.html -> failed Exiting early after 1 failures. 5409 tests run. 112.54s total testing time 5408 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 3 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7284223

It appears this patch causes that test to regress. Sorry the cq isn't smart enough to upload failure diffs yet.

As I noted in #c8, previousWordPosition is also called by removeSpellAndCorrectionMarkersFromWordsToBeEdited, and this patch seems to let spell-check code cross editing boundary, which is obviously not what we want. I wouldn't land this patch unless we investigated the failure and figured out what's the right thing to do.

Comment on attachment 77655 [details] Patch Changing to review- beacuse of the regression test failure.

(In reply to comment #30) > (From update of attachment 77655 [details]) > Changing to review- beacuse of the regression test failure. Thanks Darin and Ryosuke, I'll have a look at it when work starts up again next year.

(In reply to comment #29) > As I noted in #c8, previousWordPosition is also called by removeSpellAndCorrectionMarkersFromWordsToBeEdited, and this patch seems to let spell-check code cross editing boundary, which is obviously not what we want. I wouldn't land this patch unless we investigated the failure and figured out what's the right thing to do. Remember that the editing boundary check removal here is just fixing a regression from a few months ago. I highly doubt that this patch will let spell check code cross editing boundaries. Anyway, I've looked into this some more. The test diff is -PASS hasMarked("<textarea id='test' cols='80' rows='10'></textarea>"); is true -PASS hasMarked("<input id='test' type='text'>"); is true +FAIL hasMarked("<textarea id='test' cols='80' rows='10'></textarea>"); should be true. Was false. +FAIL hasMarked("<input id='test' type='text'>"); should be true. Was false. PASS hasMarked("<div id='test' contenteditable='true'></div>"); is true PASS hasMarked("<div id='test' contentEditable='true' spellcheck='false'></div>"); is false PASS successfullyParsed is true Which is interesting because the spelling markers still work for <div><div contenteditable>zz</div></div> but not <div><textarea>zz</textarea></div> nor <div><input type="text" value="zz"></div> Which makes me think that this is actually a bug caused by the editing boundary being in a shadow dom, most probably triggered by the rightmostEquivalent change from previousBoundary. Perhaps an incorrect use of parentNode() rather than parentOrHostNode()? Or something more subtle?

Actually, I'm wrong, it isn't rightmostEquivalent causing the problem but rather the removal of the editing check, causing the text iterator to apparently exit the shadow node but not reenter it as rightmostEquivalent would do in the <div> case.

Created attachment 78624 [details] Patch with parentOrHostNode changed to parentNode

(In reply to comment #34) > Created an attachment (id=78624) [details] > Patch with parentOrHostNode changed to parentNode I've uploaded a patch which fixes the test case by not allowing SimplifiedBackwardsTextIterator::advance to advance out of the shadow dom. It's not a nice solution, and probably incorrect so not submittable (though the layout tests all still pass). This means I'll probably have to look at an alternative solution to the rightmostEquivalent thing which makes the iterator always stay at the rightmost equivalent rather than not-doing-it-then-correcting-it-afterwards.

Created attachment 79237 [details] Patch

Attachment 79237 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/editing/TextIterator.cpp:1131: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ugh. After far too much time trying to figure this out, finally done.

Created attachment 79238 [details] Ugh. check-webkit-style.

Comment on attachment 79238 [details] Ugh. check-webkit-style. View in context: https://bugs.webkit.org/attachment.cgi?id=79238&action=review > Source/WebCore/editing/visible_units.cpp:130 > - if (it.atEnd() && next == 0) { > + if (it.atEnd() && !next) > pos = it.range()->startPosition(); > - } else if (next != 0) { > + else if (next) { I think it would have been cleaner if you did: if (!next) { if (it.atEnd()) pos = it.range()->startPosition(); return VisiblePosition(pos, DOWNSTREAM); } ...

Comment on attachment 79238 [details] Ugh. check-webkit-style. View in context: https://bugs.webkit.org/attachment.cgi?id=79238&action=review Btw I've found that this is actually failing the test again. Very mysterious since all I did, I think, is sync. Maybe I changed something else too. Looking into it now. >> Source/WebCore/editing/visible_units.cpp:130 >> + else if (next) { > > I think it would have been cleaner if you did: > if (!next) { > if (it.atEnd()) > pos = it.range()->startPosition(); > return VisiblePosition(pos, DOWNSTREAM); > } > ... This particular change is just surrendering to check-webkit-style and its demanding ways. I can change it if you really want though.

(In reply to comment #41) > >> Source/WebCore/editing/visible_units.cpp:130 > >> + else if (next) { > > > > I think it would have been cleaner if you did: > > if (!next) { > > if (it.atEnd()) > > pos = it.range()->startPosition(); > > return VisiblePosition(pos, DOWNSTREAM); > > } > > ... > > This particular change is just surrendering to check-webkit-style and its demanding ways. I can change it if you really want though. You can fix the style first in a separate patch.

Created attachment 79349 [details] Patch

Comment on attachment 79349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79349&action=review > Source/WebCore/editing/Editor.cpp:2091 > - VisibleSelection adjacentWords = VisibleSelection(startOfWord(wordStart, LeftWordIfOnBoundary), endOfWord(wordStart, RightWordIfOnBoundary)); > + VisibleSelection adjacentWords = VisibleSelection(wordStart, endOfWord(wordStart, RightWordIfOnBoundary)); I'm scared of touching this code because it was added by http://trac.webkit.org/changeset/41496 and I can't tell whether automated tests have been added or not. Could you manually verify that spellchecking works with your patch? Having said that, the only caller of this function seems to be TypingCommand::markMisspellingsAfterTyping and it calls startOfWord so this is probably correct change. > Source/WebCore/editing/visible_units.cpp:59 > + Position next = position.next(); > + while (position.node() != next.node()) { > + position = next; > + next = position.next(); > + } > + return position; Could you explain what "rightmost equivalent" mean?

Comment on attachment 79349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79349&action=review >> Source/WebCore/editing/Editor.cpp:2091 >> + VisibleSelection adjacentWords = VisibleSelection(wordStart, endOfWord(wordStart, RightWordIfOnBoundary)); > > I'm scared of touching this code because it was added by http://trac.webkit.org/changeset/41496 and I can't tell whether automated tests have been added or not. Could you manually verify that spellchecking works with your patch? > > Having said that, the only caller of this function seems to be TypingCommand::markMisspellingsAfterTyping and it calls startOfWord so this is probably correct change. Exactly. Not only is the extra startOfWord redundant (based on that single caller*), but it actually results in a bug since apparently startOfWord isn't idempotent if it's on an element boundary. That would be a nice thing to fix actually (unit tests!). There is a test, editing/spelling/spelling-hasspellingmarker. This isn't particularly rigorous (only checks "zz "), perhaps it would be a good idea to augment it with more than just a single word. zzzzzzzz * the fact that the variable is called wordStart implies that there should a precondition (i.e. ASSERT) that wordStart is actually the start of the word. Again -- this would be a good idea, if we had a way of checking that a position is actually the start of a word without "wordStart == startOfWord(wordStart)" which as I said isn't necessarily true due to non-idempotent nature of the function. >> Source/WebCore/editing/visible_units.cpp:59 >> + return position; > > Could you explain what "rightmost equivalent" mean? would rightmostVisuallyEquivalent be a more self-explanatory name?

Comment on attachment 79349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79349&action=review >>> Source/WebCore/editing/Editor.cpp:2091 >>> + VisibleSelection adjacentWords = VisibleSelection(wordStart, endOfWord(wordStart, RightWordIfOnBoundary)); >> >> I'm scared of touching this code because it was added by http://trac.webkit.org/changeset/41496 and I can't tell whether automated tests have been added or not. Could you manually verify that spellchecking works with your patch? >> >> Having said that, the only caller of this function seems to be TypingCommand::markMisspellingsAfterTyping and it calls startOfWord so this is probably correct change. > > Exactly. Not only is the extra startOfWord redundant (based on that single caller*), but it actually results in a bug since apparently startOfWord isn't idempotent if it's on an element boundary. That would be a nice thing to fix actually (unit tests!). > > There is a test, editing/spelling/spelling-hasspellingmarker. This isn't particularly rigorous (only checks "zz "), perhaps it would be a good idea to augment it with more than just a single word. zzzzzzzz > > * the fact that the variable is called wordStart implies that there should a precondition (i.e. ASSERT) that wordStart is actually the start of the word. Again -- this would be a good idea, if we had a way of checking that a position is actually the start of a word without "wordStart == startOfWord(wordStart)" which as I said isn't necessarily true due to non-idempotent nature of the function. Can we assert that ASSERT(wordStart == startOfWord(endOfWord(wordStart)) ? But I'm fine without this assertion because the argument name makes it pretty obvious. >>> Source/WebCore/editing/visible_units.cpp:59 >>> + return position; >> >> Could you explain what "rightmost equivalent" mean? > > would rightmostVisuallyEquivalent be a more self-explanatory name? What do you mean by "rightmost"? We could be either traversing the tree upwards or downwards. What kind of behavior are you expecting here?

Comment on attachment 79349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79349&action=review >>>> Source/WebCore/editing/Editor.cpp:2091 >>>> + VisibleSelection adjacentWords = VisibleSelection(wordStart, endOfWord(wordStart, RightWordIfOnBoundary)); >>> >>> I'm scared of touching this code because it was added by http://trac.webkit.org/changeset/41496 and I can't tell whether automated tests have been added or not. Could you manually verify that spellchecking works with your patch? >>> >>> Having said that, the only caller of this function seems to be TypingCommand::markMisspellingsAfterTyping and it calls startOfWord so this is probably correct change. >> >> Exactly. Not only is the extra startOfWord redundant (based on that single caller*), but it actually results in a bug since apparently startOfWord isn't idempotent if it's on an element boundary. That would be a nice thing to fix actually (unit tests!). >> >> There is a test, editing/spelling/spelling-hasspellingmarker. This isn't particularly rigorous (only checks "zz "), perhaps it would be a good idea to augment it with more than just a single word. zzzzzzzz >> >> * the fact that the variable is called wordStart implies that there should a precondition (i.e. ASSERT) that wordStart is actually the start of the word. Again -- this would be a good idea, if we had a way of checking that a position is actually the start of a word without "wordStart == startOfWord(wordStart)" which as I said isn't necessarily true due to non-idempotent nature of the function. > > Can we assert that ASSERT(wordStart == startOfWord(endOfWord(wordStart)) ? But I'm fine without this assertion because the argument name makes it pretty obvious. Yeah this is a pretty confusing assertion. >>>> Source/WebCore/editing/visible_units.cpp:59 >>>> + return position; >>> >>> Could you explain what "rightmost equivalent" mean? >> >> would rightmostVisuallyEquivalent be a more self-explanatory name? > > What do you mean by "rightmost"? We could be either traversing the tree upwards or downwards. What kind of behavior are you expecting here? Well, it should be agnostic to whether the next() position is upwards or downwards. In the sense that foo <span><span>bar |<b></b></span></span> baz is visually equivalent to, and the rightmost visually equivalent position to, foo <span><span>bar <b></b></span></span>| baz but it traverses up and down. is the correct terminology for this "upstream"? actually this function is possibly incorrect, in that i don't think |<div></div> is visually equivalent to <div></div>| sigh. the purpose is to normalise the position as in the former example.

Comment on attachment 79349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79349&action=review > Source/WebCore/editing/visible_units.cpp:140 > - return VisiblePosition(charIt.range()->endPosition(), DOWNSTREAM); > + // Take the rightmost equivalent position so that the correct containing editable region is used if a boundary is reached. > + return VisiblePosition(rightmostEquivalent(charIt.range()->endPosition()), DOWNSTREAM); Note that a downstream VisiblePosition can be instantiated from a Position implicitly so this can be written as: return rightmostEquivalent(charIt.range()->endPosition()); But per IRC discussion, Kalman is going to experiment with adding a flag to canonicalPosition to ignore editing boundary.

Gah. Why is this so difficult, it's such a trivial concept but I pull one way and somewhere else in the editing code, it pulls the other. Though at least the entire experience is very enlightening. Allowing editing boundaries to be optionally crossed from VisiblePosition's constructor did fix the problem from previousBoundary, but not nextBoundary; the latter causes a number of tests to fail, some due to a trivial difference in the render tree but others less trivial (about 3 tests like this). I think these are due to now-broken assumptions, so easy to fix, but overall this makes me nervous about the change. So in conclusion, to actually fix this bug... option 1: - submit the main part of the patch (reverting that boundary crossing stuff), - change that startOfWord(wordStart) to just wordStart to keep the incremental spell checking working, and then either - go back to the rightmostEquivalent thing, or something similar (i.e. making sure the Position passed to canonicalPosition is sane), or - keep pulling at the code option 2: - go back to something similar to the first patch I uploaded i.e. don't revert editing boundary stuff, but tweak it to be correct (adding code = sad) I might go with option 2 tomorrow since this is taking way too long.

Created attachment 79699 [details] Patch

(In reply to comment #50) > Created an attachment (id=79699) [details] > Patch Ok, much nicer. Looks like there was a subtle bug in SimplifiedBackwardsTextIterator's range checking after all that.

(In reply to comment #49) > Gah. Why is this so difficult, it's such a trivial concept but I pull one way and somewhere else in the editing code, it pulls the other. Though at least the entire experience is very enlightening. > > Allowing editing boundaries to be optionally crossed from VisiblePosition's constructor did fix the problem from previousBoundary, but not nextBoundary; the latter causes a number of tests to fail, some due to a trivial difference in the render tree but others less trivial (about 3 tests like this). I think these are due to now-broken assumptions, so easy to fix, but overall this makes me nervous about the change. > > So in conclusion, to actually fix this bug... > option 1: > - submit the main part of the patch (reverting that boundary crossing stuff), > - change that startOfWord(wordStart) to just wordStart to keep the incremental spell checking working, > and then either > - go back to the rightmostEquivalent thing, or something similar (i.e. making sure the Position passed to canonicalPosition is sane), or > - keep pulling at the code > > option 2: > - go back to something similar to the first patch I uploaded i.e. don't revert editing boundary stuff, but tweak it to be correct (adding code = sad) > > I might go with option 2 tomorrow since this is taking way too long. (ignore this)

Comment on attachment 79699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79699&action=review The change looks good but I don't know enough about these classes to r+ this patch. > Source/WebCore/editing/TextIterator.cpp:1225 > -bool SimplifiedBackwardsTextIterator::crossesEditingBoundary(Node* node) const > +bool SimplifiedBackwardsTextIterator::advanceToIfNonNullAndWithinRange(Node* next) I'm not sure if advanceToIfNonNullAndWithinRange is the best name here. Maybe moveToNode? moveToNodeWithinRange? setNodeRespectingRange? setNodeIfPossible?

Comment on attachment 79699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79699&action=review >> Source/WebCore/editing/TextIterator.cpp:1225 >> +bool SimplifiedBackwardsTextIterator::advanceToIfNonNullAndWithinRange(Node* next) > > I'm not sure if advanceToIfNonNullAndWithinRange is the best name here. Maybe moveToNode? moveToNodeWithinRange? setNodeRespectingRange? setNodeIfPossible? The notnull part is probably unnecessary, but I used "advance" to mirror the method that called it, and it has more meaning than just "set". Don't feel strongly though.

Created attachment 79884 [details] Change to advanceRespectingRange

Created attachment 80023 [details] nicer factored test

Created attachment 80024 [details] nicer factored test (2)

Created attachment 80273 [details] rebase

(In reply to comment #58) > Created an attachment (id=80273) [details] > rebase Could somebody review this please?

(In reply to comment #59) > (In reply to comment #58) > > Created an attachment (id=80273) [details] [details] > > rebase > > Could somebody review this please? *polite re-ping*.

Created attachment 83122 [details] Patch

Created attachment 83444 [details] Patch

Comment on attachment 83444 [details] Patch >Subversion Revision: 79367 >diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog >index 1f8ef2638cda9a52ecc045b3edc46763f5fd0770..db908ebb1f123f34ceded9b27cf23c657a238f0c 100644 >--- a/LayoutTests/ChangeLog >+++ b/LayoutTests/ChangeLog >@@ -1,3 +1,15 @@ >+2011-02-23 Benjamin Kalman <kalman@chromium.org> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Moving or selecting backwards by words jumps to start of contenteditable region if contenteditable=false span is encountered >+ https://bugs.webkit.org/show_bug.cgi?id=51001 >+ >+ Add regression test. >+ >+ * editing/selection/extend-backward-by-word-over-non-editable-expected.txt: Added. >+ * editing/selection/extend-backward-by-word-over-non-editable.html: Added. >+ > 2011-02-22 Jer Noble <jer.noble@apple.com> > > Unreviewed; neglected to update an expected result after changing some >diff --git a/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable-expected.txt b/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable-expected.txt >new file mode 100644 >index 0000000000000000000000000000000000000000..b52b0a9e799b23ea8d96a171fa59e247a983fe79 >--- /dev/null >+++ b/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable-expected.txt >@@ -0,0 +1,4 @@ >+PASS for LTR >+PASS for RTL >+foo bar baz qux quux >+ש×Ö¼×¨× ×©×Ö¼×¨× ×©×Ö¼×¨× ×©×Ö¼×¨× ×©×Ö¼×¨× >diff --git a/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable.html b/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable.html >new file mode 100644 >index 0000000000000000000000000000000000000000..6135db74fae40f62a34f59d39f29e7a665630d10 >--- /dev/null >+++ b/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable.html >@@ -0,0 +1,39 @@ >+<!DOCTYPE html> >+<pre id="log"></pre> >+ >+<div id="ltrTextContainer" contenteditable>foo bar <span contenteditable=false>baz</span> qux quux</div> >+<div id="rtlTextContainer" contenteditable dir="rtl">&#x05E9;&#x05D5;&#x05BC;&#x05E8;&#x05D4; &#x05E9;&#x05D5;&#x05BC;&#x05E8;&#x05D4; >+<span contenteditable=false>&#x05E9;&#x05D5;&#x05BC;&#x05E8;&#x05D4;</span> >+&#x05E9;&#x05D5;&#x05BC;&#x05E8;&#x05D4; &#x05E9;&#x05D5;&#x05BC;&#x05E8;&#x05D4;</div> >+ >+<script> >+function log(s) { >+ document.getElementById("log").appendChild(document.createTextNode(s + "\n")); >+} >+ >+function placeCursorAfterFirstNoneditableChild(container) { >+ for (var i = 0; i < container.childNodes.length; i++) { >+ var node = container.childNodes[i]; >+ if (node.isContentEditable !== undefined && !node.isContentEditable) { >+ getSelection().setPosition(node.nextSibling, 0); >+ return node; >+ } >+ } >+ throw "Couldn't find noneditable child of " + container.textContent; >+} >+ >+function extendBackwardByWord(container, bidiName) { >+ noneditableChild = placeCursorAfterFirstNoneditableChild(container); >+ getSelection().modify("extend", "backward", "word"); >+ if (getSelection().toString() === noneditableChild.textContent) >+ log("PASS for " + bidiName); >+ else >+ log("FAIL for " + bidiName + ", selection is \"" + getSelection() + "\" but should be \"" + noneditableChild.textContent + "\""); >+} >+ >+extendBackwardByWord(document.getElementById("ltrTextContainer"), "LTR"); >+extendBackwardByWord(document.getElementById("rtlTextContainer"), "RTL"); >+ >+if (window.layoutTestController) >+ layoutTestController.dumpAsText(); >+</script> >diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog >index af6dc60037807c36724ce6df9520f6f847a3de26..7b3ff2e8dbd982a4ef76a540cdc7e43bd03111a4 100644 >--- a/Source/WebCore/ChangeLog >+++ b/Source/WebCore/ChangeLog >@@ -1,3 +1,36 @@ >+2011-02-23 Benjamin Kalman <kalman@chromium.org> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Moving or selecting backwards by words jumps to start of contenteditable region if contenteditable=false span is encountered >+ https://bugs.webkit.org/show_bug.cgi?id=51001 >+ >+ Test: editing/selection/extend-backward-by-word-over-non-editable.html >+ >+ Revert some previous changes (the TextIteratorEndsAtEditingBoundary text iteration behaviour) which caused this >+ bug in the first place, and fix SimplifiedBackwardsTextIterator's iteration range check as an alternative fix. >+ >+ The original bug was that double-clicking on an inline editable span at the start of a paragraph would clear the >+ selection (webkit.org/b/36360). This was caused by upstream/downstream VisbiblePosition complications. To fix, >+ refuse to iterate beyond the start node (rather than refusing to iterate across editable boundaries, which >+ causes this bug). >+ >+ To see why this is correct, and to make it clearer that is indeed what is happening, the text iterator code has >+ been slightly restructured to express the invariant that the iterator will never advance past the start node. >+ >+ * editing/TextIterator.cpp: >+ (WebCore::TextIterator::TextIterator): Remove references to TextIterationEndsAtEditing boundary. >+ (WebCore::SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator): Remove m_pastStartNode as the >+ mechanism for iteration range checking, and use a flag m_havePassedStartNode instead. >+ (WebCore::SimplifiedBackwardsTextIterator::advance): Clean up, use advanceRespectingRange and >+ m_havePassedStartNode for iteration range checking rather than m_pastStartNode. >+ (WebCore::SimplifiedBackwardsTextIterator::advanceRespectingRange): The new way of modifying m_node >+ which updates m_havePassedStartNode and refuses to continue when it becomes true. >+ * editing/TextIterator.h: Remove TextIteratorEndsAtEditingBoundary, update for new/removed prototypes and >+ member variables. >+ * editing/visible_units.cpp: >+ (WebCore::previousBoundary): Remove references to TextIteratorEndsAtEditingBoundary. >+ > 2011-02-22 Geoffrey Garen <ggaren@apple.com> > > Reviewed by Oliver Hunt. >diff --git a/Source/WebCore/editing/TextIterator.cpp b/Source/WebCore/editing/TextIterator.cpp >index d3a65964f7e07286ecb273764324d56f1a5c8125..3b18127552af33b6358e150dc660f3412f8b0dc2 100644 >--- a/Source/WebCore/editing/TextIterator.cpp >+++ b/Source/WebCore/editing/TextIterator.cpp >@@ -194,21 +194,6 @@ static Node* nextInPreOrderCrossingShadowBoundaries(Node* rangeEndContainer, int > return 0; > } > >-static Node* previousInPostOrderCrossingShadowBoundaries(Node* rangeStartContainer, int rangeStartOffset) >-{ >- if (!rangeStartContainer) >- return 0; >- if (rangeStartOffset > 0 && !rangeStartContainer->offsetInCharacters()) { >- if (Node* previous = rangeStartContainer->childNode(rangeStartOffset - 1)) >- return previous; >- } >- for (Node* node = rangeStartContainer; node; node = node->parentOrHostNode()) { >- if (Node* previous = node->previousSibling()) >- return previous; >- } >- return 0; >-} >- > // -------- > > static inline bool fullyClipsContents(Node* node) >@@ -290,9 +275,6 @@ TextIterator::TextIterator(const Range* r, TextIteratorBehavior behavior) > , m_handledFirstLetter(false) > , m_ignoresStyleVisibility(behavior & TextIteratorIgnoresStyleVisibility) > { >- // FIXME: should support TextIteratorEndsAtEditingBoundary http://webkit.org/b/43609 >- ASSERT(behavior != TextIteratorEndsAtEditingBoundary); >- > if (!r) > return; > >@@ -1052,7 +1034,7 @@ SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator(const Range* r, > , m_node(0) > , m_positionNode(0) > { >- ASSERT(m_behavior == TextIteratorDefaultBehavior || m_behavior == TextIteratorEndsAtEditingBoundary); >+ ASSERT(m_behavior == TextIteratorDefaultBehavior); > > if (!r) > return; >@@ -1077,7 +1059,7 @@ SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator(const Range* r, > } > } > >- setCurrentNode(endNode); >+ m_node = endNode; > setUpFullyClippedStack(m_fullyClippedStack, m_node); > m_offset = endOffset; > m_handledNode = false; >@@ -1096,7 +1078,7 @@ SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator(const Range* r, > m_lastTextNode = 0; > m_lastCharacter = '\n'; > >- m_pastStartNode = previousInPostOrderCrossingShadowBoundaries(startNode, startOffset); >+ m_havePassedStartNode = false; > > advance(); > } >@@ -1108,7 +1090,7 @@ void SimplifiedBackwardsTextIterator::advance() > m_positionNode = 0; > m_textLength = 0; > >- while (m_node && m_node != m_pastStartNode) { >+ while (m_node && !m_havePassedStartNode) { > // Don't handle node if we start iterating at [node, 0]. > if (!m_handledNode && !(m_node == m_endNode && m_endOffset == 0)) { > RenderObject* renderer = m_node->renderer(); >@@ -1125,8 +1107,10 @@ void SimplifiedBackwardsTextIterator::advance() > return; > } > >- Node* next = m_handledChildren ? 0 : m_node->lastChild(); >- if (!next) { >+ if (!m_handledChildren && m_node->hasChildNodes()) { >+ m_node = m_node->lastChild(); >+ pushFullyClippedState(m_fullyClippedStack, m_node); >+ } else { > // Exit empty containers as we pass over them or containers > // where [container, 0] is where we started iterating. > if (!m_handledNode >@@ -1138,11 +1122,12 @@ void SimplifiedBackwardsTextIterator::advance() > m_handledNode = true; > m_handledChildren = true; > return; >- } >+ } > } >+ > // Exit all other containers. > while (!m_node->previousSibling()) { >- if (!setCurrentNode(m_node->parentOrHostNode())) >+ if (!advanceRespectingRange(m_node->parentOrHostNode())) > break; > m_fullyClippedStack.pop(); > exitNode(); >@@ -1153,14 +1138,12 @@ void SimplifiedBackwardsTextIterator::advance() > } > } > >- next = m_node->previousSibling(); > m_fullyClippedStack.pop(); >+ if (advanceRespectingRange(m_node->previousSibling())) >+ pushFullyClippedState(m_fullyClippedStack, m_node); >+ else >+ m_node = 0; > } >- >- if (m_node && setCurrentNode(next)) >- pushFullyClippedState(m_fullyClippedStack, m_node); >- else >- clearCurrentNode(); > > // For the purpose of word boundary detection, > // we should iterate all visible text and trailing (collapsed) whitespaces. >@@ -1240,26 +1223,17 @@ void SimplifiedBackwardsTextIterator::emitCharacter(UChar c, Node* node, int sta > m_lastCharacter = c; > } > >-bool SimplifiedBackwardsTextIterator::crossesEditingBoundary(Node* node) const >+bool SimplifiedBackwardsTextIterator::advanceRespectingRange(Node* next) > { >- return m_node && m_node->isContentEditable() != node->isContentEditable(); >-} >- >-bool SimplifiedBackwardsTextIterator::setCurrentNode(Node* node) >-{ >- if (!node) >+ if (!next) > return false; >- if (m_behavior == TextIteratorEndsAtEditingBoundary && crossesEditingBoundary(node)) >+ m_havePassedStartNode |= m_node == m_startNode; >+ if (m_havePassedStartNode) > return false; >- m_node = node; >+ m_node = next; > return true; > } > >-void SimplifiedBackwardsTextIterator::clearCurrentNode() >-{ >- m_node = 0; >-} >- > PassRefPtr<Range> SimplifiedBackwardsTextIterator::range() const > { > if (m_positionNode) >diff --git a/Source/WebCore/editing/TextIterator.h b/Source/WebCore/editing/TextIterator.h >index 8b61afe211b4d37b74f9f4a58237e1d28f2dcfb4..b0c310a6fc7e8011b4a44c2afc1cda02dd4249d3 100644 >--- a/Source/WebCore/editing/TextIterator.h >+++ b/Source/WebCore/editing/TextIterator.h >@@ -41,8 +41,7 @@ enum TextIteratorBehavior { > TextIteratorEmitsCharactersBetweenAllVisiblePositions = 1 << 0, > TextIteratorEntersTextControls = 1 << 1, > TextIteratorEmitsTextsWithoutTranscoding = 1 << 2, >- TextIteratorEndsAtEditingBoundary = 1 << 3, >- TextIteratorIgnoresStyleVisibility = 1 << 4 >+ TextIteratorIgnoresStyleVisibility = 1 << 3 > }; > > // FIXME: Can't really answer this question correctly without knowing the white-space mode. >@@ -205,9 +204,7 @@ private: > bool handleReplacedElement(); > bool handleNonTextNode(); > void emitCharacter(UChar, Node*, int startOffset, int endOffset); >- bool crossesEditingBoundary(Node*) const; >- bool setCurrentNode(Node*); >- void clearCurrentNode(); >+ bool advanceRespectingRange(Node*); > > TextIteratorBehavior m_behavior; > // Current position, not necessarily of the text being returned, but position >@@ -238,9 +235,9 @@ private: > > // Used for whitespace characters that aren't in the DOM, so we can point at them. > UChar m_singleCharacterBuffer; >- >- // The node after the last node this iterator should process. >- Node* m_pastStartNode; >+ >+ // Whether m_node has advanced beyond the iteration range (i.e. m_startNode). >+ bool m_havePassedStartNode; > }; > > // Builds on the text iterator, adding a character position so we can walk one >diff --git a/Source/WebCore/editing/visible_units.cpp b/Source/WebCore/editing/visible_units.cpp >index 281bc41c4b7b6383261418a4ac9393bd10a42da5..9cb4d6d0516b4ee24d60e12b41560f7266a938e8 100644 >--- a/Source/WebCore/editing/visible_units.cpp >+++ b/Source/WebCore/editing/visible_units.cpp >@@ -89,7 +89,7 @@ static VisiblePosition previousBoundary(const VisiblePosition& c, BoundarySearch > if (ec) > return VisiblePosition(); > >- SimplifiedBackwardsTextIterator it(searchRange.get(), TextIteratorEndsAtEditingBoundary); >+ SimplifiedBackwardsTextIterator it(searchRange.get()); > unsigned next = 0; > bool inTextSecurityMode = start.deprecatedNode() && start.deprecatedNode()->renderer() && start.deprecatedNode()->renderer()->style()->textSecurity() != TSNONE; > bool needMoreContext = false; >@@ -124,7 +124,7 @@ static VisiblePosition previousBoundary(const VisiblePosition& c, BoundarySearch > return VisiblePosition(Position(node, next), DOWNSTREAM); > > // Use the character iterator to translate the next value into a DOM position. >- BackwardsCharacterIterator charIt(searchRange.get(), TextIteratorEndsAtEditingBoundary); >+ BackwardsCharacterIterator charIt(searchRange.get()); > charIt.advance(string.size() - suffixLength - next); > return VisiblePosition(charIt.range()->endPosition(), DOWNSTREAM); > }

oops

Created attachment 83447 [details] Patch with correct svn version

Created attachment 83450 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 83450 [details]: fast/css/font-face-download-error.html bug 51757 (author: yuzo@google.com) The commit-queue is continuing to process your patch.

Comment on attachment 83450 [details] Patch for landing Clearing flags on attachment: 83450 Committed r79429: <http://trac.webkit.org/changeset/79429>

All reviewed patches have been landed. Closing bug.