Summary: | Div having contentEditable and display:flex cannot be edited if it is empty. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | ChangSeok Oh <changseok> | ||||||||||||
Component: | HTML Editing | Assignee: | ChangSeok Oh <changseok> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, darin, enrica, esprehn+autocc, hyatt, kangil.han, kling, koivisto, ossy, rego, rniwa | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | All | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=226926 | ||||||||||||||
Bug Depends on: | 141473 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
ChangSeok Oh
2015-02-03 16:32:28 PST
Created attachment 246008 [details]
Patch
Created attachment 246016 [details]
Patch
Created attachment 246017 [details]
Patch
Comment on attachment 246017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246017&action=review > LayoutTests/fast/events/key-events-in-editable-flexbox.html:1 > +<html> Missing DOCTYPE. > LayoutTests/fast/events/key-events-in-editable-flexbox.html:24 > + var onMacPlatform = navigator.userAgent.search(/\bMac OS X\b/) != -1; > + if (onMacPlatform) > + eventSender.keyDown("rightArrow", ["metaKey"]); > + else > + eventSender.keyDown("end"); Use getSelection().modify('move', 'forward', 'lineboundary') instead. > LayoutTests/fast/events/key-events-in-editable-flexbox.html:29 > + eventSender.keyDown('E'); > + eventSender.keyDown('S'); > + eventSender.keyDown('T'); > + shouldBeEqualToString("targetDiv.innerText", "TEST"); Use document.execCommand('insertText') instead. > LayoutTests/fast/events/key-events-in-editable-flexbox.html:36 > + eventSender.keyDown('\u0008'); > + eventSender.keyDown('\u0008'); > + eventSender.keyDown('\u0008'); > + eventSender.keyDown('\u0008'); > + eventSender.keyDown('\u0008'); // Remove '\n' > + shouldBeEmptyString("targetDiv.innerText"); Use document.execCommand('delete') instead. > LayoutTests/fast/events/key-events-in-editable-flexbox.html:42 > + eventSender.keyDown('T'); > + eventSender.keyDown('E'); > + eventSender.keyDown('S'); > + eventSender.keyDown('T'); > + shouldBeEqualToString("targetDiv.innerText", "TEST"); Ditto. > LayoutTests/fast/events/key-events-in-editable-flexbox.html:50 > + <body onload="test()"> > + <div id="target" contentEditable>T</div> > + </body> I don't think we normally indent test contents. Also your indentation is inconsistent. Sometimes it's 4 spaces and sometimes 2 spaces. Please at least be consistent. Comment on attachment 246017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246017&action=review > Source/WebCore/ChangeLog:13 > + having âdisplay : flexâ goes weird and VisibleSelection is empty accordingly. The quote marks here do not seem to render in the review tool. Please use these ASCII marks "" Comment on attachment 246017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246017&action=review > Source/WebCore/dom/Position.cpp:939 > + if (is<RenderBlockFlow>(*renderer) || is<RenderFlexibleBox>(*renderer)) { I think this is going to be failing for grid too. Maybe you could use here !renderer->isRenderBlockFlow() to cover both cases. Also adding a test case for grid would be nice. :-) (In reply to comment #6) > Comment on attachment 246017 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246017&action=review > > > Source/WebCore/dom/Position.cpp:939 > > + if (is<RenderBlockFlow>(*renderer) || is<RenderFlexibleBox>(*renderer)) { > > I think this is going to be failing for grid too. Maybe you could use here > !renderer->isRenderBlockFlow() to cover both cases. Mmmm, I've just realized that this won't make any sense. I'm wondering if we should just add is<RenderGrid> or if this "if" is actually needed for something else. Anyway, this is failing in grid too. Created attachment 246378 [details]
Patch
Comment on attachment 246017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246017&action=review Thanks for all comments. >> Source/WebCore/ChangeLog:13 >> + having âdisplay : flexâ goes weird and VisibleSelection is empty accordingly. > > The quote marks here do not seem to render in the review tool. Please use these ASCII marks "" Done. >>> Source/WebCore/dom/Position.cpp:939 >>> + if (is<RenderBlockFlow>(*renderer) || is<RenderFlexibleBox>(*renderer)) { >> >> I think this is going to be failing for grid too. Maybe you could use here !renderer->isRenderBlockFlow() to cover both cases. >> >> Also adding a test case for grid would be nice. :-) > > Mmmm, I've just realized that this won't make any sense. I'm wondering if we should just add is<RenderGrid> or if this "if" is actually needed for something else. > > Anyway, this is failing in grid too. Hrm.. It sounds getting complicated. Let's cover the grid issue in a new bug. I opened http://webkit.org/b/141465 >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:1 >> +<html> > > Missing DOCTYPE. Done >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:24 >> + eventSender.keyDown("end"); > > Use getSelection().modify('move', 'forward', 'lineboundary') instead. Nice improvement. Good to know. thanks. =) >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:29 >> + shouldBeEqualToString("targetDiv.innerText", "TEST"); > > Use document.execCommand('insertText') instead. ditto >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:36 >> + shouldBeEmptyString("targetDiv.innerText"); > > Use document.execCommand('delete') instead. ditto. >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:42 >> + shouldBeEqualToString("targetDiv.innerText", "TEST"); > > Ditto. O.K >> LayoutTests/fast/events/key-events-in-editable-flexbox.html:50 >> + </body> > > I don't think we normally indent test contents. Also your indentation is inconsistent. > Sometimes it's 4 spaces and sometimes 2 spaces. Please at least be consistent. Done. Comment on attachment 246378 [details]
Patch
Got r+ from rniwa, so I land the patch.
Comment on attachment 246378 [details] Patch Clearing flags on attachment: 246378 Committed r179921: <http://trac.webkit.org/changeset/179921> All reviewed patches have been landed. Closing bug. (In reply to comment #11) > Comment on attachment 246378 [details] > Patch > > Clearing flags on attachment: 246378 > > Committed r179921: <http://trac.webkit.org/changeset/179921> It made tests crash/assert on the debug bots. Re-opened since this is blocked by bug 141473 Created attachment 246398 [details]
Patch
(In reply to comment #13) > (In reply to comment #11) > > Comment on attachment 246378 [details] > > Patch > > > > Clearing flags on attachment: 246378 > > > > Committed r179921: <http://trac.webkit.org/changeset/179921> > > It made tests crash/assert on the debug bots. Wrong downcasting caused the assertion failure. Updated patch should fix it. Comment on attachment 246398 [details] Patch Clearing flags on attachment: 246398 Committed r179944: <http://trac.webkit.org/changeset/179944> All reviewed patches have been landed. Closing bug. |