On Yosemite, hit-testing is currently broken in WebKit 1 views with AppKit's contentInsets. This problem is immediately reproducible in MiniBrowser on Yosemite -- all hit-testing is offset by the top inset. <rdar://problem/17850323>
Created attachment 235774 [details] Patch
Comment on attachment 235774 [details] Patch Attachment 235774 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4932596169441280 New failing tests: platform/mac/fast/scrolling/scroll-select-bottom-test.html editing/selection/caret-at-bidi-boundary.html
Created attachment 235782 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 235784 [details] Patch
Comment on attachment 235784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=235784&action=review This makes sense. > Source/WebCore/ChangeLog:10 > + AppKitâs contentInsets are factored into scroll positions and mouse positions, but Some encoding issues in the changelog...could be bugzilla. > Source/WebCore/page/FrameView.cpp:3943 > + rect.moveBy(-scrollPosition() + IntPoint(0, headerHeight() + topContentInset(ManualOrPlatformContentInset))); The name convertFromRenderer is quite confusing with the topContentInset. Shouldn't it be renamed convertFromRendererToContainingView()? > Source/WebCore/page/FrameView.cpp:3968 > - point.moveBy(-scrollPosition() + IntPoint(0, headerHeight() + topContentInset())); > + point.moveBy(-scrollPosition() + IntPoint(0, headerHeight() + topContentInset(ManualOrPlatformContentInset))); ditto. > Source/WebCore/platform/ScrollView.h:164 > + // will be returned instead of the value set on Page. In the future, we could obviously expand this enum to include > + // a value for only PlatformContentInset, but that value is not needed at this time. You can drop the sentence: "In the future, we could obviously expand this enum to include"... > Source/WebCore/platform/ScrollView.h:165 > + enum TopContentInsetType { ManualContentInset, ManualOrPlatformContentInset }; "Manual" is a bit generic. What about WebKitContentInset or InternalContentInset? I like typed enum better (e.g. TopContentInsetType::Platform), but that's personal preference. > Source/WebCore/platform/mac/ScrollViewMac.mm:176 > + // AppKit has the inset factored into all of its scroll positions. In WebCore, we use positons that ignore typo: positons
I would prefer if you added some testing infrastructure for this but I understand if you are time constrained.
Thank you Benjamin! (In reply to comment #5) > (From update of attachment 235784 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=235784&action=review > > This makes sense. > > > Source/WebCore/ChangeLog:10 > > + AppKitâs contentInsets are factored into scroll positions and mouse positions, but > > Some encoding issues in the changelog...could be bugzilla. > Will fix. I think these are from TextEdit. :| > > Source/WebCore/page/FrameView.cpp:3943 > > + rect.moveBy(-scrollPosition() + IntPoint(0, headerHeight() + topContentInset(ManualOrPlatformContentInset))); > > The name convertFromRenderer is quite confusing with the topContentInset. Shouldn't it be renamed convertFromRendererToContainingView()? > That's a good call. I will make this change. > > Source/WebCore/page/FrameView.cpp:3968 > > - point.moveBy(-scrollPosition() + IntPoint(0, headerHeight() + topContentInset())); > > + point.moveBy(-scrollPosition() + IntPoint(0, headerHeight() + topContentInset(ManualOrPlatformContentInset))); > > ditto. > Same. > > Source/WebCore/platform/ScrollView.h:164 > > + // will be returned instead of the value set on Page. In the future, we could obviously expand this enum to include > > + // a value for only PlatformContentInset, but that value is not needed at this time. > > You can drop the sentence: "In the future, we could obviously expand this enum to include"... > I will remove it. > > Source/WebCore/platform/ScrollView.h:165 > > + enum TopContentInsetType { ManualContentInset, ManualOrPlatformContentInset }; > > "Manual" is a bit generic. What about WebKitContentInset or InternalContentInset? > > I like typed enum better (e.g. TopContentInsetType::Platform), but that's personal preference. > Yeah, those are very nice. Will use. > > Source/WebCore/platform/mac/ScrollViewMac.mm:176 > > + // AppKit has the inset factored into all of its scroll positions. In WebCore, we use positons that ignore > > typo: positons Good catch! I'm going to spend a few minutes to see if I can get some tests up and running. I actually do have WK2 tests for this stuff, and it would be cool if we could get WK1 coverage as well.
Created attachment 235847 [details] Patch with tests
Comment on attachment 235847 [details] Patch with tests Thanks for the tests!
Comment on attachment 235847 [details] Patch with tests Attachment 235847 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4581506215313408 New failing tests: platform/mac-wk2/tiled-drawing/header-and-footer-hit-testing-in-frame.html
Created attachment 235857 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Committed fix with http://trac.webkit.org/changeset/171891 and fixed that failing test with http://trac.webkit.org/changeset/171892
And a build fix in http://trac.webkit.org/changeset/171905
And another build fix in http://trac.webkit.org/changeset/171932
Comment on attachment 235847 [details] Patch with tests View in context: https://bugs.webkit.org/attachment.cgi?id=235847&action=review > Source/WebCore/page/FrameView.h:393 > - IntRect convertFromRenderer(const RenderElement*, const IntRect&) const; > - IntRect convertToRenderer(const RenderElement*, const IntRect&) const; > - IntPoint convertFromRenderer(const RenderElement*, const IntPoint&) const; > - IntPoint convertToRenderer(const RenderElement*, const IntPoint&) const; > + IntRect convertFromRendererToContainingView(const RenderElement*, const IntRect&) const; > + IntRect convertFromContainingViewToRenderer(const RenderElement*, const IntRect&) const; > + IntPoint convertFromRendererToContainingView(const RenderElement*, const IntPoint&) const; > + IntPoint convertFromContainingViewToRenderer(const RenderElement*, const IntPoint&) const; I think this rename adds ambiguity: it's no longer clear whether it's converting to This view or This view's containing view.