52742 2011-01-19 14:12:19 -0800 Stop instantiating legacy editing positions in FocusController::advanceFocusInDocumentOrder 2011-03-03 16:49:37 -0800 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) All All RESOLVED FIXED P2 Enhancement --- 52099 1 rniwa webkit-unassigned darin justin.garcia kling mario mrobinson pnormand tonikitoo xan.lopez oldest_to_newest 336783 0 rniwa 2011-01-19 14:12:19 -0800 We're currently in the process of removing all instantiations of legacy editing positions. FocusController::advanceFocusInDocumentOrder currently instantiates two legacy editing positions in the line: if (caretBrowsing) { VisibleSelection newSelection(Position(node, 0), Position(node, 0), DOWNSTREAM); if (frame->selection()->shouldChangeSelection(newSelection)) frame->selection()->setSelection(newSelection); } They should be calling one of helper functions in Position.h or htmlediting.h. 336788 1 rniwa 2011-01-19 14:14:42 -0800 I think the right thing to do here is to call firstPositionInOrBeforeNode. This function produces a position [node, 0] when node can have children for editing (div, blockquote, span, etc...) and before node when node can't have children for editing (br, img, etc...). Could someone from GTK comment on whether this is right behavior or not? 337225 2 mario 2011-01-20 03:01:42 -0800 (In reply to comment #1) > I think the right thing to do here is to call firstPositionInOrBeforeNode. > > This function produces a position [node, 0] when node can have children for editing (div, blockquote, span, etc...) and before node when node can't have children for editing (br, img, etc...). > > Could someone from GTK comment on whether this is right behavior or not? I've some doubts whether the right thing to do would be just to call firstPositionInNode(node) instead of your suggestion, but then I've tried it out and run the unit and layout tests and it seems everything is fine with firstPositionInOrBeforeNode(node). So guess you're right and thus that your patch is the right way to go. 337333 3 rniwa 2011-01-20 07:35:04 -0800 (In reply to comment #2) > I've some doubts whether the right thing to do would be just to call firstPositionInNode(node) instead of your suggestion, but then I've tried it out and run the unit and layout tests and it seems everything is fine with firstPositionInOrBeforeNode(node). Calling firstPositionInNode won't make any sense if editing can ignore contents for the node. e.g. if node was br, img, textarea, etc... it doesn't make any sense to have a position inside that node. Is there any guarantee that node is always a container and the contents are not ignored by editing? That's a requirement for us to be calling firstPositionInNode. 337364 4 mario 2011-01-20 08:45:05 -0800 (In reply to comment #3) > (In reply to comment #2) > > I've some doubts whether the right thing to do would be just to call firstPositionInNode(node) instead of your suggestion, but then I've tried it out and run the unit and layout tests and it seems everything is fine with firstPositionInOrBeforeNode(node). > > Calling firstPositionInNode won't make any sense if editing can ignore contents for the node. e.g. if node was br, img, textarea, etc... it doesn't make any sense to have a position inside that node. Is there any guarantee that node is always a container and the contents are not ignored by editing? That's a requirement for us to be calling firstPositionInNode. If I could have any doubt wrt this stuff, now you've completely dissipated them once and for all. Thanks for the explanation, I appreciate that. 361432 5 rniwa 2011-03-03 01:07:08 -0800 Mario, are you working on this bug? 361453 6 mario 2011-03-03 02:09:01 -0800 (In reply to comment #5) > Mario, are you working on this bug? Not at the moment, sorry. I've started to take a look into it but then I got distracted by other bugs and, I've to recognize, I forgot about this one. Anyway, feel free to make the change yourself if you want. As you can read in comment #2 I tried that out some weeks ago and seemed to work fine, while not breaking any test. If not, I guess I could take care of this when I finish my current tasks, but things this month are piling up so quickly I can't tell for sure... Again, sorry for overlooking this. 361456 7 84539 rniwa 2011-03-03 02:19:29 -0800 Created attachment 84539 fixes the bug 361457 8 rniwa 2011-03-03 02:20:31 -0800 Could someone from Qt / GTK ports verify that this patch doesn't cause a regression by running layout tests? 361591 9 pnormand 2011-03-03 07:00:52 -0800 I'll check this on GTK+, sorry for the delay! 361600 10 pnormand 2011-03-03 07:20:29 -0800 I checked the editing, accessibility and unit tests on GTK without spotting a failure 362087 11 rniwa 2011-03-03 16:49:37 -0800 Committed r80298: <http://trac.webkit.org/changeset/80298> 84539 2011-03-03 02:19:29 -0800 2011-03-03 08:16:57 -0800 fixes the bug bug-52742-20110303191928.patch text/plain 1649 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDgwMjE2KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEtMDMtMDMgIFJ5b3N1a2Ug Tml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgU3RvcCBpbnN0YW50aWF0aW5nIGxlZ2FjeSBlZGl0aW5nIHBvc2l0 aW9ucyBpbiBGb2N1c0NvbnRyb2xsZXI6OmFkdmFuY2VGb2N1c0luRG9jdW1lbnRPcmRlcgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTI3NDIKKworICAg ICAgICBVc2UgZmlyc3RQb3NpdGlvbkluT3JCZWZvcmVOb2RlIHRvIGluc3RhbnRpYXRlIGFwcHJv cHJpYXRlIHBvc2l0aW9uLgorCisgICAgICAgICogcGFnZS9Gb2N1c0NvbnRyb2xsZXIuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6Rm9jdXNDb250cm9sbGVyOjphZHZhbmNlRm9jdXNJbkRvY3VtZW50 T3JkZXIpOgorCiAyMDExLTAzLTAzICBNYXJpbyBTYW5jaGV6IFByYWRhICA8bXNhbmNoZXpAaWdh bGlhLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBNYXJ0aW4gUm9iaW5zb24uCkluZGV4OiBT b3VyY2UvV2ViQ29yZS9wYWdlL0ZvY3VzQ29udHJvbGxlci5jcHAKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYkNvcmUvcGFnZS9Gb2N1c0NvbnRyb2xsZXIuY3BwCShyZXZpc2lvbiA4MDIxMykKKysr IFNvdXJjZS9XZWJDb3JlL3BhZ2UvRm9jdXNDb250cm9sbGVyLmNwcAkod29ya2luZyBjb3B5KQpA QCAtNTUsNiArNTUsNyBAQAogI2luY2x1ZGUgIlNldHRpbmdzLmgiCiAjaW5jbHVkZSAiU3BhdGlh bE5hdmlnYXRpb24uaCIKICNpbmNsdWRlICJXaWRnZXQuaCIKKyNpbmNsdWRlICJodG1sZWRpdGlu Zy5oIiAvLyBGb3IgZmlyc3RQb3NpdGlvbkluT3JCZWZvcmVOb2RlCiAKIG5hbWVzcGFjZSBXZWJD b3JlIHsKIApAQCAtMjg3LDcgKzI4OCw4IEBAIGJvb2wgRm9jdXNDb250cm9sbGVyOjphZHZhbmNl Rm9jdXNJbkRvY3UKICAgICAgICAgc2V0Rm9jdXNlZEZyYW1lKG5ld0RvY3VtZW50LT5mcmFtZSgp KTsKIAogICAgIGlmIChjYXJldEJyb3dzaW5nKSB7Ci0gICAgICAgIFZpc2libGVTZWxlY3Rpb24g bmV3U2VsZWN0aW9uKFBvc2l0aW9uKG5vZGUsIDApLCBQb3NpdGlvbihub2RlLCAwKSwgRE9XTlNU UkVBTSk7CisgICAgICAgIFBvc2l0aW9uIHBvc2l0aW9uID0gZmlyc3RQb3NpdGlvbkluT3JCZWZv cmVOb2RlKG5vZGUpOworICAgICAgICBWaXNpYmxlU2VsZWN0aW9uIG5ld1NlbGVjdGlvbihwb3Np dGlvbiwgcG9zaXRpb24sIERPV05TVFJFQU0pOwogICAgICAgICBpZiAoZnJhbWUtPnNlbGVjdGlv bigpLT5zaG91bGRDaGFuZ2VTZWxlY3Rpb24obmV3U2VsZWN0aW9uKSkKICAgICAgICAgICAgIGZy YW1lLT5zZWxlY3Rpb24oKS0+c2V0U2VsZWN0aW9uKG5ld1NlbGVjdGlvbik7CiAgICAgfQo=