Bug 40753 – Hit testing on margins of body and head elements doesn't recur

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ojan Vafai]

Created attachment 58951 [details]
test case

See the test case. Clicking in white-space when the body element is scrolled puts the cursor in the wrong place.

Chromium bug: http://crbug.com/45151

[Ryosuke Niwa]

It turned out that this bug is nothing to do with scroll.  It's about hit testing code not recursing properly when hit testing is done on margins of body and head elements.

[Ryosuke Niwa]

Created attachment 107943 [details]
fixes the bug

[Darin Adler]

Comment on attachment 107943 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=107943&action=review

> Source/WebCore/rendering/RenderBlock.cpp:4235
> +    if (!ancestor || ancestor->node()->hasTagName(htmlTag) || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())

I’m not sure that htmlTag is the best way to check for the document element. It’s probably neither the most efficient way nor the most reliable. The best way I can think of would be to tie this as closely as possible to the code that makes body and head elements have special margins. I was unable to quickly research that because I don’t know what that code is.

Two other ways that come to mind that may be better:

    ancestor->isRenderView()
    ancestor->node()->parent() && ancestor->node()->parent()->isDocumentNode()

[Ryosuke Niwa]

Comment on attachment 107943 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=107943&action=review

>> Source/WebCore/rendering/RenderBlock.cpp:4235
>> +    if (!ancestor || ancestor->node()->hasTagName(htmlTag) || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())
> 
> I’m not sure that htmlTag is the best way to check for the document element. It’s probably neither the most efficient way nor the most reliable. The best way I can think of would be to tie this as closely as possible to the code that makes body and head elements have special margins. I was unable to quickly research that because I don’t know what that code is.
> 
> Two other ways that come to mind that may be better:
> 
>     ancestor->isRenderView()
>     ancestor->node()->parent() && ancestor->node()->parent()->isDocumentNode()

ancestor->isRenderView() sounds like a better solution. Try me try.

[Ryosuke Niwa]

