Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Event::GetScreenCoords should return CSSIntPoint instead of LayoutDevicePoint

RESOLVED FIXED in Firefox 45

Status

()

Core
DOM: Events
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

(Blocks: 1 bug)

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
Update Event::GetScreenCoord so that it returns a CSSIntPoint instead of a LayoutDevicePoint with containing CSSIntPoint values.
(Assignee)

Updated

2 years ago
Assignee: nobody → rbarker
(Assignee)

Comment 1

2 years ago
Created attachment 8682260 [details] [diff] [review]
0001-Fix-Event-GetScreenCoord-to-return-CSSIntPoint-15110216-b73c75a.patch
(Assignee)

Comment 2

2 years ago
Created attachment 8683238 [details] [diff] [review]
0001-Bug-1220925-Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110411-12fe91b.patch
Attachment #8682260 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d3a66316bc3
(Assignee)

Updated

2 years ago
Attachment #8683238 - Flags: review?(botond)
(Assignee)

Comment 4

2 years ago
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)
(Assignee)

Comment 5

2 years ago
Created attachment 8683335 [details] [diff] [review]
0001-try-b-do-p-linux-linux64-macosx64-win32-android-api-9-android-api-11-emulator-u-all-t-none-15110413-b1e21a7.patch
Attachment #8683238 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
Created attachment 8683336 [details] [diff] [review]
0001-Bug-1220925-Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110413-bc1d768.patch
Attachment #8683335 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=543f31866f51
(Assignee)

Updated

2 years ago
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+
(Assignee)

Comment 9

2 years ago
Created attachment 8684975 [details] [diff] [review]
0001-Bug-1220925-Event-GetScreenCoords-should-return-CSSIntPoint-instead-of-LayoutDevicePoint-15110909-2aed3e7.patch

Address review comments. Carry forward r+ from :botond

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4f334b2d473
(Assignee)

Updated

2 years ago
Attachment #8683336 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
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
Blocks: 1206872
I have a rebased version ready to land. Just waiting for the tree to reopen.
Flags: needinfo?(rbarker)

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a26be9d0cfed
https://hg.mozilla.org/mozilla-central/rev/a26be9d0cfed
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

2 years ago
Blocks: 1224754
(Assignee)

Updated

2 years ago
Blocks: 1228216
You need to log in before you can comment on or make changes to this bug.