Bug 105545

Summary: RenderBlock and RenderBox hit testing should ignore PseudoElements
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: Layout and RenderingAssignee: Elliott Sprehn <esprehn>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, esprehn, jchaffraix, koivisto, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Mihnea Ovidenie
Reported 2012-12-20 09:35:15 PST
The assertion was added in http://trac.webkit.org/changeset/137336 for WebKitBug https://bugs.webkit.org/show_bug.cgi?id=104462 Load the following HTML snippet in a debug version of WebKit on Mac: <!doctype html> <html> <head> <style> #div1:before { display: inline-block; content: "BBB"; border: 1px solid red; } #div1 { border: 1px solid green; } </style> </head> <body> <div id="div1">AAAA</div> </body> </html> Click inside the red border of the :before element and you get: ASSERTION FAILED: !m_anchorNode || !m_anchorNode->isPseudoElement() /Users/mihnea/WebKit/Source/WebCore/dom/Position.cpp(90) : WebCore::Position::Position(PassRefPtr<WebCore::Node>, WebCore::Position::LegacyEditingOffset) 1 0x10686cdc9 WebCore::Position::Position(WTF::PassRefPtr<WebCore::Node>, WebCore::Position::LegacyEditingOffset) 2 0x10686cc8b WebCore::Position::Position(WTF::PassRefPtr<WebCore::Node>, WebCore::Position::LegacyEditingOffset) 3 0x105616a9e WebCore::createLegacyEditingPosition(WTF::PassRefPtr<WebCore::Node>, int) 4 0x1068c275d WebCore::RenderBlock::positionForBox(WebCore::InlineBox*, bool) const 5 0x1068c2f38 WebCore::RenderBlock::positionForPointWithInlineChildren(WebCore::LayoutPoint const&) 6 0x1068c36b5 WebCore::RenderBlock::positionForPoint(WebCore::LayoutPoint const&) 7 0x1068c32af WebCore::positionForPointRespectingEditingBoundaries(WebCore::RenderBlock*, WebCore::RenderBox*, WebCore::LayoutPoint const&) 8 0x1068c302e WebCore::RenderBlock::positionForPointWithInlineChildren(WebCore::LayoutPoint const&) 9 0x1068c36b5 WebCore::RenderBlock::positionForPoint(WebCore::LayoutPoint const&) 10 0x105c71f9c WebCore::EventHandler::handleMousePressEventSingleClick(WebCore::MouseEventWithHitTestResults const&) 11 0x105c7290b WebCore::EventHandler::handleMousePressEvent(WebCore::MouseEventWithHitTestResults const&) 12 0x105c773c5 WebCore::EventHandler::handleMousePressEvent(WebCore::PlatformMouseEvent const&) 13 0x103525e30 WebKit::handleMouseEvent(WebKit::WebMouseEvent const&, WebKit::WebPage*, bool) 14 0x103525cf5 WebKit::WebPage::mouseEvent(WebKit::WebMouseEvent const&) 15 0x103566a97 void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WebKit::WebMouseEvent const&), WebKit::WebMouseEvent>(CoreIPC::Arguments1<WebKit::WebMouseEvent> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebKit::WebMouseEvent const&)) 16 0x103559e65 void CoreIPC::handleMessage<Messages::WebPage::MouseEvent, WebKit::WebPage, void (WebKit::WebPage::*)(WebKit::WebMouseEvent const&)>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebKit::WebMouseEvent const&)) 17 0x103554a62 WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&) 18 0x103529abd WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&) 19 0x103529b0d non-virtual thunk to WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&) 20 0x10369ab8a CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&) 21 0x1035ff179 WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&) 22 0x1032fbc48 CoreIPC::Connection::dispatchMessage(CoreIPC::MessageID, CoreIPC::MessageDecoder&) 23 0x1032f910e CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::MessageDecoder>&) 24 0x1032fbbeb CoreIPC::Connection::dispatchOneMessage() 25 0x103303482 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*) 26 0x103303405 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()() 27 0x106bc9529 WTF::Function<void ()>::operator()() const 28 0x106bc916f WebCore::RunLoop::performWork() 29 0x106bca67e WebCore::RunLoop::performWork(void*) 30 0x7fff886a0101 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 31 0x7fff8869fa25 __CFRunLoopDoSources0
Attachments
Patch (8.29 KB, patch)
2012-12-20 14:28 PST, Elliott Sprehn 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Elliott Sprehn
Comment 1 2012-12-20 14:28:14 PST
Created attachment 180411 [details] Patch
Eric Seidel (no email)
Comment 2 2012-12-20 14:31:55 PST
I'm curious what our behavior was prior to your grand patch?
Elliott Sprehn
Comment 3 2012-12-20 14:44:52 PST
(In reply to comment #2) > I'm curious what our behavior was prior to your grand patch? Good question! With the old generated content impl node() would return null for generated content nodes since they were anonymous. I've replicated that behavior with nonPseudoNode() which also returns null when the renderer is for a pseudo element. This patch switches to use nonPseudoNode() bringing back the old behavior from before my patch where editing thinks pseudo element renderers have no associated node. I specifically added the assert to catch places where I may have missed us assuming generated content was anonymous... which Mihnea has exposed one of.
Elliott Sprehn
Comment 4 2012-12-20 14:45:52 PST
(In reply to comment #3) > (In reply to comment #2) > > I'm curious what our behavior was prior to your grand patch? > > Good question! With the old generated content impl node() would return null for generated content nodes _generated content renderers_, they didn't have their own node before.
Eric Seidel (no email)
Comment 5 2012-12-20 14:59:50 PST
Comment on attachment 180411 [details] Patch In some of these we're just using nonPseudoNode as a bool instead of a pointer. Is there a nicer function for htat?
Elliott Sprehn
Comment 6 2012-12-20 15:11:10 PST
(In reply to comment #5) > (From update of attachment 180411 [details]) > In some of these we're just using nonPseudoNode as a bool instead of a pointer. Is there a nicer function for htat? Hmm, not that exists yet. You're asking the question: "Is this renderer not anonymous AND also not a pseudo element" which editing code just did if (renderer->node()) before. We could have isAnonymousOrPseudoElement() { return isAnonymous() || isPseudoElement(); } if you think that's more clear. It seemed less error prone to make all editing code use nonPseudoNode() and not worry about having isAnonymousOrPseudoElement() checks all over. I suppose we could rename nonPseudoNode to nodeForEditing() too.
Eric Seidel (no email)
Comment 7 2012-12-20 16:09:07 PST
Comment on attachment 180411 [details] Patch This seems OK. It's a bit unclear to me when to use nonPseudoNode and when to use node() however.
Elliott Sprehn
Comment 8 2012-12-20 16:12:04 PST
(In reply to comment #7) > (From update of attachment 180411 [details]) > This seems OK. It's a bit unclear to me when to use nonPseudoNode and when to use node() however. Hmm yeah. I'll rename it nodeForEditing() or something in a different patch to make it's uses more clear.
WebKit Review Bot
Comment 9 2012-12-20 16:16:29 PST
Comment on attachment 180411 [details] Patch Clearing flags on attachment: 180411 Committed r138317: <http://trac.webkit.org/changeset/138317>
WebKit Review Bot
Comment 10 2012-12-20 16:16:33 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.