(In reply to comment #4)
> > Two other ways that come to mind that may be better:
> > 
> >     ancestor->isRenderView()
> >     ancestor->node()->parent() && ancestor->node()->parent()->isDocumentNode()
> 
> ancestor->isRenderView() sounds like a better solution. Try me try.

Actually, isRenderView() doesn't really work because html element's render object is a child of the render layer.  I'll do the following instead:
ancestor->hasLayer() && ancestor->parent()->isRenderView()

[Ryosuke Niwa]

Committed r95509: <http://trac.webkit.org/changeset/95509>

[Philippe Normand]

It seems this patch broke a test in GTK: fast/repaint/japanese-rl-selection-repaint-in-regions.html

http://webkit-bots.igalia.com/amd64/svn_95509.core-when_1316490844-_-who_DumpRenderTree-_-why_11.trace.html

#0  0x00002b5eacd4ebd4 in WebCore::positionForPointRespectingEditingBoundaries (parent=0x54df1988, child=0x54e3f7c8, pointInParentCoordinates=...) at ../../Source/WebCore/rendering/RenderBlock.cpp:4235
4235	    if (!ancestor || (ancestor->hasLayer() && ancestor->parent()->isRenderView()) || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())


Thread 1 (Thread 0x2b5eba203e40 (LWP 20635)):
#0  0x00002b5eacd4ebd4 in WebCore::positionForPointRespectingEditingBoundaries (parent=0x54df1988, child=0x54e3f7c8, pointInParentCoordinates=...) at ../../Source/WebCore/rendering/RenderBlock.cpp:4235
#1  0x00002b5eacd4f44a in WebCore::RenderBlock::positionForPoint (this=0x54df1988, point=...) at ../../Source/WebCore/rendering/RenderBlock.cpp:4345
#2  0x00002b5eacb7b5ff in WebCore::selectionExtentRespectingEditingBoundary (selection=..., localPoint=..., targetNode=0x54a20880) at ../../Source/WebCore/page/EventHandler.cpp:648
#3  0x00002b5eacb7b6ac in WebCore::EventHandler::updateSelectionForMouseDrag (this=0x1630ab0, hitTestResult=...) at ../../Source/WebCore/page/EventHandler.cpp:660
#4  0x00002b5eacb7b154 in WebCore::EventHandler::handleMouseDraggedEvent (this=0x1630ab0, event=...) at ../../Source/WebCore/page/EventHandler.cpp:579
#5  0x00002b5eacb7ee83 in WebCore::EventHandler::handleMouseMoveEvent (this=0x1630ab0, mouseEvent=..., hoveredNode=0x7fffa3242960) at ../../Source/WebCore/page/EventHandler.cpp:1657
#6  0x00002b5eacb7e4fa in WebCore::EventHandler::mouseMoved (this=0x1630ab0, event=...) at ../../Source/WebCore/page/EventHandler.cpp:1527
#7  0x00002b5eac4382b6 in webkit_web_view_motion_event (widget=0x1600030, event=0x4447e620) at ../../Source/WebKit/gtk/webkit/webkitwebview.cpp:848
#8  0x00002b5eaf4c7428 in ?? () from /usr/lib/libgtk-3.so.0
#9  0x00002b5eb0e80e7e in g_closure_invoke (closure=0x158c750, return_value=0x7fffa3242c00, n_param_values=2, param_values=0x2b5efc1b0720, invocation_hint=0x7fffa3242bc0) at /tmp/buildd/glib2.0-2.28.6/./gobject/gclosure.c:767
#10 0x00002b5eb0e926e8 in signal_emit_unlocked_R (node=<optimized out>, detail=0, instance=0x1600030, emission_return=0x7fffa3242d70, instance_and_params=0x2b5efc1b0720) at /tmp/buildd/glib2.0-2.28.6/./gobject/gsignal.c:3290
#11 0x00002b5eb0e9baa5 in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=<optimized out>) at /tmp/buildd/glib2.0-2.28.6/./gobject/gsignal.c:2993
#12 0x00002b5eb0e9bed3 in g_signal_emit (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>) at /tmp/buildd/glib2.0-2.28.6/./gobject/gsignal.c:3040
#13 0x00002b5eaf5e9e59 in ?? () from /usr/lib/libgtk-3.so.0
#14 0x00002b5eaf4c6c8a in gtk_propagate_event () from /usr/lib/libgtk-3.so.0
#15 0x00002b5eaf4c7023 in gtk_main_do_event () from /usr/lib/libgtk-3.so.0
#16 0x0000000000434183 in dispatchEvent (event=0x4447e620) at ../../Tools/DumpRenderTree/gtk/EventSender.cpp:562
#17 0x000000000043431a in replaySavedEvents () at ../../Tools/DumpRenderTree/gtk/EventSender.cpp:605
#18 0x000000000043411d in sendOrQueueEvent (event=0x4447e020, shouldReplaySavedEvents=true) at ../../Tools/DumpRenderTree/gtk/EventSender.cpp:545
#19 0x0000000000433563 in mouseUpCallback (context=0x2b5efbab4100, function=0x2b5efbf0b320, thisObject=0x2b5f05492160, argumentCount=0, arguments=0x7fffa3243078, exception=0x7fffa3243118) at ../../Tools/DumpRenderTree/gtk/EventSender.cpp:372
#20 0x00002b5eab57ca90 in JSC::JSCallbackFunction::call (exec=0x2b5efbab4100) at ../../Source/JavaScriptCore/API/JSCallbackFunction.cpp:72
#21 0x00002b5eab64b06b in JSC::cti_op_call_NotJSFunction (args=0x7fffa3243250) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:2280
#22 0x00002b5eab64656d in JSC::JITThunks::tryCacheGetByID (callFrame=0x2b5efbab40c0, codeBlock=0x7fffa3243250, returnAddress=..., baseValue=..., propertyName=UnicodeEncodeError: 'ascii' codec can't encode characters in position 0-2: ordinal not in range(128)
, slot=..., stubInfo=0x1662ab0) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:942
#23 0x00002b5eab6185eb in JSC::JITCode::execute (this=0x2b5f0ed261b8, registerFile=0x1661f58, callFrame=0x2b5efbab4040, globalData=0x1662ab0) at ../../Source/JavaScriptCore/jit/JITCode.h:103
#24 0x00002b5eab614d37 in JSC::Interpreter::executeCall (this=0x1661f40, callFrame=0x2b5efbec5328, function=0x2b5f05491560, callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:971
#25 0x00002b5eab6b4d58 in JSC::call (exec=0x2b5efbec5328, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../Source/JavaScriptCore/runtime/CallData.cpp:39
#26 0x00002b5eac530f0a in WebCore::JSMainThreadExecState::call (exec=0x2b5efbec5328, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:52
#27 0x00002b5eac562a2e in WebCore::JSEventListener::handleEvent (this=0x54e7a090, scriptExecutionContext=0x54a20a18, event=0x54e11c30) at ../../Source/WebCore/bindings/js/JSEventListener.cpp:129
#28 0x00002b5eac7ab33b in WebCore::EventTarget::fireEventListeners (this=0x54e4e6b0, event=0x54e11c30, d=0x54e4e798, entry=WTF::Vector of length 1, capacity 1 = {...}) at ../../Source/WebCore/dom/EventTarget.cpp:360
#29 0x00002b5eac7ab1d8 in WebCore::EventTarget::fireEventListeners (this=0x54e4e6b0, event=0x54e11c30) at ../../Source/WebCore/dom/EventTarget.cpp:329
#30 0x00002b5eacb6e19e in WebCore::DOMWindow::dispatchEvent (this=0x54e4e6b0, prpEvent=..., prpTarget=...) at ../../Source/WebCore/page/DOMWindow.cpp:1597
#31 0x00002b5eacb6e2b2 in WebCore::DOMWindow::dispatchTimedEvent (this=0x54e4e6b0, event=..., target=0x54a20880, startTime=0x546df2b8, endTime=0x546df2c0) at ../../Source/WebCore/page/DOMWindow.cpp:1609
#32 0x00002b5eacb6deee in WebCore::DOMWindow::dispatchLoadEvent (this=0x54e4e6b0) at ../../Source/WebCore/page/DOMWindow.cpp:1572
#33 0x00002b5eac7547be in WebCore::Document::dispatchWindowLoadEvent (this=0x54a20880) at ../../Source/WebCore/dom/Document.cpp:3469
#34 0x00002b5eac74f7f8 in WebCore::Document::implicitClose (this=0x54a20880) at ../../Source/WebCore/dom/Document.cpp:2183
#35 0x00002b5eacad6551 in WebCore::FrameLoader::checkCallImplicitClose (this=0x1630428) at ../../Source/WebCore/loader/FrameLoader.cpp:795
#36 0x00002b5eacad6324 in WebCore::FrameLoader::checkCompleted (this=0x1630428) at ../../Source/WebCore/loader/FrameLoader.cpp:743
#37 0x00002b5eacad608b in WebCore::FrameLoader::finishedParsing (this=0x1630428) at ../../Source/WebCore/loader/FrameLoader.cpp:677
#38 0x00002b5eac757fea in WebCore::Document::finishedParsing (this=0x54a20880) at ../../Source/WebCore/dom/Document.cpp:4286
#39 0x00002b5eac9b228a in WebCore::HTMLTreeBuilder::finished (this=0x533595a0) at ../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2826
#40 0x00002b5eac987b9c in WebCore::HTMLDocumentParser::end (this=0x12a3c330) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:381
#41 0x00002b5eac987c99 in WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd (this=0x12a3c330) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:390
#42 0x00002b5eac986daf in WebCore::HTMLDocumentParser::prepareToStopParsing (this=0x12a3c330) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:153
#43 0x00002b5eac987cde in WebCore::HTMLDocumentParser::attemptToEnd (this=0x12a3c330) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:402
#44 0x00002b5eac987d97 in WebCore::HTMLDocumentParser::finish (this=0x12a3c330) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:429
#45 0x00002b5eacad10b0 in WebCore::DocumentWriter::endIfNotLoadingMainResource (this=0x546deb10) at ../../Source/WebCore/loader/DocumentWriter.cpp:235
#46 0x00002b5eacad0fc7 in WebCore::DocumentWriter::end (this=0x546deb10) at ../../Source/WebCore/loader/DocumentWriter.cpp:214
#47 0x00002b5eacac4f63 in WebCore::DocumentLoader::finishedLoading (this=0x546de9f0) at ../../Source/WebCore/loader/DocumentLoader.cpp:289
#48 0x00002b5eacadca8b in WebCore::FrameLoader::finishedLoading (this=0x1630428) at ../../Source/WebCore/loader/FrameLoader.cpp:2086
#49 0x00002b5eacb116f8 in WebCore::MainResourceLoader::didFinishLoading (this=0x54de1350, finishTime=0) at ../../Source/WebCore/loader/MainResourceLoader.cpp:485
#50 0x00002b5eacb1e16b in WebCore::ResourceLoader::didFinishLoading (this=0x54de1350, finishTime=0) at ../../Source/WebCore/loader/ResourceLoader.cpp:460
#51 0x00002b5eaccb3aba in WebCore::readCallback (source=0x4c80dea0, asyncResult=0x4c80dd80, data=0x0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:781
#52 0x00002b5eb0b9bb59 in async_ready_callback_wrapper (source_object=0x4c80dea0, res=0x4c80dd80, user_data=0x0) at /tmp/buildd/glib2.0-2.28.6/./gio/ginputstream.c:470
#53 0x00002b5eb0baba68 in complete_in_idle_cb_for_thread (_data=0x533594f0) at /tmp/buildd/glib2.0-2.28.6/./gio/gsimpleasyncresult.c:812
#54 0x00002b5eb17184a3 in g_main_dispatch (context=0x1589e60) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:2440
#55 g_main_context_dispatch (context=0x1589e60) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:3013
#56 0x00002b5eb1718c80 in g_main_context_iterate (context=0x1589e60, block=1, dispatch=1, self=<optimized out>) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:3091
#57 0x00002b5eb17192f2 in g_main_loop_run (loop=0x54dac100) at /tmp/buildd/glib2.0-2.28.6/./glib/gmain.c:3299
#58 0x00002b5eaf4c64cd in gtk_main () from /usr/lib/libgtk-3.so.0
#59 0x000000000042f979 in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:710
#60 0x000000000042efb1 in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:502
#61 0x00000000004312ec in main (argc=2, argv=0x7fffa3244b18) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1205

