WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
135434
Hit-testing broken in WebKit 1 views with AppKit's contentInsets
https://bugs.webkit.org/show_bug.cgi?id=135434
Summary
Hit-testing broken in WebKit 1 views with AppKit's contentInsets
Beth Dakin
Reported
2014-07-30 15:30:37 PDT
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
>
Attachments
Patch
(12.65 KB, patch)
2014-07-30 15:50 PDT
,
Beth Dakin
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
(1.69 MB, application/zip)
2014-07-30 17:22 PDT
,
Build Bot
no flags
Details
Patch
(12.70 KB, patch)
2014-07-30 17:32 PDT
,
Beth Dakin
benjamin
: review+
Details
Formatted Diff
Diff
Patch with tests
(38.42 KB, patch)
2014-07-31 14:51 PDT
,
Beth Dakin
benjamin
: review+
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
(818.37 KB, application/zip)
2014-07-31 15:41 PDT
,
Build Bot
no flags
Details
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Beth Dakin
Comment 1
2014-07-30 15:50:42 PDT
Created
attachment 235774
[details]
Patch
Build Bot
Comment 2
2014-07-30 17:22:34 PDT
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
Build Bot
Comment 3
2014-07-30 17:22:37 PDT
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
Beth Dakin
Comment 4
2014-07-30 17:32:45 PDT
Created
attachment 235784
[details]
Patch
Benjamin Poulain
Comment 5
2014-07-30 19:59:44 PDT
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
Benjamin Poulain
Comment 6
2014-07-30 20:00:35 PDT
I would prefer if you added some testing infrastructure for this but I understand if you are time constrained.
Beth Dakin
Comment 7
2014-07-31 12:33:10 PDT
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.
Beth Dakin
Comment 8
2014-07-31 14:51:12 PDT
Created
attachment 235847
[details]
Patch with tests
Benjamin Poulain
Comment 9
2014-07-31 15:28:03 PDT
Comment on
attachment 235847
[details]
Patch with tests Thanks for the tests!
Build Bot
Comment 10
2014-07-31 15:41:29 PDT
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
Build Bot
Comment 11
2014-07-31 15:41:32 PDT
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
Beth Dakin
Comment 12
2014-07-31 15:45:44 PDT
Committed fix with
http://trac.webkit.org/changeset/171891
and fixed that failing test with
http://trac.webkit.org/changeset/171892
Tim Horton
Comment 13
2014-07-31 18:29:02 PDT
And a build fix in
http://trac.webkit.org/changeset/171905
Tim Horton
Comment 14
2014-08-01 11:31:27 PDT
And another build fix in
http://trac.webkit.org/changeset/171932
Simon Fraser (smfr)
Comment 15
2014-08-01 12:02:15 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug