EventHandler::hitTestResultAtPoint and Document::nodesFromRect both have a list a boolean arguments that could be expressed as a HitTestRequest::Type instead. EventHandler::hitTestResultAtPoint already takes a HitTestRequest as an argument and chould just lose the unnecessary boolean arguments, while Document::nodesFromRect should be converted to taking HitTestRequest arguments instead. Bug based on review suggestions in https://bugs.webkit.org/show_bug.cgi?id=95204#c16
Created attachment 162001 [details] Patch
Created attachment 162019 [details] Patch Reupload to let EWS try again now that its depending patch has landed.
Comment on attachment 162019 [details] Patch Attachment 162019 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13739527
Comment on attachment 162019 [details] Patch Attachment 162019 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13746453
Comment on attachment 162019 [details] Patch Forgot to update one symbol for Mac, and some calls for EFL.
Created attachment 162186 [details] Patch
Comment on attachment 162186 [details] Patch Attachment 162186 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13746793
Created attachment 162198 [details] Patch Fix EFL build
Comment on attachment 162198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162198&action=review > Source/WebCore/rendering/HitTestRequest.h:51 > + static HitTestRequestType Default(bool allowShadowContent, bool ignoreClipping = false) What about using flags? HitTestResult::Default(ALLOW_SHADOW_CONTENT | CLIP) That is more clear
(In reply to comment #9) > (From update of attachment 162198 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162198&action=review > > > Source/WebCore/rendering/HitTestRequest.h:51 > > + static HitTestRequestType Default(bool allowShadowContent, bool ignoreClipping = false) > > What about using flags? > > HitTestRequest::Default(ALLOW_SHADOW_CONTENT | CLIP) > > That is more clear True, but if you want refactor it like that, you just stop using the arguments to HitTestRequest::Default and write it like HitTestRequest::Default() | HitTestRequest::AllowChildContent | HitTestRequest::IgnoreClipping. The reason the function exists and takes boolean arguments is to make it easy and safe to upgrade existing calls.
Comment on attachment 162198 [details] Patch Idea is good, but I am really not a big fan of the HitTestRequest::Default(true|false) new idiom. While looking over the patch, the calls Default(true|false) were making things equally non-obvious than before. IMO, the APIs we improved were degraded by the suggested alternative.
(In reply to comment #11) > (From update of attachment 162198 [details]) > Idea is good, but I am really not a big fan of the HitTestRequest::Default(true|false) new idiom. > > While looking over the patch, the calls Default(true|false) were making things equally non-obvious than before. > > IMO, the APIs we improved were degraded by the suggested alternative. What about removing the boolean arguments from Default and calling something like HitTestRequest::Default() | HitTestRequest::AllowChildContent | HitTestRequest::IgnoreClipping ?
> What about removing the boolean arguments from Default and calling something like HitTestRequest::Default() | HitTestRequest::AllowChildContent | HitTestRequest::IgnoreClipping ? Looks cleaner to me. I think kenneth would be happier as well. Would "default" be empty?
Created attachment 162262 [details] Patch Eliminated the use of boolean arguments in the redefinition
(In reply to comment #13) > > What about removing the boolean arguments from Default and calling something like HitTestRequest::Default() | HitTestRequest::AllowChildContent | HitTestRequest::IgnoreClipping ? > > Looks cleaner to me. I think kenneth would be happier as well. > > Would "default" be empty? No it is ReadOnly | Active currently. If I manage to get rid of side-effects from hit-testing, Default can be redefined to 0, as ReadOnly is no longer needed to make the hit-test just hit-test.
Comment on attachment 162262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162262&action=review > Source/WebCore/ChangeLog:13 > + To allow for simple conversion a new static function has been added to HitTestRequest to build > + a default type based on the old boolean arguments. I forgot to remove this from the ChangeLog. Will remove before landing.
Comment on attachment 162262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162262&action=review Getting close... Last question is: is that clear that Default means Active + ReadOnly, nomenclature-wise or API-wise? > Source/WebCore/page/EventHandler.cpp:1071 > - if (!allowShadowContent) > + if (!request.allowsShadowContent()) > result.setToNonShadowAncestor(); From what I remember, setToNonShadowAncestor and the HitTestRequest::allowShadowContent were not working together: the first were about point-based hit test, the latter, about rect-based. Mind to confirm? > Source/WebCore/rendering/HitTestRequest.h:45 > + static const HitTestRequestType Default = ReadOnly | Active; That is the key point. > Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:499 > + HitTestResult result(contentPos, topPadding, rightPadding, bottomPadding, leftPadding); > + if (m_targetType != Text) > + request |= HitTestRequest::AllowShadowContent; nit: swap these two lines, so the request gets constructed altogether
(In reply to comment #17) > (From update of attachment 162262 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162262&action=review > > Getting close... > > Last question is: is that clear that Default means Active + ReadOnly, nomenclature-wise or API-wise? > I guess not, but I was hoping to redefine it later, and that makes a generic name more suitable. > > Source/WebCore/page/EventHandler.cpp:1071 > > - if (!allowShadowContent) > > + if (!request.allowsShadowContent()) > > result.setToNonShadowAncestor(); > > From what I remember, setToNonShadowAncestor and the HitTestRequest::allowShadowContent were not working together: the first were about point-based hit test, the latter, about rect-based. > > Mind to confirm? > You are right. What this does is to ensure that the returned innerNode is not a shadow node, if the caller has requested no shadow nodes.
(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 162262 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=162262&action=review > > > > Getting close... > > > > Last question is: is that clear that Default means Active + ReadOnly, nomenclature-wise or API-wise? > > > I guess not, but I was hoping to redefine it later, and that makes a generic name more suitable. > We could have s/Default/ActiveAndReadOnly for convenience? > > > Source/WebCore/page/EventHandler.cpp:1071 > > > - if (!allowShadowContent) > > > + if (!request.allowsShadowContent()) > > > result.setToNonShadowAncestor(); > > > > From what I remember, setToNonShadowAncestor and the HitTestRequest::allowShadowContent were not working together: the first were about point-based hit test, the latter, about rect-based. > > > > Mind to confirm? > > > You are right. What this does is to ensure that the returned innerNode is not a shadow node, if the caller has requested no shadow nodes.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > (From update of attachment 162262 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=162262&action=review > > > > > > Getting close... > > > > > > Last question is: is that clear that Default means Active + ReadOnly, nomenclature-wise or API-wise? > > > > > I guess not, but I was hoping to redefine it later, and that makes a generic name more suitable. > > > > We could have s/Default/ActiveAndReadOnly for convenience? > There is another point to this. Active shouldn't really have any effect. It only does two things in hit-testing. If readonly is not set, it is used to update hover/active state, with ReadOnly set and it is only used in RenderLayer to make root-layers claim all tests hit it. Most of the ReadOnly hit-tests probably shouldn't even have Active set. One of the things I am hoping to do later, is find the ones that need Active, and set it on them and let the rest use only ReadOnly. If you don't like the concept of Default here, I think the alternative is to get rid of it and expand the values in full everywhere. It is pretty much what you suggest anyway with ActiveAndReadOnly, but it means any future improvements to what default is, would require changing all these ports again to update the calls.
> If you don't like the concept of Default here, I think the alternative is to get rid of it and expand the values in full everywhere. It is pretty much what you suggest anyway with ActiveAndReadOnly, but it means any future improvements to what default is, would require changing all these ports again to update the calls. I think that is preferable. Kenneth?
(In reply to comment #21) > > If you don't like the concept of Default here, I think the alternative is to get rid of it and expand the values in full everywhere. It is pretty much what you suggest anyway with ActiveAndReadOnly, but it means any future improvements to what default is, would require changing all these ports again to update the calls. > > I think that is preferable. Kenneth? Me too
Comment on attachment 162262 [details] Patch Attachment 162262 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13766734 New failing tests: fast/forms/drag-out-of-textarea.html touchadjustment/nested-shadow-node.html editing/pasteboard/drop-text-without-selection.html touchadjustment/search-cancel.html editing/pasteboard/drop-text-events.html editing/pasteboard/drag-drop-input-textarea.html editing/selection/drag-text-delay.html fast/events/5056619.html fast/forms/drag-into-textarea.html editing/pasteboard/drag-drop-url-text.html fast/events/content-changed-during-drop.html
Comment on attachment 162262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162262&action=review I seem to have managed to mix up the parameters in a few places which is why the tests fail now. > Source/WebCore/page/EventHandler.cpp:1010 > - HitTestResult hitTest = hitTestResultAtPoint(m_panScrollStartPos, true); > + HitTestResult hitTest = hitTestResultAtPoint(m_panScrollStartPos, HitTestRequest::Default | HitTestRequest::AllowChildFrameContent); Wrong autocomplete, should AllowShadowContent. > Source/WebCore/page/EventHandler.cpp:2483 > - HitTestResult result = hitTestResultAtPoint(hitTestPoint, /*allowShadowContent*/ true, /*ignoreClipping*/ false, DontHitTestScrollbars, hitType, touchRadius); > + HitTestResult result = hitTestResultAtPoint(hitTestPoint, HitTestRequest::Default | HitTestRequest::AllowChildFrameContent, touchRadius); Ditto > Source/WebCore/page/EventHandler.cpp:2501 > - HitTestResult result = hitTestResultAtPoint(hitTestPoint, /*allowShadowContent*/ true, /*ignoreClipping*/ false, DontHitTestScrollbars, hitType, touchRadius); > + HitTestResult result = hitTestResultAtPoint(hitTestPoint, HitTestRequest::Default | HitTestRequest::AllowChildFrameContent, touchRadius); Ditto > Source/WebCore/page/Frame.cpp:727 > - HitTestResult result = eventHandler()->hitTestResultAtPoint(framePoint, true); > + HitTestResult result = eventHandler()->hitTestResultAtPoint(framePoint, HitTestRequest::Default | HitTestRequest::AllowChildFrameContent); Ditto
Created attachment 162720 [details] Patch
Comment on attachment 162720 [details] Patch Attachment 162720 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13772710
Comment on attachment 162720 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162720&action=review > Source/WebKit/efl/ewk/ewk_frame.cpp:698 > + WebCore::HitTestRequest::ReadOnly | HitTestRequest::Active | WebCore::HitTestRequest::IgnoreClipping); It looks "WebCore::" is missing. s/HitTestRequest::Active/WebCore::HitTestRequest::Active/g
Comment on attachment 162720 [details] Patch Attachment 162720 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13768764
Created attachment 162752 [details] Patch Fix EFL build
Comment on attachment 162752 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162752&action=review > Source/WebCore/page/EventHandler.cpp:1070 > - if (!allowShadowContent) > + if (!request.allowsShadowContent()) > result.setToNonShadowAncestor(); so is this ok?
(In reply to comment #30) > (From update of attachment 162752 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162752&action=review > > > Source/WebCore/page/EventHandler.cpp:1070 > > - if (!allowShadowContent) > > + if (!request.allowsShadowContent()) > > result.setToNonShadowAncestor(); > > so is this ok? It duplicates what the code did before. Removing it would be changing functionality. Essentially the old allowShadowContent argument applied to both point based and area based tests, but since the HitTestRequest flag only applies automatically to area-based tests, this code ensures it is also applied to point-based tests.
Comment on attachment 162752 [details] Patch Clearing flags on attachment: 162752 Committed r127876: <http://trac.webkit.org/changeset/127876>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 96600
I'm going rollout this patch, bug 96600. Sorry about this. It seems this patch changed calculation of event position of mouse click on iframe. Spin button, input type "number", doesn't work since this change. See http://crbug.com/148842 for bug report. Please try following URI for regression checking. http://jsfiddle.net/EdP8j/ Thanks for your understanding.
I'm trying to add tests for clicking spin button in iframe: https://bugs.webkit.org/show_bug.cgi?id=96612
(In reply to comment #36) > I'm trying to add tests for clicking spin button in iframe: > https://bugs.webkit.org/show_bug.cgi?id=96612 Landed: http://trac.webkit.org/changeset/128423 New tests are: * fast/forms/number/number-spinbutton-click-in-iframe.html * fast/forms/time-multiple-fields/time-multiple-fields-spinbutton-click-in-iframe.html (for Chromium port only, or ENABLE_INPUT_TYPE_TIME_MULTIPLE_FIELDS enabled)
(In reply to comment #35) > I'm going rollout this patch, bug 96600. Sorry about this. > > It seems this patch changed calculation of event position of mouse click on iframe. > This particular patch shouldn't have any functional effect. It seems more like that a call wasn't converted correctly to the new API. Since these form-elements use shadow-dom, I suspect a call missing AllowShadowContent.
Created attachment 188065 [details] Patch Rebase and update to apply before patch for bug #95204
Attachment 188065 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.order', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/DragController.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/TouchDisambiguation.cpp', u'Source/WebCore/rendering/HitTestRequest.h', u'Source/WebCore/testing/Internals.cpp', u'Source/WebKit/blackberry/Api/WebPage.cpp', u'Source/WebKit/blackberry/ChangeLog', u'Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/ContextMenuClientImpl.cpp', u'Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp', u'Source/WebKit/chromium/src/WebFrameImpl.cpp', u'Source/WebKit/chromium/src/WebPluginContainerImpl.cpp', u'Source/WebKit/chromium/src/WebViewImpl.cpp', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/ewk/ewk_frame.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm', u'Source/WebKit/mac/WebView/WebHTMLView.mm', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebFrameAdapter.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebActionPropertyBag.cpp', u'Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in', u'Source/WebKit/win/WebView.cpp', u'Source/WebKit/wx/ChangeLog', u'Source/WebKit/wx/WebFrame.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp', u'Source/WebKit2/WebProcess/WebPage/WebFrame.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm', u'Source/autotools/symbols.filter']" exit_code: 1 Source/WebCore/dom/Document.h:37: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/dom/Document.h:386: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/dom/Document.h:387: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/efl/ewk/ewk_frame.cpp:695: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 4 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 188066 [details] Patch Fix a three missed rebase failures
Attachment 188066 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.order', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/DragController.cpp', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/EventHandler.h', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/TouchDisambiguation.cpp', u'Source/WebCore/rendering/HitTestRequest.h', u'Source/WebCore/testing/Internals.cpp', u'Source/WebKit/blackberry/Api/WebPage.cpp', u'Source/WebKit/blackberry/ChangeLog', u'Source/WebKit/blackberry/WebKitSupport/SelectionHandler.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/ContextMenuClientImpl.cpp', u'Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp', u'Source/WebKit/chromium/src/WebFrameImpl.cpp', u'Source/WebKit/chromium/src/WebPluginContainerImpl.cpp', u'Source/WebKit/chromium/src/WebViewImpl.cpp', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/ewk/ewk_frame.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm', u'Source/WebKit/mac/WebView/WebFrame.mm', u'Source/WebKit/mac/WebView/WebHTMLView.mm', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebFrameAdapter.cpp', u'Source/WebKit/qt/WebCoreSupport/QWebPageAdapter.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebActionPropertyBag.cpp', u'Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in', u'Source/WebKit/win/WebView.cpp', u'Source/WebKit/wx/ChangeLog', u'Source/WebKit/wx/WebFrame.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp', u'Source/WebKit2/WebProcess/WebPage/WebFrame.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm', u'Source/autotools/symbols.filter']" exit_code: 1 Source/WebCore/dom/Document.h:386: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/dom/Document.h:387: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebKit/efl/ewk/ewk_frame.cpp:695: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 3 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 188066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188066&action=review r=me, but fix the bad change. > Source/WebKit/chromium/src/WebViewImpl.cpp:1094 > + HitTestRequest::HitTestRequestType hitType = HitTestRequest::ReadOnly | HitTestRequest::Active | ((zoomType == FindInPage) ? HitTestRequest::IgnoreClipping : 0); Nit: I would put the zoomType checks to the next line for readibility. > Source/WebKit/mac/WebView/WebHTMLView.mm:6205 > + HitTestRequest::HitTestRequestType hitType = HitTestRequest::ReadOnly | HitTestRequest::Active | (allow ? HitTestRequest::AllowShadowContent : 0); Same nit. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1766 > + HitTestResult result = mainframe->eventHandler()->hitTestResultAtPoint(mainframe->view()->windowToContents(point), HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::AllowShadowContent); This is wrong, it should be: HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::IgnoreClipping
Comment on attachment 188066 [details] Patch r=me on the WebKit2 part
Committed r142977: <http://trac.webkit.org/changeset/142977>
Comment on attachment 188066 [details] Patch Very nice clean up. Thanks for doing this!