89159 2012-06-14 19:45:58 -0700 Selecting from a table and deleting causes an assertion failure 2024-10-22 17:48:40 -0700 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) Unspecified Unspecified NEW P2 Normal --- 54535 1 shinyak webkit-unassigned ahmad.saleem792 darin dominicc enrica hayato morrita rniwa tasak oldest_to_newest 649787 0 147714 shinyak 2012-06-14 19:45:58 -0700 Created attachment 147714 Repro Select from "hoge 2" to its some right area. Then you can see just below of "hoge 2" is selected. Then push JustifyCenter button, then TD 1 and TD 2 will be justified to center, though they are not content editable. Or press 'Delete' key, then assert will be triggered. ASSERTION FAILED: isEditablePosition(*this) Position.cpp::trailingWhitespacePosition 655022 1 shinyak 2012-06-21 16:46:44 -0700 In DeleteSelectionCommand::initializePositionData(), there are a few call of Position::upstream() and Position::downstream(). They break shadow boundaries. Fixing Position::upstream() and Position::downstream() will fix this issue, I believe. I tried fixing them before, but it's harder than I expected... 655793 2 shinyak 2012-06-22 14:45:19 -0700 The sentence "breaking Shadow boundaries" is not correct. I should have said: element having Shadow DOM should be treated as atomic... 657837 3 hayato 2012-06-26 12:17:36 -0700 I've filed a bug for the spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17608 I think hoge1 and hoge2 should be contenteditable in this case. But in current WebKit implementation, we use '-user-modify' style property to judge it is contenteditable or not. So the implementation might be tricky. 'hoge1 and hoge2' should not inherit 'user-modify' in this case because there is a shadow boundary. We use 'composed shadow DOM tree' in resolving style inheritance. So these elements must not be 'contenteditable' in current WebKit's style resolution. But it seems the 'user-modify' property of those elements became READ-WRITE. It's like 'an unexpected pass'. We should investigate further... 658108 4 hayato 2012-06-26 16:47:07 -0700 It seems that the root cause is ComparePositions() in the repro case. Let me explain using the following simplified example. <div id=top contenteditable> // There is no contenteditable ancestor <div>hello</div> <div>world</div> </div> After mouse dragging from [wo|rld] to outside of top, suppose VisibleSelection::validate() is called with the status: start: {anchorNode: text node of [world], anchorType: OffsetInAnchor} end: {anchorNode: #top, anchorType: AfterAnchor} After validate(), a visibleSelection should be: start: {anchorNode: text node of [world], anchorType: OffsetInAnchor} end: {anchorNode: #top, anchorType: AfterChildren} But the actual result is: start: {anchorNode: text node of [world], anchorType: OffsetInAnchor} end: {anchorNode: #top, anchorType: AfterAnchor} Yeah, nothing changes. That should be changed after adjustSelectionToAvoidCrossingEditingBoundaries() is called in validate(). The following code in adjustSelectionToAvoidCrossingEditingBoundaries(): VisiblePosition last = lastEditablePositionBeforePositionInRoot(m_end, baseRoot); returns Position: {anchorNode: #top, anchorType: AfterAnchor} as last. In lastEditablePositionBeforePositionInRoot(m_end, baseRoot), comparePositions is called as follows: if (comparePositions(position, lastPositionInNode(highestRoot)) == 1) with position: anchorNode: {anchorNode: #top, anchorType: AfterAnchor} lastPositionInNode(highestRoot): {anchorNode: #top, anchorType: AfterChildren} Ideally, comparePositions() should return 1 in this case. But that seemed to return non-1 value. If it returns 1, lastPositionInNode(highestRoot) is used as end() and things will work well. 658712 5 shinyak 2012-06-27 10:59:36 -0700 I found that this happend even if Shadow DOM is not attached. It's a real editing problem. 658713 6 shinyak 2012-06-27 11:00:01 -0700 So I remove the dependency from 658715 7 shinyak 2012-06-27 11:00:38 -0700 Oops... So I remove the dependency to 82697. 658736 8 149776 shinyak 2012-06-27 11:33:17 -0700 Created attachment 149776 Repro Removed Shadow DOM from tests. 658812 9 hayato 2012-06-27 13:17:37 -0700 Quick update: Now ComparePositions() were fixed. But it seemed there are other code places we needed to fix after ComparePositions returns correct value. (In reply to comment #8) > Created an attachment (id=149776) [details] > Repro > > Removed Shadow DOM from tests. (In reply to comment #4) > It seems that the root cause is ComparePositions() in the repro case. > > Let me explain using the following simplified example. > > <div id=top contenteditable> // There is no contenteditable ancestor > <div>hello</div> > <div>world</div> > </div> > > After mouse dragging from [wo|rld] to outside of top, suppose VisibleSelection::validate() is called with the status: > > start: {anchorNode: text node of [world], anchorType: OffsetInAnchor} > end: {anchorNode: #top, anchorType: AfterAnchor} > > After validate(), a visibleSelection should be: > > start: {anchorNode: text node of [world], anchorType: OffsetInAnchor} > end: {anchorNode: #top, anchorType: AfterChildren} > > But the actual result is: > > start: {anchorNode: text node of [world], anchorType: OffsetInAnchor} > end: {anchorNode: #top, anchorType: AfterAnchor} > > Yeah, nothing changes. That should be changed after adjustSelectionToAvoidCrossingEditingBoundaries() is called in validate(). > The following code in adjustSelectionToAvoidCrossingEditingBoundaries(): > > VisiblePosition last = lastEditablePositionBeforePositionInRoot(m_end, baseRoot); > > returns Position: {anchorNode: #top, anchorType: AfterAnchor} as last. > > In lastEditablePositionBeforePositionInRoot(m_end, baseRoot), comparePositions is called as follows: > > if (comparePositions(position, lastPositionInNode(highestRoot)) == 1) > > with position: anchorNode: {anchorNode: #top, anchorType: AfterAnchor} > lastPositionInNode(highestRoot): {anchorNode: #top, anchorType: AfterChildren} > > Ideally, comparePositions() should return 1 in this case. But that seemed to return non-1 value. If it returns 1, lastPositionInNode(highestRoot) is used as end() and things will work well. 2069952 10 ahmad.saleem792 2024-10-22 17:48:40 -0700 Blink fix (for similar assertion): https://chromium.googlesource.com/chromium/src/+/308ca1b8a40d58ec8e8c1bf6a0f62a7681c62fea 147714 2012-06-14 19:45:58 -0700 2012-06-27 11:33:17 -0700 Repro editing-with-tables.html text/html 3777 shinyak PCFET0NUWVBFIGh0bWw+CjxodG1sPgo8Ym9keT4KPHNjcmlwdCBzcmM9InJlc291cmNlcy9wb2x5 ZmlsbC5qcyI+PC9zY3JpcHQ+CjxzY3JpcHQgc3JjPSIuLi9yZXNvdXJjZXMvZXZlbnQtc2VuZGVy LXV0aWwuanMiPjwvc2NyaXB0Pgo8c2NyaXB0IHNyYz0iLi4vLi4vanMvcmVzb3VyY2VzL2pzLXRl c3QtcHJlLmpzIj48L3NjcmlwdD4KCjxkaXYgaWQ9ImNvbnRhaW5lciI+CiAgICA8dGFibGUgYm9y ZGVyPSIxIiBpZD0iaG9zdCIgY29udGVudGVkaXRhYmxlPgogICAgICAgIDx0cj48dGQ+aG9nZSAx PC90ZD48L3RyPgogICAgICAgIDx0cj48dGQ+aG9nZSAyPC90ZD48L3RyPgogICAgPC90YWJsZT4K PC9kaXY+Cgo8ZGl2IGNvbnRlbnRlZGl0YWJsZT4KICAgIDx0YWJsZSBib3JkZXI9IjEiIGNvbnRl bnRlZGl0YWJsZT4KICAgICAgICA8dHI+PHRkPmZ1Z2EgMTwvdGQ+PC90cj4KICAgICAgICA8dHI+ PHRkPmZ1Z2EgMjwvdGQ+PC90cj4KICAgIDwvdGFibGU+CiAgICA8cD5UaGlzIGlzIG5vdCBTaGFk b3cgRE9NLjwvcD4KICAgIDxwPlRoaXMgaXMgbm90IFNoYWRvdyBET00uPC9wPgo8L2Rpdj4KCjxo ciAvPgoKPGRpdiBpZD0iYnV0dG9ucyI+PC9kaXY+Cgo8aHIgLz4KCjxwcmUgaWQ9ImNvbnNvbGUi PjwvcHJlPgoKPHNjcmlwdD4KdmFyIGVkaXRpbmdDb21tYW5kcyA9IFsKICAgICAgICAgIkJhY2tD b2xvciIsCiAgICAgICAgICJCYWNrd2FyZERlbGV0ZSIsCiAgICAgICAgICJCb2xkIiwgCiAgICAg ICAgICJDb3B5IiwgCiAgICAgICAgICJDcmVhdGVMaW5rIiwKICAgICAgICAgIkN1dCIsCiAgICAg ICAgICJEZWZhdWx0UGFyYWdyYXBoU2VwYXJhdG9yIiwKICAgICAgICAgIkRlbGV0ZSIsCiAgICAg ICAgICJEZWxldGVCYWNrd2FyZCIsCiAgICAgICAgICJEZWxldGVCYWNrd2FyZEJ5RGVjb21wb3Np bmdQcmV2aW91c0NoYXJhY3RlciIsCiAgICAgICAgICJEZWxldGVGb3J3YXJkIiwKICAgICAgICAg IkRlbGV0ZVRvQmVnaW5uaW5nT2ZMaW5lIiwKICAgICAgICAgIkRlbGV0ZVRvQmVnaW5uaW5nT2ZQ YXJhZ3JhcGgiLAogICAgICAgICAiRGVsZXRlVG9FbmRPZkxpbmUiLAogICAgICAgICAiRGVsZXRl VG9FbmRPZlBhcmFncmFwaCIsCiAgICAgICAgICJEZWxldGVUb01hcmsiLAogICAgICAgICAiRGVs ZXRlV29yZEJhY2t3YXJkIiwKICAgICAgICAgIkRlbGV0ZVdvcmRGb3J3YXJkIiwKICAgICAgICAg IkZpbmRTdHJpbmciLAogICAgICAgICAiRm9udE5hbWUiLAogICAgICAgICAiRm9udFNpemUiLAog ICAgICAgICAiRm9udFNpemVEZWx0YSIsCiAgICAgICAgICJGb3JlQ29sb3IiLAogICAgICAgICAi Rm9ybWF0QmxvY2siLAogICAgICAgICAiRm9yd2FyZERlbGV0ZSIsCiAgICAgICAgICJIaWxpdGVD b2xvciIsCiAgICAgICAgICJJZ25vcmVTcGVsbGluZyIsCiAgICAgICAgICJJbmRlbnQiLAogICAg ICAgICAiSW5zZXJ0QmFja3RhYiIsCiAgICAgICAgICJJbnNlcnRIVE1MIiwKICAgICAgICAgIklu c2VydEhvcml6b250YWxSdWxlIiwKICAgICAgICAgIkluc2VydEltYWdlIiwKICAgICAgICAgIklu c2VydExpbmVCcmVhayIsCiAgICAgICAgICJJbnNlcnROZXdsaW5lIiwKICAgICAgICAgIkluc2Vy dE5ld2xpbmVJblF1b3RlZENvbnRlbnQiLAogICAgICAgICAiSW5zZXJ0T3JkZXJlZExpc3QiLAog ICAgICAgICAiSW5zZXJ0UGFyYWdyYXBoIiwKICAgICAgICAgIkluc2VydFRhYiIsCiAgICAgICAg ICJJbnNlcnRUZXh0IiwKICAgICAgICAgIkluc2VydFVub3JkZXJlZExpc3QiLAogICAgICAgICAi SXRhbGljIiwKICAgICAgICAgIkp1c3RpZnlDZW50ZXIiLAogICAgICAgICAiSnVzdGlmeUZ1bGwi LAogICAgICAgICAiSnVzdGlmeUxlZnQiLAogICAgICAgICAiSnVzdGlmeU5vbmUiLAogICAgICAg ICAiSnVzdGlmeVJpZ2h0IiwKICAgICAgICAgIk91dGRlbnQiLAogICAgICAgICAiUGFzdGUiLAog ICAgICAgICAiUGFzdGVBbmRNYXRjaFN0eWxlIiwKICAgICAgICAgIlBhc3RlQXNQbGFpblRleHQi LAogICAgICAgICAiUmVkbyIsCiAgICAgICAgICJSZW1vdmVGb3JtYXQiLAogICAgICAgICAiU2Ny b2xsUGFnZUJhY2t3YXJkIiwKICAgICAgICAgIlNjcm9sbFBhZ2VGb3J3YXJkIiwKICAgICAgICAg IlNjcm9sbExpbmVVcCIsCiAgICAgICAgICJTY3JvbGxMaW5lRG93biIsCiAgICAgICAgICJTY3Jv bGxUb0JlZ2lubmluZ09mRG9jdW1lbnQiLAogICAgICAgICAiU2Nyb2xsVG9FbmRPZkRvY3VtZW50 IiwKICAgICAgICAgIlNlbGVjdEFsbCIsCiAgICAgICAgICJTZWxlY3RMaW5lIiwKICAgICAgICAg IlNlbGVjdFBhcmFncmFwaCIsCiAgICAgICAgICJTZWxlY3RTZW50ZW5jZSIsCiAgICAgICAgICJT ZWxlY3RUb01hcmsiLAogICAgICAgICAiU2VsZWN0V29yZCIsCiAgICAgICAgICJTZXRNYXJrIiwK ICAgICAgICAgIlN0cmlrZXRocm91Z2giLAogICAgICAgICAiU3R5bGVXaXRoQ1NTIiwKICAgICAg ICAgIlN1YnNjcmlwdCIsCiAgICAgICAgICJTdXBlcnNjcmlwdCIsCiAgICAgICAgICJTd2FwV2l0 aE1hcmsiLAogICAgICAgICAiVG9nZ2xlQm9sZCIsCiAgICAgICAgICJUb2dnbGVJdGFsaWMiLAog ICAgICAgICAiVG9nZ2xlVW5kZXJsaW5lIiwKICAgICAgICAgIlRyYW5zcG9zZSIsCiAgICAgICAg ICJVbmRlcmxpbmUiLAogICAgICAgICAiVW5kbyIsCiAgICAgICAgICJVbmxpbmsiLAogICAgICAg ICAiVW5zY3JpcHQiLAogICAgICAgICAiVW5zZWxlY3QiLAogICAgICAgICAiVXNlQ1NTIiwKICAg ICAgICAgIllhbmsiLAogICAgICAgICAiWWFua0FuZFNlbGVjdCIsCl07CgpmdW5jdGlvbiBjcmVh dGVUUihjb250ZW50KSB7CiAgICB2YXIgdHIgPSBkb2N1bWVudC5jcmVhdGVFbGVtZW50KCd0cicp OwogICAgdmFyIHRkID0gZG9jdW1lbnQuY3JlYXRlRWxlbWVudCgndGQnKTsKICAgIHRyLmFwcGVu ZENoaWxkKHRkKTsKCiAgICB0ZC5pbm5lckhUTUwgPSBjb250ZW50OwogICAgY29uc29sZS5sb2co dHIuaW5uZXJIVE1MKTsKCiAgICByZXR1cm4gdHI7Cn0KCnZhciBzaGFkb3dSb290ID0gbmV3IFdl YktpdFNoYWRvd1Jvb3QoaG9zdCk7CnNoYWRvd1Jvb3QuYXBwZW5kQ2hpbGQoY3JlYXRlVFIoJ1RE IDEnKSk7CnNoYWRvd1Jvb3QuYXBwZW5kQ2hpbGQoZG9jdW1lbnQuY3JlYXRlRWxlbWVudCgnY29u dGVudCcpKTsKc2hhZG93Um9vdC5hcHBlbmRDaGlsZChjcmVhdGVUUignVEQgMicpKTsKCmZvciAo dmFyIGkgPSAwOyBpIDwgZWRpdGluZ0NvbW1hbmRzLmxlbmd0aDsgKytpKSB7CiAgICB2YXIgYnV0 dG9uID0gZG9jdW1lbnQuY3JlYXRlRWxlbWVudCgnaW5wdXQnKTsKICAgIGJ1dHRvbi5zZXRBdHRy aWJ1dGUoJ3R5cGUnLCAnYnV0dG9uJyk7CiAgICBidXR0b24uc2V0QXR0cmlidXRlKCd2YWx1ZScs IGVkaXRpbmdDb21tYW5kc1tpXSk7CiAgICBidXR0b24uYWRkRXZlbnRMaXN0ZW5lcignY2xpY2sn LCAoZnVuY3Rpb24oYnV0dG9uLCBjb21tYW5kKSB7CiAgICAgICAgcmV0dXJuIGZ1bmN0aW9uKCkg eyAgICAgICAgICAgICAgICAgICAgCiAgICAgICAgICAgIGNvbnNvbGUubG9nKGNvbW1hbmQpOwog ICAgICAgICAgICBkb2N1bWVudC5leGVjQ29tbWFuZChjb21tYW5kKTsKICAgICAgICB9OwogICAg fSkoYnV0dG9uLCBlZGl0aW5nQ29tbWFuZHNbaV0pKTsKICAgIGJ1dHRvbnMuYXBwZW5kQ2hpbGQo YnV0dG9uKTsKfQoKdmFyIHN1Y2Nlc3NmdWxseVBhcnNlZCA9IHRydWU7Cjwvc2NyaXB0PgoKPHNj cmlwdCBzcmM9Ii4uLy4uL2pzL3Jlc291cmNlcy9qcy10ZXN0LXBvc3QuanMiPjwvc2NyaXB0Pgo8 L2JvZHk+CjwvaHRtbD4K 149776 2012-06-27 11:33:17 -0700 2012-06-27 11:33:17 -0700 Repro delete-in-table.html text/html 807 shinyak PCFET0NUWVBFIGh0bWw+CjxodG1sPgo8Ym9keT4KCjxwPlNlbGVjdHMgZnJvbSAnc2VsZWN0IGZy b20gaGVyZScgdG8gc29tZXdoZXJlIHJpZ2h0IHRvIHRoZSB0YWJsZSwgYW5kIHByZXNzIGRlbGV0 ZSBrZXkuClRoaXMgc2hvdWxkIG5vdCB0cmlnZ2VyIGFuIGFzc2VydGlvbi48L3A+Cgo8ZGl2IGlk PSJjb250YWluZXIiPgogICAgPHRhYmxlIGJvcmRlcj0iMSIgaWQ9Imhvc3QiIGNvbnRlbnRlZGl0 YWJsZT4KICAgICAgICA8dHI+PHRkPmZvb2JhcjwvdGQ+PC90cj4KICAgICAgICA8dHI+PHRkIGlk PSJzb3VyY2UiPnNlbGVjdCBmcm9tIGhlcmU8L3RkPjwvdHI+CiAgICA8L3RhYmxlPgo8L2Rpdj4K CjxzY3JpcHQ+CmlmICh3aW5kb3cubGF5b3V0VGVzdENvbnRyb2xsZXIpCiAgICBsYXlvdXRUZXN0 Q29udHJvbGxlci5kdW1wQXNUZXh0KCk7CgppZiAod2luZG93LmV2ZW50U2VuZGVyKSB7CiAgICBl dmVudFNlbmRlci5tb3VzZU1vdmVUbyhzb3VyY2Uub2Zmc2V0TGVmdCArIHNvdXJjZS5vZmZzZXRX aWR0aCAvIDIsIHNvdXJjZS5vZmZzZXRUb3AgKyBzb3VyY2Uub2Zmc2V0SGVpZ2h0IC8gMik7CiAg ICBldmVudFNlbmRlci5tb3VzZURvd24oKTsKCiAgICBldmVudFNlbmRlci5tb3VzZU1vdmVUbyhz b3VyY2Uub2Zmc2V0TGVmdCArIHNvdXJjZS5vZmZzZXRXaWR0aCArIDMwLCBzb3VyY2Uub2Zmc2V0 VG9wICsgc291cmNlLm9mZnNldEhlaWdodCAvIDIpOwogICAgZXZlbnRTZW5kZXIubW91c2VVcCgp OwoKICAgIGV2ZW50U2VuZGVyLmtleURvd24oJ2RlbGV0ZScpOwp9Cjwvc2NyaXB0Pgo8L2JvZHk+ CjwvaHRtbD4K