[Webkit-unassigned] [Bug 135434] Hit-testing broken in WebKit 1 views with AppKit's contentInsets

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135434





--- Comment #7 from Beth Dakin <bdakin at apple.com>  2014-07-31 12:33:10 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.