Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(466)

Issue 1201193005: Move UIEvent.pageX/pageY/layerX/layerY to MouseEvent (Closed)

Created:
5 years, 6 months ago by dtapuska
Modified:
5 years, 5 months ago
CC:
blink-reviews, vivekg, arv+blink, Inactive, vivekg_samsung
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Move UIEvent.pageX/pageY/layerX/layerY to MouseEvent Bring the MouseEvent to match the DOM specification a little more by moving these fields to the MouseEvent from the UIEvent. These fields were previously always reported as 0 as non-mouse events and provided no real value. This matches the IE implementation but deviates from the FireFox and Safari implementations. Approved Intent to Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pcMwyHRhbCU BUG=503274, 503276 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198226

Patch Set 1 #

Total comments: 8

Patch Set 2 : Rebase and fix nits #

Patch Set 3 : Rebase again; UseCounter changed already again #

Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -102 lines) Patch
M LayoutTests/fast/events/init-events-expected.txt View 3 chunks +0 lines, -12 lines 0 comments Download
M LayoutTests/fast/events/script-tests/init-events.js View 3 chunks +0 lines, -12 lines 0 comments Download
M LayoutTests/fast/events/touch/basic-multi-touch-events-expected.txt View 4 chunks +0 lines, -8 lines 0 comments Download
M LayoutTests/fast/events/touch/basic-multi-touch-events-limited-expected.txt View 3 chunks +0 lines, -6 lines 0 comments Download
M LayoutTests/fast/events/touch/basic-single-touch-events-expected.txt View 4 chunks +0 lines, -10 lines 0 comments Download
M LayoutTests/fast/events/touch/script-tests/basic-multi-touch-events.js View 1 chunk +0 lines, -2 lines 0 comments Download
M LayoutTests/fast/events/touch/script-tests/basic-multi-touch-events-limited.js View 1 chunk +0 lines, -2 lines 0 comments Download
M LayoutTests/fast/events/touch/script-tests/basic-single-touch-events.js View 1 chunk +0 lines, -2 lines 0 comments Download
M LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt View 1 2 chunks +4 lines, -4 lines 0 comments Download
M LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 chunks +4 lines, -4 lines 0 comments Download
M Source/core/events/MouseEvent.idl View 1 2 chunks +4 lines, -0 lines 0 comments Download
M Source/core/events/MouseRelatedEvent.h View 1 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/events/UIEvent.h View 1 chunk +0 lines, -6 lines 0 comments Download
M Source/core/events/UIEvent.cpp View 1 chunk +0 lines, -20 lines 0 comments Download
M Source/core/events/UIEvent.idl View 1 chunk +0 lines, -6 lines 0 comments Download
M Source/core/frame/UseCounter.h View 1 2 2 chunks +2 lines, -4 lines 0 comments Download

Messages

