Bug 135434 – Hit-testing broken in WebKit 1 views with AppKit's contentInsets

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

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>

[Beth Dakin]

Created attachment 235774 [details]
Patch

[Build Bot]

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]

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]

Created attachment 235784 [details]
Patch

[Benjamin Poulain]

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]

I would prefer if you added some testing infrastructure for this but I understand if you are time constrained.

[Beth Dakin]

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]

Created attachment 235847 [details]
Patch with tests

[Benjamin Poulain]

Comment on attachment 235847 [details]
Patch with tests

Thanks for the tests!

[Build Bot]

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]

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]

Committed fix with http://trac.webkit.org/changeset/171891 and fixed that failing test with http://trac.webkit.org/changeset/171892

[Tim Horton]

And a build fix in http://trac.webkit.org/changeset/171905

[Tim Horton]

And another build fix in http://trac.webkit.org/changeset/171932

[Simon Fraser (smfr)]

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.