[Ryosuke Niwa]

(In reply to comment #7)
> It seems this patch broke a test in GTK: fast/repaint/japanese-rl-selection-repaint-in-regions.html
> 
> http://webkit-bots.igalia.com/amd64/svn_95509.core-when_1316490844-_-who_DumpRenderTree-_-why_11.trace.html
> 
> #0  0x00002b5eacd4ebd4 in WebCore::positionForPointRespectingEditingBoundaries (parent=0x54df1988, child=0x54e3f7c8, pointInParentCoordinates=...) at ../../Source/WebCore/rendering/RenderBlock.cpp:4235
> 4235        if (!ancestor || (ancestor->hasLayer() && ancestor->parent()->isRenderView()) || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())

That doesn't make much sense.

I only added "ancestor->hasLayer() && ancestor->parent()->isRenderView()" but ancestor can't be null here or else would have hit !ancestor. And ancestor should have non-null parent because we know it has a node.

[Philippe Normand]

(In reply to comment #8)
> (In reply to comment #7)
> > It seems this patch broke a test in GTK: fast/repaint/japanese-rl-selection-repaint-in-regions.html
> > 
> > http://webkit-bots.igalia.com/amd64/svn_95509.core-when_1316490844-_-who_DumpRenderTree-_-why_11.trace.html
> > 
> > #0  0x00002b5eacd4ebd4 in WebCore::positionForPointRespectingEditingBoundaries (parent=0x54df1988, child=0x54e3f7c8, pointInParentCoordinates=...) at ../../Source/WebCore/rendering/RenderBlock.cpp:4235
> > 4235        if (!ancestor || (ancestor->hasLayer() && ancestor->parent()->isRenderView()) || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())
> 
> That doesn't make much sense.
> 
> I only added "ancestor->hasLayer() && ancestor->parent()->isRenderView()" but ancestor can't be null here or else would have hit !ancestor. And ancestor should have non-null parent because we know it has a node.

Well the m_hasLayer is true and m_parent field of ancestor is null:

(gdb) p ancestor
$1 = (WebCore::RenderObject *) 0x52047a98
(gdb) p *ancestor
$2 = {<WebCore::CachedResourceClient> = {_vptr.CachedResourceClient = 0x2acbf0ae4d70}, m_style = {m_ptr = 0x1085a970}, m_node = 0x52896020, 
  m_parent = 0x0, m_previous = 0x0, m_next = 0x0, m_hasAXObject = true, m_setNeedsLayoutForbidden = false, m_needsLayout = false, 
  m_needsPositionedMovementLayout = false, m_normalChildNeedsLayout = false, m_posChildNeedsLayout = false, 
  m_needsSimplifiedNormalFlowLayout = false, m_preferredLogicalWidthsDirty = true, m_floating = false, m_positioned = false, 
  m_relPositioned = false, m_paintBackground = true, m_isAnonymous = false, m_isText = false, m_isBox = true, m_inline = false, 
  m_replaced = false, m_horizontalWritingMode = false, m_isDragging = false, m_hasLayer = true, m_hasOverflowClip = false, m_hasTransform = false, 
  m_hasReflection = false, m_hasCounterNodeMap = false, m_everHadLayout = true, m_childrenInline = false, m_marginBeforeQuirk = false, 
  m_marginAfterQuirk = false, m_hasMarkupTruncation = false, m_selectionState = 0, m_hasColumns = false, static s_affectsParentBlock = false}

So the crash happens when calling ancestor->parent()->isRenderView(). Would it make sense to check is the ancestor has a parent before this call?

[Ryosuke Niwa]

Reopen the bug. I'll land the patch again with a null check.

[Ryosuke Niwa]

Alright I'm just going to land my original patch.  It seems like a too much trouble for me figure out how to call hasLayer/isRenderView properly.

[Ryosuke Niwa]

Per IRC discussion with philn-tp, we have:
(gdb) p ancestor->showRenderTreeForThis()
* RenderView 0x818de3c                  	#document	0x819b0f0
  RenderBlock 0x81a42ac                	HTML	0x81a1e10 STYLE=border:10px solid maroon; -webkit-writing-mode:vertical-rl
    RenderBody 0x81a4d3c               	BODY	0x81a22c0 STYLE=border:5px solid black;
      RenderRegion 0x81e51a4           	DIV	0x81e4828 STYLE=content:-webkit-from-flow('thread'); position:absolute;right:100px; top:100px; border:2px solid black; width:400px; height:400px
  RenderFlowThread 0x81a506c           	#document	0x819b0f0
    RenderBlock 0x81a5584              	DIV	0x81a4a60 STYLE=-webkit-flow:'thread'
      RenderText 0x814b94c             	#text	0x81a4b10 "\nせっかく見つけたすばらしい記事がどこにあったか忘れてしまった経験はありますかならタイトルとアドレスだけでなく、訪問したウェブページのコンテンツからも検索することができます。せっかく見つけたすばらしい記事がどこにあったか忘れてしまった経験はありますか ならタイトルとアドレスだけでなく、訪問したウェブページのコンテンツからも検索することができます。訪問したウェブページのコンテンツからも検索することができます。せっかく見つけたすばらしい記事がどこにあったか忘れてしまった経験はありますか ならタイトルとアドレスだけでなく、訪問\n"
$1 = void

with child=0x81a42ac (html)

so !ancestor->parent() || (ancestor->hasLayer() && ancestor->parent()->isRenderView()) should work.

[Darin Adler]

(In reply to comment #11)
> Alright I'm just going to land my original patch. It seems like a too much trouble for me figure out how to call hasLayer/isRenderView properly.

Really it would be best if the code was tied to the special margin rules. I’m not sure what code triggers those rules. We can revisit this after you land your code.

Do you know where the margin rules come from? Is it code in RenderView?

[Ryosuke Niwa]

Created attachment 108022 [details]
another attempt

[Ryosuke Niwa]

Hi Darin, could you review this patch again?

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #11)
> > Alright I'm just going to land my original patch. It seems like a too much trouble for me figure out how to call hasLayer/isRenderView properly.
> 
> Really it would be best if the code was tied to the special margin rules. I’m not sure what code triggers those rules. We can revisit this after you land your code.
>
> Do you know where the margin rules come from? Is it code in RenderView?

I don't know what you're referring here.

[Darin Adler]

(In reply to comment #16)
> (In reply to comment #13)
> > Really it would be best if the code was tied to the special margin rules. I’m not sure what code triggers those rules. We can revisit this after you land your code.
> >
> > Do you know where the margin rules come from? Is it code in RenderView?
> 
> I don't know what you're referring here.

I thought you had said at some point that body and head handle margins differently from other elements. Reading back I can’t find the comment that says that. But clearly something makes the handling different. It would be best if this code was tied as closely as possible to that something. Maybe I am just misunderstanding though. It’s happened before!

[Darin Adler]

Comment on attachment 108022 [details]
another attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=108022&action=review

> Source/WebCore/rendering/RenderBlock.cpp:4236
> -    if (!ancestor || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())
> +    if (!ancestor || !ancestor->parent() || (ancestor->hasLayer() && ancestor->parent()->isRenderView())
> +        || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())

Putting this condition in a named function might make clearer what it's doing.

[Ryosuke Niwa]

(In reply to comment #18)
> (From update of attachment 108022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108022&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:4236
> > -    if (!ancestor || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())
> > +    if (!ancestor || !ancestor->parent() || (ancestor->hasLayer() && ancestor->parent()->isRenderView())
> > +        || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())
> 
> Putting this condition in a named function might make clearer what it's doing.

Sure. Does shouldIgnoreEditability sound good to you?

[Darin Adler]

Comment on attachment 108022 [details]
another attempt

View in context: https://bugs.webkit.org/attachment.cgi?id=108022&action=review

>>> Source/WebCore/rendering/RenderBlock.cpp:4236
>>> +        || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())
>> 
>> Putting this condition in a named function might make clearer what it's doing.
> 
> Sure. Does shouldIgnoreEditability sound good to you?

I was thinking more:

    if (!isEditabilityBoundary(ancestor, child))

[Ryosuke Niwa]

(In reply to comment #20)
> (From update of attachment 108022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108022&action=review
> 
> >>> Source/WebCore/rendering/RenderBlock.cpp:4236
> >>> +        || ancestor->node()->rendererIsEditable() == childNode->rendererIsEditable())
> >> 
> >> Putting this condition in a named function might make clearer what it's doing.
> > 
> > Sure. Does shouldIgnoreEditability sound good to you?
> 
> I was thinking more:
> 
>     if (!isEditabilityBoundary(ancestor, child))

Will do that then.

[Ryosuke Niwa]

Committed r95574: <http://trac.webkit.org/changeset/95574>