Total messages: 12 (3 generated)
dtapuska
5 years, 6 months ago (2015-06-23 16:52:30 UTC) #2
Rick Byers
Please add a link to the intent to ship thread to your CL description. LGTM ...
5 years, 6 months ago (2015-06-23 19:33:00 UTC) #3
dtapuska
https://codereview.chromium.org/1201193005/diff/1/Source/core/events/MouseEvent.idl File Source/core/events/MouseEvent.idl (right): https://codereview.chromium.org/1201193005/diff/1/Source/core/events/MouseEvent.idl#newcode28 Source/core/events/MouseEvent.idl:28: readonly attribute long pageX; On 2015/06/23 19:33:00, Rick Byers ...
5 years, 6 months ago (2015-06-23 19:36:39 UTC) #4
Rick Byers
https://codereview.chromium.org/1201193005/diff/1/Source/core/events/MouseEvent.idl File Source/core/events/MouseEvent.idl (right): https://codereview.chromium.org/1201193005/diff/1/Source/core/events/MouseEvent.idl#newcode28 Source/core/events/MouseEvent.idl:28: readonly attribute long pageX; On 2015/06/23 19:36:39, Dave Tapuska ...
5 years, 6 months ago (2015-06-23 19:44:47 UTC) #5
philipj_slow
I think you'll have to update LayoutTests/webexposed/ and LayoutTests/virtual/stable/webexposed/. Then maybe run the bots to ...
5 years, 5 months ago (2015-07-01 14:17:46 UTC) #6
philipj_slow
Oh, that was actually an accidental LGTM, but I'll let it stand, this will be ...
5 years, 5 months ago (2015-07-01 14:18:46 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1201193005/40001
5 years, 5 months ago (2015-07-02 16:41:46 UTC) #10
commit-bot: I haz the power
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198226
5 years, 5 months ago (2015-07-02 16:45:36 UTC) #11
dtapuska
5 years, 5 months ago (2015-07-02 17:32:35 UTC) #12
Message was sent while issue was closed.
Updated webexposed tests; not sure why I didn't catch that the first time round;
but thanks!

https://codereview.chromium.org/1201193005/diff/1/Source/core/events/MouseEve...
File Source/core/events/MouseEvent.idl (right):

https://codereview.chromium.org/1201193005/diff/1/Source/core/events/MouseEve...
Source/core/events/MouseEvent.idl:28: readonly attribute long             pageX;
On 2015/07/01 14:17:46, philipj wrote:
> On 2015/06/23 19:44:46, Rick Byers wrote:
> > On 2015/06/23 19:36:39, Dave Tapuska wrote:
> > > On 2015/06/23 19:33:00, Rick Byers wrote:
> > > > nit: Philip has been trying to align the interfaces with the specs. 
> Please
> > > move
> > > > these to a new section below with a comment referencing
> > > > http://www.w3.org/TR/cssom-view/#extensions-to-the-mouseevent-interface
> > > 
> > > I was trying to keep the idl matching the spec (for ordering).
> > > 
> > > I'd argue that this class needs to be re-organized to match that; I can
put
> > that
> > > reference to the spec above screenX...
> > > 
> > > and move the x,y,offsetX,offsetY from the Non-standard section.
> > > 
> > 
> > The official home of this event definition is
> > https://w3c.github.io/uievents/#interface-MouseEvent (which is the top-level
> > comment for this file).
> > 
> > CSSOM View defines some changes / extensions (mostly, I assume, because
CSSOM
> > View is where fractional positioning comes from).  Until the time that we
> switch
> > to 'double', we're really implemewnting the UIEvents version of this API.
> > 
> > So I'd suggest putting pageX/pageY, offsetX,offsetY and x,y in a new section
> > referencing the CSSOM View spec with a "TODO: Should be double instead of
long
> > (http://crbug.com/456625)%22.
> > 
> > Or if you prefer you can ALL the co-ordinate members down into the new CSSOM
> > View section (ordered as in that spec).  But hopefully the UIEvents spec
will
> > eventually be updated to incorporate the improvements and additions from
CSSOM
> > View.
> 
> I made a mistake when cleaning up this file, I missed that CSSOM View defines
> some of what I documented as non-standard. I've rectified this in
> https://codereview.chromium.org/1214923005/ which I hope will land soon.
(Rick,
> maybe you can LGTM and CQ if Jens isn't around?)
> 
> When that lands, this CL should be a matter of moving pageX/Y into that new
> section, and layerX/Y into the non-standard section.

Done.

https://codereview.chromium.org/1201193005/diff/1/Source/core/events/MouseEve...
Source/core/events/MouseEvent.idl:74: [MeasureAs=MouseEventLayerX] readonly
attribute long layerX;
On 2015/07/01 14:17:46, philipj wrote:
> I would suggest just using [Measure] and using the generated names. (It's
> newish, so you don't see it everywhere.)

Done.

Powered by Google App Engine
This is Rietveld 408576698