Closed Bug 1220925 Opened 4 years ago Closed 4 years ago

Event::GetScreenCoords should return CSSIntPoint instead of LayoutDevicePoint

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Update Event::GetScreenCoord so that it returns a CSSIntPoint instead of a LayoutDevicePoint with containing CSSIntPoint values.
Assignee: nobody → rbarker
Attachment #8683238 - Flags: review?(botond)
Comment on attachment 8683238 [details] [diff] [review]
0001-Bug-1220925-Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110411-12fe91b.patch

Need to address try bustage with latest patch.
Attachment #8683238 - Flags: review?(botond)
Attachment #8683336 - Flags: review?(botond)
Comment on attachment 8683336 [details] [diff] [review]
0001-Bug-1220925-Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110413-bc1d768.patch

Review of attachment 8683336 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed.

We should be on alert for any regressions caused by this, as it's changing the behaviour of a pretty core piece of code in some cases. I believe the change makes the behaviour correct, but who knows what may have been relying on the old (incorrect) behaviour...

::: dom/events/Event.cpp
@@ +933,5 @@
>    }
>  
>    WidgetGUIEvent* guiEvent = aEvent->AsGUIEvent();
>    if (!guiEvent->widget) {
> +    return RoundedToInt(aPoint / aPresContext->CSSToDevPixelScale());

CSSToDevPixelScale() uses AppUnitsPerDevPixel(), whereas below we use AppUnitsPerDevPixelAtUnitFullZoom(). I think we should be doing the same in both cases.

I'd suggest the following:

  WidgetGUIEvent* guiEvent = aEvent->AsGUIEvent();
  LayoutDeviceIntPoint offset = aPoint;
  if (guiEvent->widget) {
    offset += guiEvent->widget->WidgetToScreenOffset();
  }
  nsPoint pt = ...;

::: dom/events/UIEvent.cpp
@@ +363,4 @@
>      Event::GetScreenCoords(mPresContext, mEvent, mEvent->refPoint);
>    nsresult rv = Event::DuplicatePrivateData();
>    if (NS_SUCCEEDED(rv)) {
> +    mEvent->refPoint = mPresContext ? RoundedToInt(screenPoint * mPresContext->CSSToDevPixelScale()) : LayoutDeviceIntPoint(screenPoint.x, screenPoint.y);

Would prefer:

  CSSToLayoutDeviceScale scale = mPresContext ? mPresContext->CSSToDevPixelScale()
                                              : CSSToLayoutDeviceScale(1);
  mEvent->refPoint = RoundedToInt(screenPoint * scale);

::: dom/events/UIEvent.h
@@ -39,5 @@
>    NS_IMETHOD DuplicatePrivateData() override;
>    NS_IMETHOD_(void) Serialize(IPC::Message* aMsg, bool aSerializeInterfaceType) override;
>    NS_IMETHOD_(bool) Deserialize(const IPC::Message* aMsg, void** aIter) override;
>  
> -  static LayoutDeviceIntPoint CalculateScreenPoint(nsPresContext* aPresContext,

Nice find! (The duplication that you're removing, I mean.)
Attachment #8683336 - Flags: review?(botond) → review+
Attachment #8683336 - Attachment is obsolete: true
Keywords: checkin-needed
Hi, this didn;t apply cleanly:

applying -Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110909-2aed3e7.patch
patching file dom/events/Touch.cpp
Hunk #2 FAILED at 48
1 out of 2 hunks FAILED -- saving rejects to file dom/events/Touch.cpp.rej
patching file dom/events/Touch.h
Hunk #1 FAILED at 75
1 out of 1 hunks FAILED -- saving rejects to file dom/events/Touch.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh -Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110909-2aed3e7.patch
Flags: needinfo?(rbarker)
Keywords: checkin-needed
I have a rebased version ready to land. Just waiting for the tree to reopen.
Flags: needinfo?(rbarker)
https://hg.mozilla.org/mozilla-central/rev/a26be9d0cfed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1224754
Blocks: 1228216
You need to log in before you can comment on or make changes to this bug.