This will fix a similar issue with Bug 141218, the only difference from 141218 is that a div has display:grid.
Manuel Rego Casasnovas said : >> 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.
Just to give you more information. There're some of these bugs that are both in flex and grid, mostly because of they're not RenderBlockFlow. In some cases, we need some special methods to check that they're not RenderBlockFlow but they're not buttons either (as RenderButton, like RenderFullScreen) inherit from RenderFlexibleBox. For example take a look to this method: http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlock.cpp#L2927
(In reply to comment #2) > Just to give you more information. There're some of these bugs that are both > in flex and grid, mostly because of they're not RenderBlockFlow. > > In some cases, we need some special methods to check that they're not > RenderBlockFlow but they're not buttons either (as RenderButton, like > RenderFullScreen) inherit from RenderFlexibleBox. > > For example take a look to this method: > http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlock. > cpp#L2927 Thanks for the comment. But I'm not sure if I understand your intention correctly. Why do we need to check that? As my understanding any element can be editable with the contenteditable attribute. https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_Editable Actually I checked RenderFullScreen is allowed to edit in firefox. Could you provide me a sample which should not be allowed to edit even though it has a contenteditable attribute? I think just adding is<RenderGrid>(*renderer) is enough there? (Of course more touches are required to make grid work properly for this case.)
Created attachment 246469 [details] Patch
(In reply to comment #3) > (In reply to comment #2) > > Just to give you more information. There're some of these bugs that are both > > in flex and grid, mostly because of they're not RenderBlockFlow. > > > > In some cases, we need some special methods to check that they're not > > RenderBlockFlow but they're not buttons either (as RenderButton, like > > RenderFullScreen) inherit from RenderFlexibleBox. > > > > For example take a look to this method: > > http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlock. > > cpp#L2927 > > Thanks for the comment. But I'm not sure if I understand your intention > correctly. Why do we need to check that? As my understanding any element can > be editable with the contenteditable attribute. > https://developer.mozilla.org/en-US/docs/Web/Guide/HTML/Content_Editable > Actually I checked RenderFullScreen is allowed to edit in firefox. > Could you provide me a sample which should not be allowed to edit even > though it has a contenteditable attribute? > I think just adding is<RenderGrid>(*renderer) is enough there? (Of course > more touches are required to make grid work properly for this case.) Sorry for the misleading comment. I was not thinking about editable or not, just about how we usually detect in other parts of the code that a renderer is a flex or a grid. We usually use that kind of methods, as flex and grid don't inherit from RenderBlockFlow. However, sometimes we need that buttons, full-screen and other that inherit from RenderFlexibleBox behave like a RenderBlockFlow and we've this kind of special method. Anyway you can forget about my comment, sorry for the noise.
Comment on attachment 246469 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246469&action=review > Source/WebCore/ChangeLog:8 > + This bug is qutie similar with webkit.org/b/141218. RenderGrid should be also treated as a candidate qutie -> quite similar with -> similar to > Source/WebCore/ChangeLog:11 > + a minimum height of a single line if it is editable as well as RenderFlexibleBox does. Where is this specified? > LayoutTests/fast/events/key-events-in-editable-gridbox.html:7 > + display: grid; No need for the unprefixed version.
Comment on attachment 246469 [details] Patch Setting cq- while we wait for clarifications.
Comment on attachment 246469 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246469&action=review Thanks for the review guys! Let me update the patch soon. =) >> Source/WebCore/ChangeLog:8 >> + This bug is qutie similar with webkit.org/b/141218. RenderGrid should be also treated as a candidate > > qutie -> quite > > similar with -> similar to Oops thanks. >> Source/WebCore/ChangeLog:11 >> + a minimum height of a single line if it is editable as well as RenderFlexibleBox does. > > Where is this specified? http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderFlexibleBox.cpp#L707 >> LayoutTests/fast/events/key-events-in-editable-gridbox.html:7 >> + display: grid; > > No need for the unprefixed version. O.K Removed. BTW doesn't webkit have a plan to remove the --webkit- prefix in the future?
Created attachment 246525 [details] Patch
Comment on attachment 246525 [details] Patch Clearing flags on attachment: 246525 Committed r180050: <http://trac.webkit.org/changeset/180050>
(In reply to comment #10) > Comment on attachment 246525 [details] > Patch > > Clearing flags on attachment: 246525 > > Committed r180050: <http://trac.webkit.org/changeset/180050> It broke the !ENABLE(CSS_GRID_LAYOUT) build - new bug report: bug141647
Closing this as it landed some time ago.